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 8: (6 comments) Thanks for the comments Vihang. I tried to address them. 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 Done 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: : Pass the correc > Can we add a test to the test_show_create_table e2e test to make sure that Observability changes like show create are not fully functional in this change. They are tracked in other JIRAS, for example: https://issues.apache.org/jira/browse/IMPALA-8290. I updated the commit message to reflect this. http://gerrit.cloudera.org:8080/#/c/14592/7/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@388 PS7, Line 388: : /** : * Returns a "CREATE TABLE" string that creates the table with the specified properties. : * The tableName must not be null. If columnsSql is null, the schema syntax will : * not be generated. : */ : public static String getCreateTableSql(String dbName, String tableName, : String tableComment, List<String> columnsSql, List<String> partitionColumnsSql, : List<String> primaryKeysSql, List<String> foreignKeysSql, : String kuduPartitionByParams, Pair<List<String>, TSortingOrder> sortProperties, : Map<String, String> tblProperties, Map<String, String> serdeParameters, : boolean isExternal, boolean ifNotExists, RowFormat rowFormat, : > Do we need a method for this? Can we use CollectionUtils.isNotEmpty() That should work. Did not know it existed. http://gerrit.cloudera.org:8080/#/c/14592/7/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@425 PS7, Line 425: Joiner.on(", ").appendTo(sb, primaryKeysSql).append(")"); > This condition is redundant since the inner if blocks already check for the Right. Thanks for catching that. 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: new PrimaryKeysRequest(msTbl.getDbName(), msTbl.getTableName()))); : // Get foreign keys info. : foreignKeys_.addAll(client.getForeignKeys(new ForeignKeysRequest(null,null, : msTbl.getDbName(),msTbl.getTableName()))); : } : loadValidWriteIdList(client); > Shouldn't this be under loadTableSchema flag? Moved them under loadTableSchema. For the RPC calls, wouldn't users always want to know this information? Do you mean we should have a flag and only if it is turned on users will be able to get constraints information? 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 coln Added the test. Thanks. -- 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: 8 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: Fri, 08 Nov 2019 01:25:34 +0000 Gerrit-HasComments: Yes
