Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13375 )
Change subject: IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration ...................................................................... Patch Set 3: (7 comments) did a quick first pass http://gerrit.cloudera.org:8080/#/c/13375/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13375/3//COMMIT_MSG@10 PS3, Line 10: with Kudu's integration with the Hive Metastore nit: when integration with Hive Metastore is enabled http://gerrit.cloudera.org:8080/#/c/13375/3//COMMIT_MSG@11 PS3, Line 11: after the table is created in the : Kudu, relies on Kudu to create the table in the HMS. nit: consider rewriting it to be easier to parse http://gerrit.cloudera.org:8080/#/c/13375/3/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/13375/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@117 PS3, Line 117: if (kuduTableName_ == null || kuduTableName_.isEmpty()) { : // When 'kudu.table_name' property is empty, it implies Kudu/HMS : // integration is enabled. : // TODO: remove this hack once Kudu support 'kudu.table_name' : // property with the new storage handler. Would it make sense to create a method/function that will hide these implementation details, so once the new storage handler is available, you will need to update it in one place only. http://gerrit.cloudera.org:8080/#/c/13375/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@260 PS3, Line 260: if (kuduTableName_ == null || kuduTableName_.isEmpty()) { : // When 'kudu.table_name' property is empty, it implies Kudu/HMS : // integration is enabled. : // TODO: remove this hack once Kudu support 'kudu.table_name' : // property with the new storage handler. ditto -- would it help to create a function/method and then re-use it in such cases. http://gerrit.cloudera.org:8080/#/c/13375/3/tests/common/skip.py File tests/common/skip.py: http://gerrit.cloudera.org:8080/#/c/13375/3/tests/common/skip.py@110 PS3, Line 110: hms_integration nit: maybe rename into hms_integration_enabled ? http://gerrit.cloudera.org:8080/#/c/13375/3/tests/common/skip.py@112 PS3, Line 112: to be set in Kudu to not be set ? Maybe, replace the whole 'reason' string with "Test assumes Kudu+HMS integration is not enabled." http://gerrit.cloudera.org:8080/#/c/13375/3/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/13375/3/tests/custom_cluster/test_kudu.py@128 PS3, Line 128: self.run_test_case('QueryTest/kudu_create', vector, use_db=unique_database) > What scenarios is this testing? Hard to tell without being more familiar wi +1 -- To view, visit http://gerrit.cloudera.org:8080/13375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icffe412395f47f5e07d97bad457020770cfa7502 Gerrit-Change-Number: 13375 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao <hao....@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-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Tue, 28 May 2019 18:22:23 +0000 Gerrit-HasComments: Yes
