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 1: (56 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 betw Yeah, the handling part will be in a follow up patch. http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@9 PS1, Line 9: CATS > CTAS - Create Table As Select? Done http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@9 PS1, Line 9: statments > statements Done 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? I updated the patch to be only about managed table, due to the in flight discussion. 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. Yes, I updated the patch to be only about managed table, due to the in flight discussion. http://gerrit.cloudera.org:8080/#/c/13318/1/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/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@255 PS1, Line 255: private void analyzeKuduTableProperties(Analyzer analyzer, boolean isHMSIntegrationEnabled) > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@270 PS1, Line 270: // TODO(hao): remove ALL privileges on SERVER validation when HMS Integration is enabled. > line too long (95 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@285 PS1, Line 285: (!isHMSIntegrationEnabled && !handler.equals(KuduTable.KUDU_LEGACY_STORAGE_HANDLER)))) { > line too long (97 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@304 PS1, Line 304: String.format("Table property %s should not be specified when creating an Kudu table.", > line too long (95 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@309 PS1, Line 309: * Populates Kudu master addresses either from table property or the -kudu_master_hosts flag. > line too long (96 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@322 PS1, Line 322: private void analyzeExternalKuduTableParams(Analyzer analyzer, boolean isHMSIntegrationEnabled) > line too long (97 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@353 PS1, Line 353: private void analyzeManagedKuduTableParams(Analyzer analyzer, boolean isHMSIntegrationEnabled) > line too long (96 > 90) Done 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? Yeah, I think here is to cover legacy tables with Kudu, to remove TABLE_NAME property with managed table. 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 Actually I think I will just use KUDU_STORAGE_HANDLER here for two reasons. 1) Going forward we are deprecating KUDU_LEGACY_STORAGE_HANDLER no matter HMS integration is enabled. 2) This is only used at ToSqlUtils https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java#L317, which is just to set to null if it is a KuduTable (https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java#L326). But please correct me if my understanding is wrong. http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@177 PS1, Line 177: public static boolean isHMSIntegrationEnabled(String kuduMasters) throws ImpalaRuntimeException { > line too long (99 > 90) Done 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 n Yes, I am planning to handle it a separate patch, added a TODO here. http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@242 PS1, Line 242: throw new TableLoadingException("Unable to get Kudu HMS integration configuration " + > line too long (93 > 90) Done 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 Yeah, good catch, even though I am not sure if we want to support cases when table is not upgraded but HMS integration is enabled. But I will keep the original logic which relies on TABLE_NAME no matter HMS integration is enabled or not, as the plan is to always use the new storage handler. 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@208 PS1, Line 208: throw new LocalCatalogException("Unable to get Kudu HMS integration configuration " + > line too long (93 > 90) Done 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? Perha I reverted the change as the reason documented in KuduTable.load. 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? This is going to be changed in follow-up patches. Right now it is purely a method renaming. 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 fo Done 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 Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@182 PS1, Line 182: AnalyzesOk(String.format("create table tab (x int primary key) partition by hash (x) " + > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@190 PS1, Line 190: AnalyzesOk(String.format("create table tdata_no_port (id int primary key, name string, " + > line too long (94 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@201 PS1, Line 201: AnalyzesOk(String.format("create table tab (x int primary key) stored as kudu tblproperties (" + > line too long (100 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@423 PS1, Line 423: String.format("Table property %s should not be specified when creating an Kudu table.", > line too long (95 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@437 PS1, Line 437: AnalysisError("create external table t stored as kudu tblproperties('kudu.table_name'='t')", > line too long (98 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@438 PS1, Line 438: String.format("Table property %s should not be specified when creating an external " + > line too long (96 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@452 PS1, Line 452: "%s 'kudu.master_addresses' = '%s')", tableName.isEmpty() ? tableName : tableName + ",", > line too long (96 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@465 PS1, Line 465: AnalysisError(String.format("create external table t (x int primary key) stored as kudu %s", > line too long (96 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@474 PS1, Line 474: "%s 'storage_handler'='foo')", tableName.isEmpty() ? tableName : tableName + ","), > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@483 PS1, Line 483: AnalysisError(String.format("create external table t stored as kudu cached in 'testPool' %s", > line too long (97 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@487 PS1, Line 487: AnalysisError(String.format("create external table t stored as kudu location '/warehouse' " + > line too long (97 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@492 PS1, Line 492: "%s 'kudu.table_id'='123456')", tableName.isEmpty() ? tableName : tableName + ","), > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@493 PS1, Line 493: String.format("Table property %s should not be specified when creating an Kudu table.", > line too long (95 > 90) Done 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 ad Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@24 PS1, Line 24: new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "SELECT"))); > line too long (94 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@31 PS1, Line 31: new TAccessEvent("functional_kudu.alltypes", TCatalogObjectType.TABLE, "SELECT"), > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@32 PS1, Line 32: new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "INSERT"))); > line too long (94 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@39 PS1, Line 39: new TAccessEvent("functional_kudu.alltypes", TCatalogObjectType.TABLE, "SELECT"), > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@40 PS1, Line 40: new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "ALL"))); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@46 PS1, Line 46: new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "SELECT"), > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@47 PS1, Line 47: new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "ALL"))); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@54 PS1, Line 54: new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "SELECT"), > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@55 PS1, Line 55: new TAccessEvent("functional_kudu.alltypes", TCatalogObjectType.TABLE, "SELECT"), > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@56 PS1, Line 56: new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "ALL"))); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@62 PS1, Line 62: new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "SELECT"), > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@63 PS1, Line 63: new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "ALL"))); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@78 PS1, Line 78: new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "ALTER"), > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@79 PS1, Line 79: new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "SELECT"))); > line too long (94 > 90) Done 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? This is a test suite when HMS integration is enabled, currently all tests in AnalyzeKuduDDLTest, AuditingKuduTest, ParserTest, ToSqlTest will be run. http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@337 PS1, Line 337: testToSql(String.format("create table p (a bigint primary key, b timestamp default '1987-05-19') " + > line too long (104 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@371 PS1, Line 371: testToSql(String.format("create table p primary key (a,b) partition by hash(a) partitions 3, " + > line too long (100 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@376 PS1, Line 376: String.format("CREATE TABLE default.p PRIMARY KEY (a, b) PARTITION BY HASH (a) PARTITIONS 3, " + > line too long (105 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@379 PS1, Line 379: "SELECT int_col a, bigint_col b FROM functional.alltypes", kuduMasters, storageHandler), true); > line too long (103 > 90) 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: 1 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: Fri, 17 May 2019 05:56:08 +0000 Gerrit-HasComments: Yes
