Alex Behm has posted comments on this change. Change subject: IMPALA-1670,IMPALA-4141: Support multiple partitions in ALTER TABLE ADD PARTITION ......................................................................
Patch Set 22: (10 comments) Thanks for the clarifications. Change looks pretty good to me. http://gerrit.cloudera.org:8080/#/c/4144/22/fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java: Line 92: throw new AnalysisException( Add a test for this in AnalyzeDDLTest http://gerrit.cloudera.org:8080/#/c/4144/22/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: Line 1859: * 2. If a partition exists in HMS but not in catalog cache, reload partition from HMS. mention that caching directives are only applied to new partitions that were absent from both the catalog cache and the HMS Line 1861: private Table alterTableAddPartitions(Table tbl, Since there are some interesting details with existing partitions (in cache or HMS), I think it would be nice to return the number of partitions actually created to the user. I'm ok with doing that in a follow-on patch. If you decide to go that route, please file a JIRA. http://gerrit.cloudera.org:8080/#/c/4144/22/testdata/workloads/functional-query/queries/QueryTest/alter-table.test File testdata/workloads/functional-query/queries/QueryTest/alter-table.test: Line 1023: # IMPALA-1670: Cannot add duplicate partition specs. remove, this is covered by an analyzer test Line 1033: # IMPALA-1670: Cannot add duplicate partition specs, not even if 'IF NOT EXISTS' is used. remove, this is covered by an analyzer test Line 1056: # IMPALA-1670: If 'IF NOT EXISTS' is not used, ALTER TABLE ADD PARTITION cannot add a This should be covered by an analyzer test instead Line 1066: # IMPALA-1670: If adding one partition fails, the entire statement fails. Not really accurate. There are also modes of partial failure/success. If analysis fails, it's clear no state should have changed (because the stmt does not go into runtime). Line 1076: # IMPALA-1670: If 'IF NOT EXISTS' is used ALTER TABLE ADD PARTITION works with preexisting Nice! Line 1150: # IMPALA-1670: After REFRESH Impala can access previously added partitions and partition Seems a little excessive to me, not sure if we need this test. http://gerrit.cloudera.org:8080/#/c/4144/22/testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test File testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test: Line 242: # IMPALA-1670: Revoke privileges, so that user would not have privileges to run ALTER These cases should be covered by AuthorizationTest already. I don't think we need to test grant/revoking and then performing this ALTER TABLE specifically. What do you think, Dimitris? -- To view, visit http://gerrit.cloudera.org:8080/4144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iddbc951f2931f488f7048c9780260f6b49100750 Gerrit-PatchSet: 22 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-HasComments: Yes
