Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13409 )
Change subject: IMPALA-8506: Support RENAME TABLE statement with Kudu/HMS integration ...................................................................... Patch Set 1: (3 comments) Looks good, somewhat just nits here. http://gerrit.cloudera.org:8080/#/c/13409/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/13409/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2584 PS1, Line 2584: // When Kudu's integration with the Hive Metastore is enabled, relies on : // Kudu to rename the HMS entry for managed table. nit: mind updating the comment to explain the conditional block, instead of the !condition? I.e. something like "When Kudu's is not integrated with the Hive Metastore or if this is an external table, Kudu will not automatically update the HMS metadata, so we have to do it manually." http://gerrit.cloudera.org:8080/#/c/13409/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2797 PS1, Line 2797: // TODO: this should be denied since IMPALA-5654? Could you clarify what we need to do here? How does this code need to be updated? Should a jira be filed for this? http://gerrit.cloudera.org:8080/#/c/13409/1/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test: PS1: Does the .test format allow for file-level comments? If so, might be worth explaining the expected setup required to run this test. E.g. does Kudu's HMS integration need to be enabled? If not, perhaps just call this kudu_alter.test? Actually it looks like there is already a kudu_alter.test. Why can't we use that one? -- To view, visit http://gerrit.cloudera.org:8080/13409 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If7155e0b385b8ad81eda0a84277bc85171a88269 Gerrit-Change-Number: 13409 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Thu, 23 May 2019 21:00:21 +0000 Gerrit-HasComments: Yes
