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 2: (7 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: !handler.equals(KuduTable.KUDU_STORAGE_HANDLER) && Use !KuduTable.isKuduStorageHandler(...) here. 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@220 PS2, Line 220: String fullTableName = msTbl.getDbName() + "." + msTbl.getTableName(); I think this can be reverted to be the KEY_TABLE_NAME and the error messages below can say > "... property found for Kudu table" + kuduTableName_ http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@227 PS2, Line 227: // When Kudu/HMS integration is enabled, generates the Kudu table name from I don't think this comment is relevant anymore, this isn't generating a name. http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@229 PS2, Line 229: kuduTableName_ = msTable_.getParameters().get(KuduTable.KEY_TABLE_NAME); See my comment about about always using kuduTableName_ from KEY_TABLE_NAME. 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? 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? 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: This assumes a custom name was : * not provided. What does this mean? -- 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: 2 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: Hao Hao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Fri, 17 May 2019 13:22:19 +0000 Gerrit-HasComments: Yes
