Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14750 )
Change subject: IMPALA-9092: Add support for creating external Kudu table ...................................................................... Patch Set 10: Code-Review+2 (7 comments) LGTM. Thanks for addressing on the comments! Please carry on the +2 after fixing some nits. http://gerrit.cloudera.org:8080/#/c/14750/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/14750/1/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@75 PS1, Line 75: "TRANSLATED_TO_EXTERNAL" > As per my understanding, this property is internal to HMS which is set when I don't think it'll be misleading to users since we already show TRANSLATED_TO_EXTERNAL in "DESCRIBE FORMATTED" statement along with "external.table.purge" that also set by Hive transformer. However, I'm not sure whether users will have interests to classify tables based on it (this looks like the only usage for this property currently...). So I'm ok to not block the patch for this :) http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/main/java/org/apache/impala/catalog/Table.java@199 PS6, Line 199: Returns if the given HMS table is an external table (uses table type if : * available o > sorry for the typos. Let me know if the updated one is clearer. It's clear for me now. Thanks! http://gerrit.cloudera.org:8080/#/c/14750/10/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/14750/10/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@103 PS10, Line 103: nit: looks like indentions are still inconsistent in some lines like this http://gerrit.cloudera.org:8080/#/c/14750/10/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@148 PS10, Line 148: "Range partition values cannot be NULL. Range partition: 'PARTITION " + nit: and this http://gerrit.cloudera.org:8080/#/c/14750/10/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@177 PS10, Line 177: "Number of specified range partition values is different than the number of " + nit: and this http://gerrit.cloudera.org:8080/#/c/14750/10/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@209 PS10, Line 209: "Column 'b' in 'RANGE (b) (PARTITION VALUE = 'abc')' is not a key column. " + nit: and this http://gerrit.cloudera.org:8080/#/c/14750/10/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@278 PS10, Line 278: , nit: leading comma -- To view, visit http://gerrit.cloudera.org:8080/14750 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I76f81d41db0cf2269ee1b365857164a43677e14d Gerrit-Change-Number: 14750 Gerrit-PatchSet: 10 Gerrit-Owner: Vihang Karajgaonkar <[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: Joe McDonnell <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Fri, 13 Dec 2019 02:31:02 +0000 Gerrit-HasComments: Yes
