Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14397 )
Change subject: IMPALA-9030: Handle translated external Kudu tables ...................................................................... Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/14397/6/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/14397/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2185 PS6, Line 2185: isExternalTable > It should be updated to check for not (synchronized) tables. I think we should continue checking only external table here because for external kudu tables, Impala expects the table to exist in Kudu and it populates the schema from Kudu table in the newTable object. I think the behavior if the user issues a create external table foo (id int) tblproperties ('external.purge.table'='true') is unclear. if table is defined as external, impala will expect the table to exist in Kudu and hence will read the schema from that table which is what the code is doing currently and the patch doesn't really change that. http://gerrit.cloudera.org:8080/#/c/14397/6/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/14397/6/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@75 PS6, Line 75: !KuduTable.isExternalTable(msTbl) > +1. I think checks in CatalogOpExecutor::createKuduTable() also need to be So this is interesting. Note that this method is implemented to create managed tables (it will go and create a kudu table for you) so we want to allow only the such tables to go further to line 76. if we are checking for !KuduTable.isSynchronized(msTbl) what we are essentially checking is that msTbl is !(managed || (external && external.purge.table=true)) which translates to !managed && !(external && external.purge.table=true) which translates to !managed && !external || external.purge.table=false/unset which translates to external.purge.table=false/unset so all the statements below will be good: create table foo (id int); create external table foo (id int); create external table foo (id int) tblproperties ('external.purge.table' = 'false'); We don't want this to happen since for any external table we expect the Kudu table to exist and hence the method will fail at line 88. We should really be just checking for !external tables since the code below goes ahead and creates Kudu table. The open question here is what should happen if the user does this: create table foo(id int) tblproperties ('external.purge.table'='true'); This in my opinion is an incorrect usage of the tbl property and we should disallow it. But Hive allows it and hence Impala has to allow it too for compatiblity. What will happen with this SQL is it will be sent to Kudu which will create a table and HMS will eventually create a external table foo with 'external.purge.table'='true' after translation so the drop/rename will work just as same as a managed table as expected. Basically the tbl property provided by user is unnecessary and does not impact anything as expected. I am not sure if this helps or makes it more confusing :) Let me know if see any problems with my reasoning above. Once we agree I will add comment there to explain the same or do necessary changes. -- To view, visit http://gerrit.cloudera.org:8080/14397 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I324523361c923b7d291cb4d0f1028b1a5b653b36 Gerrit-Change-Number: 14397 Gerrit-PatchSet: 6 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Thu, 17 Oct 2019 23:14:40 +0000 Gerrit-HasComments: Yes
