Quanlong Huang 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: Code-Review+1 (7 comments) 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, > 8 was not used before. Somehow, the field numbers were out of order, I pick In line 353, there's a removed field id 8. However, I think it's ok since THdfsTable is only used inside Impala cluster. It's not allowed to run catalogd and coordinators in different versions together. 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@1497 PS3, Line 1497: null > TableDef.ForeignKey.fkName is the unique name given to a FK constraint. In Oh, I realize that KW_FOREIGN is just "foreign" and key_ident is "key"! Thanks for explanation! http://gerrit.cloudera.org:8080/#/c/14592/4/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/4/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@28 PS4, Line 28: import org.apache.hadoop.hive.metastore.api.SQLPrimaryKey; nit: import lines are sorted. It'd be better to not break it. http://gerrit.cloudera.org:8080/#/c/14592/4/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@50 PS4, Line 50: import com.google.common.base.Joiner; nit: import lines are sorted. http://gerrit.cloudera.org:8080/#/c/14592/4/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/4/fe/src/main/java/org/apache/impala/analysis/TableDef.java@461 PS4, Line 461: } : else { nit: one line http://gerrit.cloudera.org:8080/#/c/14592/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/14592/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@233 PS4, Line 233: //P nit: need space http://gerrit.cloudera.org:8080/#/c/14592/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@958 PS4, Line 958: //G nit: need space -- 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: Thu, 07 Nov 2019 02:10:31 +0000 Gerrit-HasComments: Yes
