Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14592 )
Change subject: IMPALA-2112: Support primary key/foreign key constraints as part of create table in Impala. ...................................................................... Patch Set 4: (19 comments) Thank you for your feedback and catching all the nits! I have addressed them. http://gerrit.cloudera.org:8080/#/c/14592/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14592/3//COMMIT_MSG@38 PS3, Line 38: CREATE TABLE pk(id INT, PRIMARY KEY(id) DISABLE, NOVALIDATE, RELY); > May worth to give some more examples. Without looking into the oracle doc, Done http://gerrit.cloudera.org:8080/#/c/14592/3//COMMIT_MSG@42 PS3, Line 42: CREATE TABLE pk(id INT, PRIMARY KEY(id) ENABLE, VALIDATE, RELY); > Is pk/fk not supported in local-catalog mode coordinators? Looks like chang Done http://gerrit.cloudera.org:8080/#/c/14592/3/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/14592/3/common/thrift/CatalogObjects.thrift@370 PS3, Line 370: 8: optional list<hive_metastore.SQLPrimaryKey> primary_keys, > Is it ok to reuse field id "8"? 8 was not used before. Somehow, the field numbers were out of order, I picked the lowest unused number. http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java@138 PS3, Line 138: > nit: need a space Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/cup/sql-parser.cup@518 PS3, Line 518: Use > nit: Need space and capitcal for first char. Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/cup/sql-parser.cup@1445 PS3, Line 1445: tbl_ > nit: no indention Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/cup/sql-parser.cup@1454 PS3, Line 1454: > nit: need indention Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/cup/sql-parser.cup@1497 PS3, Line 1497: null > Should it be key_ident? TableDef.ForeignKey.fkName is the unique name given to a FK constraint. In this change, we do not support user specified constraint names, they are generated using UUIDs in TableDef.java L591. Hence, this is always passed as null here. Plan is to add support for user specified constraint names like 'constraint "constraint_name" foreign key(id)...' in future. http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@20 PS3, Line 20: import java.util.ArrayList; > nit: move this line below to be together with "import com.google" stuffs. Done http://gerrit.cloudera.org:8080/#/c/14592/3/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/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@190 PS3, Line 190: // > nit: should be consistent with other comments that with one more space afte Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@255 PS3, Line 255: foreig > typo: foreign. Same in other places. Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@449 PS3, Line 449: for (ColumnDef colDef: columnDefs_) { > nit: May worth to give it a clear name or define it later before the last f Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@464 PS3, Line 464: > nit: simplify it to primaryKeyColNames_? Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@509 PS3, Line 509: if (foreignKeysList_ == null || foreignKeysList_.size() == 0) return; > Should check null first to avoid NPE. BTW, why not using foreignKeysList_ d Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@517 PS3, Line 517: if > nit: need a space Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@523 PS3, Line 523: ) > nit: redundant space Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@562 PS3, Line 562: > nit: need spaces arround Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@567 PS3, Line 567: } else { : con > nit: should be in same line in our code style, and one white space before " Done http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java: http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@257 PS3, Line 257: k > nit: comma in wrong line Done -- To view, visit http://gerrit.cloudera.org:8080/14592 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id03d8d4d41a2ac1b15e7060e2a013e334d044ee7 Gerrit-Change-Number: 14592 Gerrit-PatchSet: 4 Gerrit-Owner: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Wed, 06 Nov 2019 18:18:32 +0000 Gerrit-HasComments: Yes
