Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration ...................................................................... Patch Set 1: (15 comments) http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration I was under the impression there would need to be special coordination between calls to create the table in HMS and Kudu based on the design doc. In the legacy world, Impala creates an HMS entry and a Kudu table. Now, I think we just create the Kudu table and that auto-populates the HMS. But I didn't see the calls to the HMS removed in this patch. For legacy tables, calls to HMS likely still need to occur. Though those are not likely create table calls, but alter and drop calls. http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@9 PS1, Line 9: CATS CTAS - Create Table As Select? http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@9 PS1, Line 9: statments statements http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@13 PS1, Line 13: CREATE EXTERNAL TABLE t STORED AS KUDU TBLPROPERTIES How is this different then it was before? Why does it need to be different? http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@21 PS1, Line 21: 2) When Kudu/HMS integration is enabled, the external table is no longer I think this discussion is still in flight with Kudu devs. http://gerrit.cloudera.org:8080/#/c/13318/1/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/13318/1/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@329 PS1, Line 329: // When Kudu/HMS integration is not enabled, remove hidden table property What about legacy tables? http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@130 PS1, Line 130: public String getStorageHandlerClassName() { Should this return the handler from the HMS instead of generating one based on HMS integration being enabled? I suspect there are two cases, the first is when creating a table, and the second is when using an already created table. When creating a table I suspect KUDU_STORAGE_HANDLER would always be used. When using an already created table, it could be KUDU_LEGACY_STORAGE_HANDLER, even if the HMS integration was turned on. http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@183 PS1, Line 183: hmsConfig = kuduClient.getHiveMetastoreConfig(); Do we care if Kudu is configured to use a different HMS than Impala? They need to be the same right? http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@245 PS1, Line 245: // When Kudu/HMS integration is enabled, generates the Kudu table name from Does it matter if it's a legacy table here? In that case it will be `impala::db_name.table_name` right? http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java: http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@212 PS1, Line 212: // When Kudu/HMS integration is enabled, generates the Kudu table name from Can this logic be shared with the table name logic in KuduTable.load? Perhaps factored next to the table name function in KuduUtil. http://gerrit.cloudera.org:8080/#/c/13318/1/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/13318/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2578 PS1, Line 2578: String newKuduTableName = KuduUtil.getLegacyDefaultKuduTableName( What about when HMS integration is enabled? http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/util/KuduUtil.java File fe/src/main/java/org/apache/impala/util/KuduUtil.java: http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/util/KuduUtil.java@397 PS1, Line 397: public static String getKuduTableName(String metastoreDbName, Maybe this should take a boolean for HMS enabled, and return the correct format based on that boolean. http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java: PS1: Because this doesn't diff with AnalyzeKuduDDLTest, can you comment on what you added/changed? http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java File fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java: PS1: Because this doesn't diff with AuditingTest, can you comment on what you added/changed? http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java File fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java: http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java@32 PS1, Line 32: public class KuduHMSIntegrationTest { Where is this used? Do tests need to be added yet? -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Wed, 15 May 2019 16:00:58 +0000 Gerrit-HasComments: Yes
