Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14750 )
Change subject: IMPALA-9092: Add support for creating external Kudu table ...................................................................... Patch Set 4: (4 comments) Overall looks good to me. Thanks Vihang for fixing this! http://gerrit.cloudera.org:8080/#/c/14750/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/14750/4/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@383 PS4, Line 383: if (Boolean.parseBoolean( : getTblProperties().get(KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE))) { : throw new AnalysisException(String.format("Table property '%s' cannot be set to " + : "true with an external Kudu table.", KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE)); : } This can be removed as 'external.table.purge' = true has been checked at L290? http://gerrit.cloudera.org:8080/#/c/14750/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/14750/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2211 PS4, Line 2211: except we skip Kudu table creation if : * it already exists From the code in KuduCatalogOpExecutor, it looks like we skip Kudu table creation (throw error) for both managed and external purge table? http://gerrit.cloudera.org:8080/#/c/14750/4/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/14750/4/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@92 PS4, Line 92: if table is externalPurgeTable nit: if table is managed or externalPurgeTable? http://gerrit.cloudera.org:8080/#/c/14750/4/fe/src/main/java/org/apache/impala/service/MetadataOp.java File fe/src/main/java/org/apache/impala/service/MetadataOp.java: http://gerrit.cloudera.org:8080/#/c/14750/4/fe/src/main/java/org/apache/impala/service/MetadataOp.java@21 PS4, Line 21: import java.util.Collections; nit: why is the added while no other change in this file? -- To view, visit http://gerrit.cloudera.org:8080/14750 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I76f81d41db0cf2269ee1b365857164a43677e14d Gerrit-Change-Number: 14750 Gerrit-PatchSet: 4 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Thu, 21 Nov 2019 22:32:14 +0000 Gerrit-HasComments: Yes
