[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commit adds support for the syntax of CREATE TABLE (and CTAS) statements for managed Kudu tables with Kudu/HMS integration. A follow up patch will address the actual handling of CREATE TABLE statement with Kudu/HMS integration. For a managed table the syntax remains the same. However, the detailed changes includes: 1) Kudu table will always be created with the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' even when Kudu/HMS integration is disabled. The legacy storage handler will be eventually deprecated. 2) When Kudu/HMS integration is enabled, the Kudu table underneath the managed HMS table will follow the naming convention 'db_name.table_name' instead of 'impala::db_name.table_name'. 3) Add 'kudu.table_id' table property to be used with Kudu/HMS integration. This commit also extracts Kudu-related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Reviewed-on: http://gerrit.cloudera.org:8080/13318 Tested-by: Impala Public Jenkins Reviewed-by: Thomas Marshall --- M bin/run-all-tests.sh M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java A fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java A fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java M testdata/cluster/node_templates/common/etc/init.d/kudu-master M tests/query_test/test_kudu.py 16 files changed, 920 insertions(+), 577 deletions(-) Approvals: Impala Public Jenkins: Verified Thomas Marshall: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 14 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 13: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 13 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 31 May 2019 23:33:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 13: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 13 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 31 May 2019 23:24:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 13: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4367/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 13 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 31 May 2019 17:56:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 12: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3456/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 31 May 2019 01:52:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hello Thomas Marshall, Mike Percy, Alexey Serbin, Andrew Wong, Adar Dembo, Grant Henke, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13318 to look at the new patch set (#12). Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commit adds support for the syntax of CREATE TABLE (and CTAS) statements for managed Kudu tables with Kudu/HMS integration. A follow up patch will address the actual handling of CREATE TABLE statement with Kudu/HMS integration. For a managed table the syntax remains the same. However, the detailed changes includes: 1) Kudu table will always be created with the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' even when Kudu/HMS integration is disabled. The legacy storage handler will be eventually deprecated. 2) When Kudu/HMS integration is enabled, the Kudu table underneath the managed HMS table will follow the naming convention 'db_name.table_name' instead of 'impala::db_name.table_name'. 3) Add 'kudu.table_id' table property to be used with Kudu/HMS integration. This commit also extracts Kudu-related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c --- M bin/run-all-tests.sh M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java A fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java A fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java M testdata/cluster/node_templates/common/etc/init.d/kudu-master M tests/query_test/test_kudu.py 16 files changed, 920 insertions(+), 577 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13318/12 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 11: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4350/ -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 30 May 2019 22:51:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 11: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4350/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 30 May 2019 17:11:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3435/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 30 May 2019 07:11:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hello Thomas Marshall, Mike Percy, Alexey Serbin, Andrew Wong, Adar Dembo, Grant Henke, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13318 to look at the new patch set (#10). Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commit adds support for the syntax of CREATE TABLE (and CTAS) statements for managed Kudu tables with Kudu/HMS integration. A follow up patch will address the actual handling of CREATE TABLE statement with Kudu/HMS integration. For a managed table the syntax remains the same. However, the detailed changes includes: 1) Kudu table will always be created with the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' even when Kudu/HMS integration is disabled. The legacy storage handler will be eventually deprecated. 2) When Kudu/HMS integration is enabled, the Kudu table underneath the managed HMS table will follow the naming convention 'db_name.table_name' instead of 'impala::db_name.table_name'. 3) Add 'kudu.table_id' table property to be used with Kudu/HMS integration. This commit also extracts Kudu-related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c --- M bin/run-all-tests.sh M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java A fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java A fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java M testdata/cluster/node_templates/common/etc/init.d/kudu-master 15 files changed, 919 insertions(+), 576 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13318/10 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 8: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 30 May 2019 02:37:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 29 May 2019 22:43:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 7:
(5 comments)
Looks good, just a couple more nitpicks I think before this can go in
http://gerrit.cloudera.org:8080/#/c/13318/7/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/13318/7/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@171
PS7, Line 171: Error getting the configuration on whether
I find this awkwardly worded, maybe 'Error determining if'
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@173
PS7, Line 173: Kudu error
unnecessary
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java@56
PS7, Line 56: new TAccessEvent("functional.alltypes",
TCatalogObjectType.TABLE, "SELECT")));
Probably better to leave off unrelated formatting changes like this. It makes
it more difficult to deal with stuff like backports
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java
File fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java:
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java@49
PS7, Line 49: public static int RestartMiniclusterComponent(String component,
This should be removed
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java
File fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java:
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java@32
PS7, Line 32: mini cluster
minicluster component
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c
Gerrit-Change-Number: 13318
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Wed, 29 May 2019 19:37:24 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3427/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 29 May 2019 21:44:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 8:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13318/7/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/13318/7/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@171
PS7, Line 171: Error determining if Kudu's integration wi
> I find this awkwardly worded, maybe 'Error determining if'
Done
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@173
PS7, Line 173:
> unnecessary
Done
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java@56
PS7, Line 56: new TAccessEvent("functional.alltypes",
TCatalogObjectType.TABLE, "SELECT")
> Probably better to leave off unrelated formatting changes like this. It mak
Good point, though I have to change the lines below due to they exceeds the
line length limit(90).
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java@212
PS7, Line 212: Assert.assertEquals(accessEvents, Sets.newHashSet(
> Why this change?
Good catch, accidentally changed this.
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java
File fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java:
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java@49
PS7, Line 49:
> This should be removed
Done
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java
File fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java:
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java@32
PS7, Line 32: minicluster
> minicluster component
Done
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c
Gerrit-Change-Number: 13318
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Wed, 29 May 2019 21:04:13 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hello Thomas Marshall, Mike Percy, Alexey Serbin, Andrew Wong, Adar Dembo, Grant Henke, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13318 to look at the new patch set (#8). Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commit adds support for the syntax of CREATE TABLE (and CTAS) statements for managed Kudu tables with Kudu/HMS integration. A follow up patch will address the actual handling of CREATE TABLE statement with Kudu/HMS integration. For a managed table the syntax remains the same. However, the detailed changes includes: 1) Kudu table will always be created with the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' even when Kudu/HMS integration is disabled. The legacy storage handler will be eventually deprecated. 2) When Kudu/HMS integration is enabled, the Kudu table underneath the managed HMS table will follow the naming convention 'db_name.table_name' instead of 'impala::db_name.table_name'. 3) Add 'kudu.table_id' table property to be used with Kudu/HMS integration. This commit also extracts Kudu-related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c --- M bin/run-all-tests.sh M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java A fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java A fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java M testdata/cluster/node_templates/common/etc/init.d/kudu-master 15 files changed, 919 insertions(+), 576 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13318/8 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java@212
PS7, Line 212: new TAccessEvent("tpch.new_view",
TCatalogObjectType.TABLE, "CREATE")));
Why this change?
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c
Gerrit-Change-Number: 13318
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Wed, 29 May 2019 14:23:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 7: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/13318/4/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/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@192 PS4, Line 192: // TODO: Change this to reflect the number of pk columns and modify all the > LGTM, will pull this change out to the follow up patch where it is used. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 29 May 2019 01:50:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3417/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 29 May 2019 00:46:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13318/4/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/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@192
PS4, Line 192: // TODO: Change this to reflect the number of pk columns and
modify all the
> I see. I'm fine with keeping this in if it's a hassle for you to rebase, bu
LGTM, will pull this change out to the follow up patch where it is used.
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@200
PS4, Line 200: tch (KuduException e) {
> SGTM. Will that be in the follow-up patch where this function is used or a
It will be in the follow-up where it is used.
http://gerrit.cloudera.org:8080/#/c/13318/6/fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java
File
fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/6/fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java@38
PS6, Line 38: AnalyzeKuduDDLTest.class, AuditingKuduTest.class,
: ParserTest.class, ToSqlTest.class
> Do these also get run without the HMS integration enabled?
Yes, it does.
http://gerrit.cloudera.org:8080/#/c/13318/6/fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java@60
PS6, Line 60: List envp = new ArrayList<>();
> nit: definition could be gated by enableHMSIntegration if this were put at
Done
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c
Gerrit-Change-Number: 13318
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Wed, 29 May 2019 00:07:44 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hello Thomas Marshall, Mike Percy, Alexey Serbin, Andrew Wong, Adar Dembo, Grant Henke, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13318 to look at the new patch set (#7). Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commit adds support for the syntax of CREATE TABLE (and CTAS) statements for managed Kudu tables with Kudu/HMS integration. A follow up patch will address the actual handling of CREATE TABLE statement with Kudu/HMS integration. For a managed table the syntax remains the same. However, the detailed changes includes: 1) Kudu table will always be created with the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' even when Kudu/HMS integration is disabled. The legacy storage handler will be eventually deprecated. 2) When Kudu/HMS integration is enabled, the Kudu table underneath the managed HMS table will follow the naming convention 'db_name.table_name' instead of 'impala::db_name.table_name'. 3) Add 'kudu.table_id' table property to be used with Kudu/HMS integration. This commit also extracts Kudu-related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c --- M bin/run-all-tests.sh M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java A fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java A fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java M testdata/cluster/node_templates/common/etc/init.d/kudu-master 16 files changed, 935 insertions(+), 580 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13318/7 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@19 PS3, Line 19: ion is enabled, the Kudu table underneath the :managed HMS table will follow > You mean a Kudu table is created with HMS integration enabled and later the I see, that makes sense. Thanks for clarifying! -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 28 May 2019 18:21:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3402/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 28 May 2019 04:53:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13318/4/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/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@192
PS4, Line 192: public static boolean
isHMSIntegrationEnabledAndValidate(String kuduMasters,
> No, it is used in the follow up patch.
I see. I'm fine with keeping this in if it's a hassle for you to rebase, but it
would have been nice to separate it out early since 1) as you note, this patch
is already quite large, and 2) this function not being used also implies there
isn't testing for it, and 3) without seeing how it's used, in reviewing, it's
hard to determine whether it looks right.
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@200
PS4, Line 200: hmsUris.equals(kuduHMSUris)
> Yeah, I think it will be nice to check that, though I would prefer to add a
SGTM. Will that be in the follow-up patch where this function is used or a Jira?
http://gerrit.cloudera.org:8080/#/c/13318/6/fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java
File
fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/6/fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java@38
PS6, Line 38: AnalyzeKuduDDLTest.class, AuditingKuduTest.class,
: ParserTest.class, ToSqlTest.class
Do these also get run without the HMS integration enabled?
http://gerrit.cloudera.org:8080/#/c/13318/6/fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java@60
PS6, Line 60: final String hmsIntegrationEnv =
String.format("IMPALA_KUDU_STARTUP_FLAGS=" +
nit: definition could be gated by enableHMSIntegration if this were put at L66
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c
Gerrit-Change-Number: 13318
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Tue, 28 May 2019 04:52:01 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 4:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/13318/4/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/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@171
PS4, Line 171: String.format("Error getting the configuration on
whether Kudu's " +
> Are these error scenarios tested?
Added a test case in AnalyzeKuduDDLTest.
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@188
PS4, Line 188: Check with Kudu master to see if Kudu/HMS integration is
enabled. As well
:* as validate if Kudu is configured to the given Hive
Metastore that Impala
:* is configured to use.
> nit: reword slightly "Check with the Kudu master to see if its Kudu-HMS int
Done
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@192
PS4, Line 192: public static boolean
isHMSIntegrationEnabledAndValidate(String kuduMasters,
> Is this used in this patch?
No, it is used in the follow up patch.
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@200
PS4, Line 200: hmsUris.equals(kuduHMSUris)
> Do you think it's important to check the set equality of addresses vs just
Yeah, I think it will be nice to check that, though I would prefer to add a
todo and do it in a follow up patch, to avoid putting too much stuff in this
commit.
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@204
PS4, Line 204: When Kudu's integration with the Hive Metastore is enabled
> nit: this reads as a general statement, rather than a statement of what exi
Done
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@251
PS4, Line 251: (k
> nit: extra parentheses
Done
http://gerrit.cloudera.org:8080/#/c/13318/4/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/13318/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2579
PS4, Line 2579: false
> nit: not sure if this is standard practice in the Impala codebase, but migh
Done
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2579
PS4, Line 2579: false
> We do that sometimes, its definitely appropriate.
Done
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java:
PS4:
> What has changed from AnalyzeDDLTest? It's a bit hard to determine since th
This is only to extract Kudu table related test. Nothing has changed except I
added a new test about 'Kudu table is not allowed to set table property
'kudu.table_id' and another negative test for getting HMS integration
configuration as you requested. I extract it to a new file because it doesn't
make sense to run non-kudu table related test twice with/without HMS
integration.
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java:
PS4:
> Same here: what's changed with AuditingTest here that warrants a new file?
This is only about Kudu tables and nothing has changed here. Just to run the
test with/without HMS integration.
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@333
PS4, Line 333: String kuduMasters = catalog_.getDefaultKuduMasterHosts();
> Just making sure I understand this: the only necessary change here was the
Yes, I also update the Kudu master host address to be a valid one to not break
the test.
http://gerrit.cloudera.org:8080/#/c/13318/5/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java
File fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java:
http://gerrit.cloudera.org:8080/#/c/13318/5/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java@29
PS5, Line 29: public
> Leaving off this 'public' was intentional to reduce the chances someone wil
Sure, done.
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hello Thomas Marshall, Mike Percy, Alexey Serbin, Andrew Wong, Adar Dembo, Grant Henke, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13318 to look at the new patch set (#6). Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commit adds support for the syntax of CREATE TABLE (and CTAS) statements for managed Kudu tables with Kudu/HMS integration. A follow up patch will address the actual handling of CREATE TABLE statement with Kudu/HMS integration. For a managed table the syntax remains the same. However, the detailed changes includes: 1) Kudu table will always be created with the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' even when Kudu/HMS integration is disabled. The legacy storage handler will be eventually deprecated. 2) When Kudu/HMS integration is enabled, the Kudu table underneath the managed HMS table will follow the naming convention 'db_name.table_name' instead of 'impala::db_name.table_name'. 3) Add 'kudu.table_id' table property to be used with Kudu/HMS integration. This commit also extracts Kudu-related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c --- M bin/run-all-tests.sh M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java A fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java A fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java M testdata/cluster/node_templates/common/etc/init.d/kudu-master 16 files changed, 958 insertions(+), 580 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13318/6 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/13318/4/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/13318/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2579 PS4, Line 2579: false > nit: not sure if this is standard practice in the Impala codebase, but migh We do that sometimes, its definitely appropriate. Also, I know you already have a patch out to fix this, but could you leave a TODO saying that we need to properly handle HMS integration here? Its good practice to avoid forgetting things, and it lets the reviewers know that this was intentional. http://gerrit.cloudera.org:8080/#/c/13318/5/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java File fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java: http://gerrit.cloudera.org:8080/#/c/13318/5/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java@29 PS5, Line 29: public Leaving off this 'public' was intentional to reduce the chances someone will try to use this outside this package (which would break the filtering on package name in run-all-tests.sh). Could you instead just move the function you added to an equivalent package-private utility class in the 'customservice' package? -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 24 May 2019 21:51:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3348/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 23 May 2019 05:52:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hello Thomas Marshall, Mike Percy, Alexey Serbin, Andrew Wong, Adar Dembo, Grant Henke, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13318 to look at the new patch set (#5). Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commit adds support for the syntax of CREATE TABLE (and CTAS) statements for managed Kudu tables with Kudu/HMS integration. A follow up patch will address the actual handling of CREATE TABLE statement with Kudu/HMS integration. For a managed table the syntax remains the same. However, the detailed changes includes: 1) Kudu table will always be created with the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' even when Kudu/HMS integration is disabled. The legacy storage handler will be eventually deprecated. 2) When Kudu/HMS integration is enabled, the Kudu table underneath the managed HMS table will follow the naming convention 'db_name.table_name' instead of 'impala::db_name.table_name'. 3) Add 'kudu.table_id' table property to be used with Kudu/HMS integration. This commit also extracts Kudu-related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c --- M bin/run-all-tests.sh M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java A fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java M testdata/cluster/node_templates/common/etc/init.d/kudu-master 15 files changed, 906 insertions(+), 581 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13318/5 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 4:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/13318/4/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/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@171
PS4, Line 171: String.format("Error getting the configuration on
whether Kudu's " +
Are these error scenarios tested?
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@188
PS4, Line 188: Check with Kudu master to see if Kudu/HMS integration is
enabled. As well
:* as validate if Kudu is configured to the given Hive
Metastore that Impala
:* is configured to use.
nit: reword slightly "Check with the Kudu master to see if its Kudu-HMS
integration is enabled; if so, validate that it is integrated with the same
Hive Metastore that Impala is configured to use."
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@192
PS4, Line 192: public static boolean
isHMSIntegrationEnabledAndValidate(String kuduMasters,
Is this used in this patch?
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@200
PS4, Line 200: hmsUris.equals(kuduHMSUris)
Do you think it's important to check the set equality of addresses vs just the
strings? E.g. if order changes or something?
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@204
PS4, Line 204: When Kudu's integration with the Hive Metastore is enabled
nit: this reads as a general statement, rather than a statement of what exists
in Kudu and Impala. Perhaps "Kudu is integrated with a different Hive Metastore
than that used by Impala, Kudu is ..."
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@251
PS4, Line 251: (k
nit: extra parentheses
http://gerrit.cloudera.org:8080/#/c/13318/4/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/13318/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2579
PS4, Line 2579: false
nit: not sure if this is standard practice in the Impala codebase, but might be
nice to annotate the name.
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java:
PS4:
What has changed from AnalyzeDDLTest? It's a bit hard to determine since this
is now a different file. Could we have kept it in the existing file?
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java:
PS4:
Same here: what's changed with AuditingTest here that warrants a new file?
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@333
PS4, Line 333: String kuduMasters = catalog_.getDefaultKuduMasterHosts();
Just making sure I understand this: the only necessary change here was the new
storage handler type, right?
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c
Gerrit-Change-Number: 13318
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Wed, 22 May 2019 01:25:01 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3311/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 21 May 2019 20:22:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 4:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@9
PS3, Line 9: commit
> commit
Done
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@9
PS3, Line 9: dds sup
> adds support for
Done
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@11
PS3, Line 11: l addr
> actual
Done
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@13
PS3, Line 13:
> a managed
Done
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@19
PS3, Line 19: ion is enabled, the Kudu table underneath the
:managed HMS table will follow
> BTW, what happens with the name of the table created in Kudu when the integ
You mean a Kudu table is created with HMS integration enabled and later the
integration is disabled? Yeah, that should be handled by the tool.
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@23
PS3, Line 23:
> nit: Kudu-related
Done
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@23
PS3, Line 23:
> commit
Done
http://gerrit.cloudera.org:8080/#/c/13318/3/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/13318/3/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@329
PS3, Line 329: u t
> nit: drop?
Done
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@329
PS3, Line 329: a manag
> nit: a managed Kudu
Done
http://gerrit.cloudera.org:8080/#/c/13318/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/13318/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@192
PS3, Line 192: public static boolean
isHMSIntegrationEnabledAndValidate(String kuduMasters,
> Where is this used?
This will be used when the DDL actual handling. And I don't see there is a need
to expose HiveMetastoreConfig so far. As its content is only used for
validation HMS Uris so far.
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@194
PS3, Line 194: HiveMetastoreConfig hmsConfig =
getHiveMetastoreConfig(kuduMasters);
> hmsConfig could be null here.
Done
http://gerrit.cloudera.org:8080/#/c/13318/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/13318/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@440
PS3, Line 440: .KuduStorageHa
> Is it possible to create a Kudu table with STORED AS KUDU syntax and simult
It is possible for appropriate storage handler, added such a test. For non
appropriate is covered below.
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@485
PS3, Line 485: g.format("Tab
> nit: a Kudu table (should be fixed in corresponding place)
Done
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@1
PS3, Line 1: // Licensed to the Apache Software Foundation (ASF) under one
> License header
Done
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@22
PS3, Line 22: il.Set;
> What about ALTER TABLE? If generating auditing events for ALTER is not sup
This is extracted from AuditingTest class which has coverage for ALTER
statements. My understanding is this is for Kudu specific/only DDLs/DMLs.
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@24
PS3, Line 24: import org.apache.impala.authorization.AuthorizationException;
> What about CTAS statement? Do we expect both CREATE and SELECT audit event
Yeah, added a test in AuditingTest.
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@81
PS3, Line 81: Assert.assertEquals(accessEvents, Sets.newHashSet(
: new TAccessEvent("functional_kudu.testtbl",
: TCatalogObjectType.TABLE, "SELECT"),
: new TAccessEvent("functional_kudu.alltypes",
> What about 'DROP TABLE ... IF EXISTS'?
Done
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch:
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hello Thomas Marshall, Mike Percy, Alexey Serbin, Andrew Wong, Adar Dembo, Grant Henke, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13318 to look at the new patch set (#4). Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commit adds support for the syntax of CREATE TABLE (and CTAS) statements for managed Kudu tables with Kudu/HMS integration. A follow up patch will address the actual handling of CREATE TABLE statement with Kudu/HMS integration. For a managed table the syntax remains the same. However, the detailed changes includes: 1) Kudu table will always be created with the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' even when Kudu/HMS integration is disabled. The legacy storage handler will be eventually deprecated. 2) When Kudu/HMS integration is enabled, the Kudu table underneath the managed HMS table will follow the naming convention 'db_name.table_name' instead of 'impala::db_name.table_name'. 3) Add 'kudu.table_id' table property to be used with Kudu/HMS integration. This commit also extracts Kudu-related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c --- M bin/run-all-tests.sh M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java A fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java M testdata/cluster/node_templates/common/etc/init.d/kudu-master 15 files changed, 906 insertions(+), 581 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13318/4 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 3:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@9
PS3, Line 9: commits
commit
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@9
PS3, Line 9: support
adds support for
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@11
PS3, Line 11: actaul
actual
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@13
PS3, Line 13: managed
a managed
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@19
PS3, Line 19: the naming convention 'db_name.table_name' instead of
:'impala::db_name.table_name'.
BTW, what happens with the name of the table created in Kudu when the
integration with HMS was enabled upon toggling off HMS integration? Will it be
re-named by automatically or by one of the tools?
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@23
PS3, Line 23: commits
commit
http://gerrit.cloudera.org:8080/#/c/13318/3//COMMIT_MSG@23
PS3, Line 23: Kudu related
nit: Kudu-related
http://gerrit.cloudera.org:8080/#/c/13318/3/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/13318/3/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@329
PS3, Line 329: ()
nit: drop?
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@329
PS3, Line 329: managed
nit: a managed Kudu
http://gerrit.cloudera.org:8080/#/c/13318/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/13318/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@440
PS3, Line 440: STORED AS KUDU
Is it possible to create a Kudu table with STORED AS KUDU syntax and
simultaneously specifying the appropriate/non-appropriate 'storage_handle'
property?
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@485
PS3, Line 485: an Kudu table
nit: a Kudu table (should be fixed in corresponding place)
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@22
PS3, Line 22: TestKuduStatements
What about ALTER TABLE? If generating auditing events for ALTER is not
supported explicitly, please add a comment about that.
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@24
PS3, Line 24: // Select
What about CTAS statement? Do we expect both CREATE and SELECT audit events to
be issued?
http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@81
PS3, Line 81: // Drop table
: accessEvents = AnalyzeAccessEvents("drop table
functional_kudu.testtbl");
: Assert.assertEquals(accessEvents, Sets.newHashSet(new
TAccessEvent(
: "functional_kudu.testtbl", TCatalogObjectType.TABLE,
"DROP")));
What about 'DROP TABLE ... IF EXISTS'?
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c
Gerrit-Change-Number: 13318
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Tue, 21 May 2019 00:46:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/13318/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/13318/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@192 PS3, Line 192: public static boolean isHMSIntegrationEnabledAndValidate(String kuduMasters, Where is this used? Could this be `validateHiveMetastoreConfig(HiveMetastoreConfig hmsConfig)` and take the HiveMetastoreConfig from getHiveMetastoreConfig. I imagine the code that would want to validate would also need the config to use. http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@194 PS3, Line 194: HiveMetastoreConfig hmsConfig = getHiveMetastoreConfig(kuduMasters); hmsConfig could be null here. http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java File fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java: http://gerrit.cloudera.org:8080/#/c/13318/3/fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java@1 PS3, Line 1: package org.apache.impala.analysis; License header -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 20 May 2019 13:18:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 3: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/3292/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 20 May 2019 07:28:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hao Hao has abandoned this change. ( http://gerrit.cloudera.org:8080/13374 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Abandoned -- To view, visit http://gerrit.cloudera.org:8080/13374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Ib02e46109bec4180b037452fd6a6ab8054e7b13f Gerrit-Change-Number: 13374 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 3:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/13318/2/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/13318/2/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@266
PS2, Line 266: throw new AnalysisException("Invalid storage handler
specified for Kudu table: " +
> Use !KuduTable.isKuduStorageHandler(...) here.
Done
http://gerrit.cloudera.org:8080/#/c/13318/2/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/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@57
PS2, Line 57: // Alias to the string key that identifies the storage handler
for Kudu tables.
> This isn't used anywhere
Done
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@167
PS2, Line 167: try {
> How difficult would this be to implement and what's the failure mode if its
I removed the TODO and updated the actual validation.
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@220
PS2, Line 220: KuduClient kuduClient =
KuduUtil.getKuduClient(getKuduMasterHosts());
> I think this can be reverted to be the KEY_TABLE_NAME and the error message
Done
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@227
PS2, Line 227: }
> I don't think this comment is relevant anymore, this isn't generating a nam
Done
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@229
PS2, Line 229:
> See my comment about about always using kuduTableName_ from KEY_TABLE_NAME.
Done
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@a193
PS2, Line 193:
> Why did this need to change?
Reverted it back.
http://gerrit.cloudera.org:8080/#/c/13318/2/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/13318/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2579
PS2, Line 2579: newTableName.getDb(), newTableName.getTbl(), false);
> Don't we need to check if HMS is enabled?
Yes, this will be a follow up change.
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java
File fe/src/main/java/org/apache/impala/util/KuduUtil.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@387
PS2, Line 387: me.metastoreTableName'. This
:* should only b
> What does this mean?
I think this refers to external table which the Kudu table name is provided by
the users.
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@390
PS2, Line 390: String metastoreDbName,
: String metastoreTableName, boolean
isHMSIntegrationEnabled) {
: return isHMSIntegrationEnabled ? metastoreDbName + "." +
metastoreTableNam
> Please try to match the formatting in the rest of the file, here and elsewh
Done
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@397
PS2, Line 397:* Check if the given name is the default Kudu table name for
managed table
> brief comment
Done
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java@65
PS2, Line 65: "SELECT"),
> line too long (93 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java
File fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java@39
PS2, Line 39:
> This makes me nervous. We don't currently have any tests that restart any m
Thanks for the suggestion!
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hello Thomas Marshall, Mike Percy, Alexey Serbin, Andrew Wong, Adar Dembo, Grant Henke, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13318 to look at the new patch set (#3). Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commits support the syntax of CREATE TABLE (and CTAS) statements for managed Kudu tables with Kudu/HMS integration. A follow up patch will address the actaul handling of CREATE TABLE statement with Kudu/HMS integration. For managed table the syntax remains the same. However, the detailed changes includes: 1) Kudu table will always be created with the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' even when Kudu/HMS integration is disabled. The legacy storage handler will be eventually deprecated. 2) When Kudu/HMS integration is enabled, the Kudu table underneath the managed HMS table will follow the naming convention 'db_name.table_name' instead of 'impala::db_name.table_name'. 3) Add 'kudu.table_id' table property to be used with Kudu/HMS integration. This commits also extracts Kudu related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c --- M bin/run-all-tests.sh M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java A fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java M testdata/cluster/node_templates/common/etc/init.d/kudu-master 15 files changed, 873 insertions(+), 578 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13318/3 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13374 Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commits support the syntax of CREATE TABLE (and CTAS) statements for managed Kudu tables with Kudu/HMS integration. A follow up patch will address the actaul handling of CREATE TABLE statement with Kudu/HMS integration. For managed table the syntax remains the same. However, the detailed changes includes: 1) Kudu table will always be created with the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' even when Kudu/HMS integration is disabled. The legacy storage handler will be eventually deprecated. 2) When Kudu/HMS integration is enabled, the Kudu table underneath the managed HMS table will follow the naming convention 'db_name.table_name' instead of 'impala::db_name.table_name'. 3) Add 'kudu.table_id' table property to be used with Kudu/HMS integration. This commits also extracts Kudu related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: Ib02e46109bec4180b037452fd6a6ab8054e7b13f --- M bin/run-all-tests.sh M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java A fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java M testdata/cluster/node_templates/common/etc/init.d/kudu-master 15 files changed, 873 insertions(+), 578 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/13374/1 -- To view, visit http://gerrit.cloudera.org:8080/13374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ib02e46109bec4180b037452fd6a6ab8054e7b13f Gerrit-Change-Number: 13374 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13318/2/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/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@57
PS2, Line 57: private static final Logger LOG =
Logger.getLogger(KuduTable.class);
This isn't used anywhere
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@167
PS2, Line 167: // TODO: validate Kudu is configured to use the same HMS as
Impala.
How difficult would this be to implement and what's the failure mode if its not
the case?
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java
File fe/src/main/java/org/apache/impala/util/KuduUtil.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@390
PS2, Line 390: String metastoreDbName,
:String
metastoreTableName,
:boolean
isHMSIntegrationEnabled
Please try to match the formatting in the rest of the file, here and elsewhere.
Eg. don't separate all parameters onto their own lines, just wrap as necessary
and indent four spaces.
You might want to look into clang-format-diff as linked here:
https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala
though note that its more of a guideline than a rule, and in places where it
differs from the existing formatting you should just match what we already do
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@397
PS2, Line 397: public static boolean isDefaultKuduTableName(String name,
brief comment
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java
File fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java@39
PS2, Line 39: public class KuduHMSIntegrationTest {
This makes me nervous. We don't currently have any tests that restart any
minicluster components like this. There are a lot of tests that will run after
this that depend on the minicluster being set up in a particular way and we
should be careful not to mess with those.
I think that what we should do is essentially define a new test suite that will
run at the very end.
I have a patch out right now that does something similar:
https://gerrit.cloudera.org/#/c/13337/ You can copy what I've done there:
- Put this class in its own package, say: org.apache.impala.customservice
- Copy the changes I made to bin/run-all-tests.sh with appropriate modifications
- It would also be great if you could define a generic
'restartMiniclusterComponent' function in a utility class like the
CustomClusterRunner class that I added that takes a component name (i.e.
'kudu') and a set of extra env variables to set and then use that in
setup/cleanup
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@24
PS2, Line 24: import org.apache.impala.catalog.KuduTable;
nit: not needed
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@51
PS2, Line 51: import org.apache.impala.catalog.KuduTable;
nit: not needed
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c
Gerrit-Change-Number: 13318
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Fri, 17 May 2019 21:38:32 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/13318/2/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/13318/2/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@266 PS2, Line 266: !handler.equals(KuduTable.KUDU_STORAGE_HANDLER) && Use !KuduTable.isKuduStorageHandler(...) here. http://gerrit.cloudera.org:8080/#/c/13318/2/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/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@220 PS2, Line 220: String fullTableName = msTbl.getDbName() + "." + msTbl.getTableName(); I think this can be reverted to be the KEY_TABLE_NAME and the error messages below can say > "... property found for Kudu table" + kuduTableName_ http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@227 PS2, Line 227: // When Kudu/HMS integration is enabled, generates the Kudu table name from I don't think this comment is relevant anymore, this isn't generating a name. http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@229 PS2, Line 229: kuduTableName_ = msTable_.getParameters().get(KuduTable.KEY_TABLE_NAME); See my comment about about always using kuduTableName_ from KEY_TABLE_NAME. http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java: http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@a193 PS2, Line 193: Why did this need to change? http://gerrit.cloudera.org:8080/#/c/13318/2/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/13318/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2579 PS2, Line 2579: newTableName.getDb(), newTableName.getTbl(), false); Don't we need to check if HMS is enabled? http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java File fe/src/main/java/org/apache/impala/util/KuduUtil.java: http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@387 PS2, Line 387: This assumes a custom name was :* not provided. What does this mean? -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 17 May 2019 13:22:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 2: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/3260/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 17 May 2019 06:37:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 1:
(56 comments)
http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@7
PS1, Line 7: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
> I was under the impression there would need to be special coordination betw
Yeah, the handling part will be in a follow up patch.
http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@9
PS1, Line 9: CATS
> CTAS - Create Table As Select?
Done
http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@9
PS1, Line 9: statments
> statements
Done
http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@13
PS1, Line 13: CREATE EXTERNAL TABLE t STORED AS KUDU TBLPROPERTIES
> How is this different then it was before? Why does it need to be different?
I updated the patch to be only about managed table, due to the in flight
discussion.
http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@21
PS1, Line 21: 2) When Kudu/HMS integration is enabled, the external table is no
longer
> I think this discussion is still in flight with Kudu devs.
Yes, I updated the patch to be only about managed table, due to the in flight
discussion.
http://gerrit.cloudera.org:8080/#/c/13318/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/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@255
PS1, Line 255: private void analyzeKuduTableProperties(Analyzer analyzer,
boolean isHMSIntegrationEnabled)
> line too long (93 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@270
PS1, Line 270: // TODO(hao): remove ALL privileges on SERVER validation
when HMS Integration is enabled.
> line too long (95 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@285
PS1, Line 285: (!isHMSIntegrationEnabled &&
!handler.equals(KuduTable.KUDU_LEGACY_STORAGE_HANDLER {
> line too long (97 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@304
PS1, Line 304: String.format("Table property %s should not be specified
when creating an Kudu table.",
> line too long (95 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@309
PS1, Line 309:* Populates Kudu master addresses either from table property
or the -kudu_master_hosts flag.
> line too long (96 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@322
PS1, Line 322: private void analyzeExternalKuduTableParams(Analyzer analyzer,
boolean isHMSIntegrationEnabled)
> line too long (97 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@353
PS1, Line 353: private void analyzeManagedKuduTableParams(Analyzer analyzer,
boolean isHMSIntegrationEnabled)
> line too long (96 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/13318/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/13318/1/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@329
PS1, Line 329: // When Kudu/HMS integration is not enabled, remove hidden
table property
> What about legacy tables?
Yeah, I think here is to cover legacy tables with Kudu, to remove TABLE_NAME
property with managed table.
http://gerrit.cloudera.org:8080/#/c/13318/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/13318/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@130
PS1, Line 130: public String getStorageHandlerClassName() {
> Should this return the handler from the HMS instead of generating one based
Actually I think I will just use KUDU_STORAGE_HANDLER here for two reasons. 1)
Going forward we are deprecating KUDU_LEGACY_STORAGE_HANDLER no matter HMS
integration is enabled. 2) This is only used at ToSqlUtils
https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java#L317,
which is just to set to null if it is a KuduTable
(https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java#L326).
But please correct me if my understanding is wrong.
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java@65
PS2, Line 65: new TAccessEvent("_impala_builtins",
TCatalogObjectType.DATABASE, "VIEW_METADATA")));
line too long (93 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c
Gerrit-Change-Number: 13318
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Fri, 17 May 2019 05:56:05 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hello Thomas Marshall, Mike Percy, Alexey Serbin, Andrew Wong, Adar Dembo, Grant Henke, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13318 to look at the new patch set (#2). Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commits support the syntax of CREATE TABLE (and CTAS) statements for managed Kudu tables with Kudu/HMS integration. A follow up patch will address the actaul handling of CREATE TABLE statement with Kudu/HMS integration. For managed table the syntax remains the same. However, the detailed changes includes: 1) Kudu table will always be created with the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' even when Kudu/HMS integration is disabled. The legacy storage handler will be eventually deprecated. 2) When Kudu/HMS integration is enabled, the Kudu table underneath the managed HMS table will follow the naming convention 'db_name.table_name' instead of 'impala::db_name.table_name'. 3) Add 'kudu.table_id' table property to be used with Kudu/HMS integration. This commits also extracts Kudu related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c --- M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java A fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java 13 files changed, 825 insertions(+), 572 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13318/2 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 1:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@7
PS1, Line 7: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
I was under the impression there would need to be special coordination between
calls to create the table in HMS and Kudu based on the design doc.
In the legacy world, Impala creates an HMS entry and a Kudu table.
Now, I think we just create the Kudu table and that auto-populates the HMS. But
I didn't see the calls to the HMS removed in this patch.
For legacy tables, calls to HMS likely still need to occur. Though those are
not likely create table calls, but alter and drop calls.
http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@9
PS1, Line 9: CATS
CTAS - Create Table As Select?
http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@9
PS1, Line 9: statments
statements
http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@13
PS1, Line 13: CREATE EXTERNAL TABLE t STORED AS KUDU TBLPROPERTIES
How is this different then it was before? Why does it need to be different?
http://gerrit.cloudera.org:8080/#/c/13318/1//COMMIT_MSG@21
PS1, Line 21: 2) When Kudu/HMS integration is enabled, the external table is no
longer
I think this discussion is still in flight with Kudu devs.
http://gerrit.cloudera.org:8080/#/c/13318/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/13318/1/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@329
PS1, Line 329: // When Kudu/HMS integration is not enabled, remove hidden
table property
What about legacy tables?
http://gerrit.cloudera.org:8080/#/c/13318/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/13318/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@130
PS1, Line 130: public String getStorageHandlerClassName() {
Should this return the handler from the HMS instead of generating one based on
HMS integration being enabled?
I suspect there are two cases, the first is when creating a table, and the
second is when using an already created table. When creating a table I suspect
KUDU_STORAGE_HANDLER would always be used. When using an already created table,
it could be KUDU_LEGACY_STORAGE_HANDLER, even if the HMS integration was turned
on.
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@183
PS1, Line 183: hmsConfig = kuduClient.getHiveMetastoreConfig();
Do we care if Kudu is configured to use a different HMS than Impala? They need
to be the same right?
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@245
PS1, Line 245: // When Kudu/HMS integration is enabled, generates the
Kudu table name from
Does it matter if it's a legacy table here? In that case it will be
`impala::db_name.table_name` right?
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java:
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@212
PS1, Line 212: // When Kudu/HMS integration is enabled, generates the
Kudu table name from
Can this logic be shared with the table name logic in KuduTable.load? Perhaps
factored next to the table name function in KuduUtil.
http://gerrit.cloudera.org:8080/#/c/13318/1/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/13318/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2578
PS1, Line 2578: String newKuduTableName =
KuduUtil.getLegacyDefaultKuduTableName(
What about when HMS integration is enabled?
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/util/KuduUtil.java
File fe/src/main/java/org/apache/impala/util/KuduUtil.java:
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/util/KuduUtil.java@397
PS1, Line 397: public static String getKuduTableName(String metastoreDbName,
Maybe this should take a boolean for HMS enabled, and return the correct format
based on that boolean.
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java:
PS1:
Because this doesn't
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/3193/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 13 May 2019 07:25:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13318 Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commits support the syntax of CREATE TABLE (and CATS) statments for both managed and external Kudu tables with Kudu/HMS integration. Syntax example for external table with Kudu/HMS integration enabled: CREATE EXTERNAL TABLE t STORED AS KUDU TBLPROPERTIES For managed table the syntax remains the same. The detailed changes includes: 1) Add the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' that is used in Kudu/HMS integration. Tables with either the legacy storage handler 'com.cloudera.kudu.hive.KuduStorageHandler' and the one are considered as Kudu tables. 2) When Kudu/HMS integration is enabled, the external table is no longer allowed to set table property TABLE_NAME. The name of underlying Kudu table is derived from the HMS table with the format 'db_name.table_name'. 3) Add 'kudu.table_id' table property for both managed and external table to match with the new metadata format with Kudu/HMS integration. This commits also extracts Kudu related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c --- M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java A fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java 13 files changed, 884 insertions(+), 595 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13318/1 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 1:
(41 comments)
http://gerrit.cloudera.org:8080/#/c/13318/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/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@255
PS1, Line 255: private void analyzeKuduTableProperties(Analyzer analyzer,
boolean isHMSIntegrationEnabled)
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@270
PS1, Line 270: // TODO(hao): remove ALL privileges on SERVER validation
when HMS Integration is enabled.
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@285
PS1, Line 285: (!isHMSIntegrationEnabled &&
!handler.equals(KuduTable.KUDU_LEGACY_STORAGE_HANDLER {
line too long (97 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@304
PS1, Line 304: String.format("Table property %s should not be specified
when creating an Kudu table.",
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@309
PS1, Line 309:* Populates Kudu master addresses either from table property
or the -kudu_master_hosts flag.
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@322
PS1, Line 322: private void analyzeExternalKuduTableParams(Analyzer analyzer,
boolean isHMSIntegrationEnabled)
line too long (97 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@353
PS1, Line 353: private void analyzeManagedKuduTableParams(Analyzer analyzer,
boolean isHMSIntegrationEnabled)
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/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/13318/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@177
PS1, Line 177: public static boolean isHMSIntegrationEnabled(String
kuduMasters) throws ImpalaRuntimeException {
line too long (99 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@242
PS1, Line 242: throw new TableLoadingException("Unable to get Kudu HMS
integration configuration " +
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java:
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@208
PS1, Line 208: throw new LocalCatalogException("Unable to get Kudu HMS
integration configuration " +
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/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/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@182
PS1, Line 182: AnalyzesOk(String.format("create table tab (x int primary
key) partition by hash (x) " +
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@190
PS1, Line 190: AnalyzesOk(String.format("create table tdata_no_port (id int
primary key, name string, " +
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@201
PS1, Line 201: AnalyzesOk(String.format("create table tab (x int primary
key) stored as kudu tblproperties (" +
line too long (100 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@423
PS1, Line 423: String.format("Table property %s should not be specified
when creating an Kudu table.",
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@437
PS1, Line 437: AnalysisError("create external table t stored as kudu
tblproperties('kudu.table_name'='t')",
line too long (98 > 90)
http://gerrit.cloudera.org:8080/#/c/13318/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@438
PS1, Line 438: String.format("Table property %s should not be
specified when
