Quanlong Huang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15213 )

Change subject: IMPALA-9256: Refactor constraint information into a separate 
class.
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/15213/1/fe/src/main/java/org/apache/impala/analysis/TableDef.java
File fe/src/main/java/org/apache/impala/analysis/TableDef.java:

http://gerrit.cloudera.org:8080/#/c/15213/1/fe/src/main/java/org/apache/impala/analysis/TableDef.java@578
PS1, Line 578:         if (fk.getFkConstraintName() == null) {
             :           if (i == 0){
nit: can combine these two. BTW, can we move it outside the loop?


http://gerrit.cloudera.org:8080/#/c/15213/1/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java:

http://gerrit.cloudera.org:8080/#/c/15213/1/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@169
PS1, Line 169:       sqlConstraints = new 
SqlConstraints(c.getHiveClient().getPrimaryKeys(
Should we check whether IMetaStoreClient.getPrimaryKeys and 
IMetaStoreClient.getForeignKeys return nulls here? Just worry on any bugs in 
Hive or future versions of Hive.


http://gerrit.cloudera.org:8080/#/c/15213/1/fe/src/main/java/org/apache/impala/service/MetadataOp.java
File fe/src/main/java/org/apache/impala/service/MetadataOp.java:

http://gerrit.cloudera.org:8080/#/c/15213/1/fe/src/main/java/org/apache/impala/service/MetadataOp.java@648
PS1, Line 648:           TResultRow row = new TResultRow();
Oops... Is this a bug? I'm curious why it didn't cause test failures before 
this patch. May worth to mention it in the commit message.


http://gerrit.cloudera.org:8080/#/c/15213/1/fe/src/test/java/org/apache/impala/service/JdbcTest.java
File fe/src/test/java/org/apache/impala/service/JdbcTest.java:

http://gerrit.cloudera.org:8080/#/c/15213/1/fe/src/test/java/org/apache/impala/service/JdbcTest.java@437
PS1, Line 437: pkCount - 1
Need assertion to avoid index overflow


http://gerrit.cloudera.org:8080/#/c/15213/1/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:

http://gerrit.cloudera.org:8080/#/c/15213/1/tests/hs2/test_hs2.py@508
PS1, Line 508: results = fetch_results_resp.results
nit: this should be moved outside since it's used outside the loop



--
To view, visit http://gerrit.cloudera.org:8080/15213
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f1c441c24df84d2d0791ffe94dff60d039a3341
Gerrit-Change-Number: 15213
Gerrit-PatchSet: 1
Gerrit-Owner: Anurag Mantripragada <anu...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <anu...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com>
Gerrit-Comment-Date: Fri, 14 Feb 2020 08:26:52 +0000
Gerrit-HasComments: Yes

Reply via email to