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 3: (15 comments) http://gerrit.cloudera.org:8080/#/c/13318/2/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/2/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@266 PS2, Line 266: throw new AnalysisException("Invalid storage handler specified for Kudu table: " + > Use !KuduTable.isKuduStorageHandler(...) here. Done http://gerrit.cloudera.org:8080/#/c/13318/2/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/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@57 PS2, Line 57: // Alias to the string key that identifies the storage handler for Kudu tables. > This isn't used anywhere Done http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@167 PS2, Line 167: try { > How difficult would this be to implement and what's the failure mode if its I removed the TODO and updated the actual validation. http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@220 PS2, Line 220: KuduClient kuduClient = KuduUtil.getKuduClient(getKuduMasterHosts()); > I think this can be reverted to be the KEY_TABLE_NAME and the error message Done http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@227 PS2, Line 227: } > I don't think this comment is relevant anymore, this isn't generating a nam Done http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@229 PS2, Line 229: > See my comment about about always using kuduTableName_ from KEY_TABLE_NAME. Done http://gerrit.cloudera.org:8080/#/c/13318/2/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/2/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@a193 PS2, Line 193: > Why did this need to change? Reverted it back. http://gerrit.cloudera.org:8080/#/c/13318/2/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/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2579 PS2, Line 2579: newTableName.getDb(), newTableName.getTbl(), false); > Don't we need to check if HMS is enabled? Yes, this will be a follow up change. http://gerrit.cloudera.org:8080/#/c/13318/2/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/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@387 PS2, Line 387: me.metastoreTableName'. This : * should only b > What does this mean? I think this refers to external table which the Kudu table name is provided by the users. http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@390 PS2, Line 390: String metastoreDbName, : String metastoreTableName, boolean isHMSIntegrationEnabled) { : return isHMSIntegrationEnabled ? metastoreDbName + "." + metastoreTableNam > Please try to match the formatting in the rest of the file, here and elsewh Done http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@397 PS2, Line 397: * Check if the given name is the default Kudu table name for managed table > brief comment Done http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java File fe/src/test/java/org/apache/impala/analysis/AuditingTest.java: http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java@65 PS2, Line 65: "SELECT"), > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/13318/2/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/2/fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java@39 PS2, Line 39: > This makes me nervous. We don't currently have any tests that restart any m Thanks for the suggestion! http://gerrit.cloudera.org:8080/#/c/13318/2/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/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@24 PS2, Line 24: import org.apache.impala.common.AnalysisException; > nit: not needed Done http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java: http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@51 PS2, Line 51: import org.apache.impala.catalog.ScalarType; > nit: not needed 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: 3 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: Mon, 20 May 2019 06:28:36 +0000 Gerrit-HasComments: Yes
