Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13358 )
Change subject: IMPALA-8504: Introduce the new Kudu storage handler ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/13358/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13358/1//COMMIT_MSG@9 PS1, Line 9: commit > commit Done http://gerrit.cloudera.org:8080/#/c/13358/1//COMMIT_MSG@13 PS1, Line 13: st > drop Done http://gerrit.cloudera.org:8080/#/c/13358/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/13358/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@222 PS1, Line 222: if (KuduTable.KUDU_LEGACY_STORAGE_HANDLER.equals( > Should this check if it's either format? Or just the new format since the o This is intentionally to only check the legacy format as this commit is only introducing the new storage handler. And there are a series of follow-up patches will actual use this storage handler. The motivation is to avoid giant patch to review and also hopefully to unblock other's testing work which rely on use new storage handler to identify Kudu tables. http://gerrit.cloudera.org:8080/#/c/13358/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@222 PS1, Line 222: if (KuduTable.KUDU_LEGACY_STORAGE_HANDLER.equals( > I'm also curious why new storage handler is not accounted for in this logic Yeah, this is intentional, as explained above. I will add more detailed explanation in the commit msg. http://gerrit.cloudera.org:8080/#/c/13358/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@265 PS1, Line 265: if (handler != null && !handler.equals(KuduTable.KUDU_LEGACY_STORAGE_HANDLER)) { > Same here. Same as above comment. http://gerrit.cloudera.org:8080/#/c/13358/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@270 PS1, Line 270: KuduTable.KUDU_LEGACY_STORAGE_HANDLER); > Should this be the new format since the old format won't be used to create Same as above. http://gerrit.cloudera.org:8080/#/c/13358/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/13358/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@74 PS1, Line 74: // KEY_STORAGE_HANDLER. This is expected to be deprecated eventually. > Is this comment still true? I think we will use KUDU_STORAGE_HANDLER for al Good catch, you're right. http://gerrit.cloudera.org:8080/#/c/13358/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@79 PS1, Line 79: // KEY_STORAGE_HANDLER. > We also don't need to mention HMS integration here. Done -- To view, visit http://gerrit.cloudera.org:8080/13358 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75bcd5246005f4e35251aef9219f4d07eeb87dc6 Gerrit-Change-Number: 13358 Gerrit-PatchSet: 2 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-Comment-Date: Fri, 17 May 2019 23:28:29 +0000 Gerrit-HasComments: Yes
