Vihang Karajgaonkar 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 7: (6 comments) Mostly looks good. I took a quick pass at this since Quanlong has already done a detailed review. Left some comments below after which its good to go from my side. http://gerrit.cloudera.org:8080/#/c/14592/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/14592/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@227 PS7, Line 227: arguments It would be good to add the information about the extra arguments which are null and what they imply (if not obvious by their names) http://gerrit.cloudera.org:8080/#/c/14592/7/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/7/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@255 PS7, Line 255: getCreateTableSql Can we add a test to the test_show_create_table e2e test to make sure that the output is being tested for tables with primary and foreign keys? http://gerrit.cloudera.org:8080/#/c/14592/7/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@388 PS7, Line 388: /** : * Helper to check if foreign keys exist. : */ : private static boolean getForeignKeysExist(List<String> foreignKeysSql) { : return foreignKeysSql != null && !foreignKeysSql.isEmpty(); : } : : /** : * Helper to check if primary keys exist. : */ : private static boolean getPrimaryKeysExist(List<String> primaryKeysSql) { : return primaryKeysSql != null && !primaryKeysSql.isEmpty(); : } Do we need a method for this? Can we use CollectionUtils.isNotEmpty() http://gerrit.cloudera.org:8080/#/c/14592/7/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@425 PS7, Line 425: if (getPrimaryKeysExist(primaryKeysSql) || getForeignKeysExist(foreignKeysSql)) This condition is redundant since the inner if blocks already check for them http://gerrit.cloudera.org:8080/#/c/14592/7/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/7/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@958 PS7, Line 958: // Get Primary keys info. : primaryKeys_.addAll(client.getPrimaryKeys( : new PrimaryKeysRequest(msTbl.getDbName(), msTbl.getTableName()))); : // Get foreign keys info. : foreignKeys_.addAll(client.getForeignKeys(new ForeignKeysRequest(null,null, : msTbl.getDbName(),msTbl.getTableName()))); Shouldn't this be under loadTableSchema flag? Also, Since these are 2 additional RPC calls to HMS, is there a way to issue these calls only if necessary? http://gerrit.cloudera.org:8080/#/c/14592/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/14592/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@2360 PS7, Line 2360: AnalysisError("create table foo(id int, year int, primary key (id, year) disable" : + " validate rely)", "VALIDATE feature is not supported yet.") Does it also error out if the primary key is created on a non-existing colname? If yes, can we add those cases here? -- 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: 7 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 23:06:36 +0000 Gerrit-HasComments: Yes
