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 3: (19 comments) This patch generally looks good to me. I leave some comments mostly on code style or "nits". BTW, is there a JIRA for using these pk/fk meta in planner? E.g. revisit pk/fk codes in JoinNode.java? 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, I thought we shoud turn off them by: CREATE TABLE pk(id INT, PRIMARY KEY(id) DISABLE(false), NOVALIDATE(false), RELY); http://gerrit.cloudera.org:8080/#/c/14592/3//COMMIT_MSG@42 PS3, Line 42: Is pk/fk not supported in local-catalog mode coordinators? Looks like changes in LocalFsTable are empty. May worth to mention it in commit message. 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"? 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: p nit: need a space 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: used nit: Need space and capitcal for first char. http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/cup/sql-parser.cup@1445 PS3, Line 1445: nit: no indention http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/cup/sql-parser.cup@1454 PS3, Line 1454: nit: need indention 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? 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 com.google.common.base.Joiner; nit: move this line below to be together with "import com.google" stuffs. 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 after "//". http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@255 PS3, Line 255: forign typo: foreign. Same in other places. http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@449 PS3, Line 449: int cnt = 1; nit: May worth to give it a clear name or define it later before the last for-loop. http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@464 PS3, Line 464: getPrimaryKeyColumnNames() nit: simplify it to primaryKeyColNames_? http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@509 PS3, Line 509: if (getForeignKeysList().size() == 0 || getForeignKeysList() == null) return; Should check null first to avoid NPE. BTW, why not using foreignKeysList_ directly inside the class? 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 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 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 http://gerrit.cloudera.org:8080/#/c/14592/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@567 PS3, Line 567: } : else{ nit: should be in same line in our code style, and one white space before "{". 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: , nit: comma in wrong line -- 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: 3 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-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Wed, 06 Nov 2019 07:24:10 +0000 Gerrit-HasComments: Yes
