Hao Hao 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 4: (17 comments) http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@9 PS3, Line 9: commit > commit Done http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@9 PS3, Line 9: dds sup > adds support for Done http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@11 PS3, Line 11: l addr > actual Done http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@13 PS3, Line 13: > a managed Done http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@19 PS3, Line 19: ion is enabled, the Kudu table underneath the : managed HMS table will follow > BTW, what happens with the name of the table created in Kudu when the integ You mean a Kudu table is created with HMS integration enabled and later the integration is disabled? Yeah, that should be handled by the tool. http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@23 PS3, Line 23: > nit: Kudu-related Done http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@23 PS3, Line 23: > commit Done http://gerrit.cloudera.org:8080/#/c/13318/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/13318/3/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@329 PS3, Line 329: u t > nit: drop? Done http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@329 PS3, Line 329: a manag > nit: a managed Kudu Done http://gerrit.cloudera.org:8080/#/c/13318/3/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/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@192 PS3, Line 192: public static boolean isHMSIntegrationEnabledAndValidate(String kuduMasters, > Where is this used? This will be used when the DDL actual handling. And I don't see there is a need to expose HiveMetastoreConfig so far. As its content is only used for validation HMS Uris so far. http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@194 PS3, Line 194: HiveMetastoreConfig hmsConfig = getHiveMetastoreConfig(kuduMasters); > hmsConfig could be null here. Done http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java: http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@440 PS3, Line 440: .KuduStorageHa > Is it possible to create a Kudu table with STORED AS KUDU syntax and simult It is possible for appropriate storage handler, added such a test. For non appropriate is covered below. http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@485 PS3, Line 485: g.format("Tab > nit: a Kudu table (should be fixed in corresponding place) Done http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java File fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java: http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@1 PS3, Line 1: // Licensed to the Apache Software Foundation (ASF) under one > License header Done http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@22 PS3, Line 22: il.Set; > What about ALTER TABLE? If generating auditing events for ALTER is not sup This is extracted from AuditingTest class which has coverage for ALTER statements. My understanding is this is for Kudu specific/only DDLs/DMLs. http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@24 PS3, Line 24: import org.apache.impala.authorization.AuthorizationException; > What about CTAS statement? Do we expect both CREATE and SELECT audit event Yeah, added a test in AuditingTest. http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@81 PS3, Line 81: Assert.assertEquals(accessEvents, Sets.newHashSet( : new TAccessEvent("functional_kudu.testtbl", : TCatalogObjectType.TABLE, "SELECT"), : new TAccessEvent("functional_kudu.alltypes", > What about 'DROP TABLE ... IF EXISTS'? Done -- 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: 4 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Tue, 21 May 2019 19:29:02 +0000 Gerrit-HasComments: Yes
