Andrew Wong 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 3: (5 comments) 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" > No worries, this is used in the getCreateTableSql to display the SQL which Ah, I see. Thanks for explaining. The missing piece for me was that this gets set by Hive over here: https://github.com/apache/hive/blob/a2b10a4f6cd750810a6451e17e4394ed6f8dec0a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetastoreDefaultTransformer.java#L578 http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@98 PS1, Line 98: econditions.checkState(!Strings.isNullOrEmpty(kuduTableName)); : Schema schema = createTableSchema(params); : CreateTableOptions tableOpts = buildTableOptions(msTbl, params, sc > You make a good point. So far, I was always assuming that we should only al Yeah, the new code looks good to me, keeping "managed" and "external+purge" consistent. I agree, and good point about sharing tables. Framed that way, we definitely _shouldn't_ create an external purge table if it already exists in Kudu. My mental model assumes that a Kudu table is synchronized by at most one entity (whether that's Kudu with the HMS integration, or a single Impala service). And so not creating an HMS entry is more conservative in that regard. http://gerrit.cloudera.org:8080/#/c/14750/3/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/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@43 PS3, Line 43: isSynchronizedTbl nit: managed and external purge tables are both synchronized. Maybe rename to isExternalPurgeTbl or something similar? http://gerrit.cloudera.org:8080/#/c/14750/3/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java File fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java: http://gerrit.cloudera.org:8080/#/c/14750/3/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java@261 PS3, Line 261: areKuduTablesSynchronized nit: the Hive versioning is a bit orthogonal to the table being synchronized, since both of the below tables are synchronized. Maybe rename this to 'areDefaultSynchronizedTablesExternal' or somesuch? http://gerrit.cloudera.org:8080/#/c/14750/3/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/14750/3/tests/query_test/test_kudu.py@1341 PS3, Line 1341: # TODO should we block this? Good question. I don't see the harm in keeping it around, as long as Hive3 correctly updates it. -- 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: 3 Gerrit-Owner: Vihang Karajgaonkar <vih...@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: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Thu, 21 Nov 2019 03:02:51 +0000 Gerrit-HasComments: Yes
