Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-1670,IMPALA-4141: Support multiple partitions in ALTER TABLE ADD PARTITION ......................................................................
Patch Set 19: Code-Review+1 (8 comments) Minor fixes in the tests. I'll ask Alex to see if he can make a final pass. http://gerrit.cloudera.org:8080/#/c/4144/19/fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java: PS19, Line 73: List<TPartitionParams> partParams = Lists.newArrayList(); : for (PartitionParams p: partitions_) partParams.add(p.toThrift()); I think you can simply use TAlterTableParams.AddToPartition_params(). It already handles the initialization of the list and can save you a few precious lines :) http://gerrit.cloudera.org:8080/#/c/4144/19/fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java File fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java: PS19, Line 211: nit: this is just for comparison purposes, no need to pretty print (save a few bytes :)). http://gerrit.cloudera.org:8080/#/c/4144/19/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: PS19, Line 1810: addedHmsPartitions.addAll(getPartitionsFromHms(msClient, difference)); Let's try to be a bit more defensive here. If getPartiionsFromHms() throws an exception we should inform the user that the some metadata inconsistency has occurred and the user should run INVALIDATE METADATA <tbl> to recover. http://gerrit.cloudera.org:8080/#/c/4144/19/testdata/workloads/functional-query/queries/QueryTest/alter-table.test File testdata/workloads/functional-query/queries/QueryTest/alter-table.test: PS19, Line 963: drop table if exists i1670_alter; You don't need the drop table statements. Plz remove PS19, Line 987: Duplicate partition spec: (j=11, n='alex', i=1) Isn't this an analysis exception? I believe it will be an ImpalaRuntimeException which you can catch here if "IF NOT EXISTS" is not used. PS19, Line 990: # IMPALA-1670: IF NOT EXISTS works as expected: : # - ignores existing partitions and adds the rest. : # - location and caching options of existing partitions are not modified. : # - if adding one partition fails, the entire statement fails. The query that follows this comment does not use the IF NOT EXISTS clause which is confusing. You may want to remove this and add a comment before each ALTER TABLE stmt that indicates what's being currently tested. http://gerrit.cloudera.org:8080/#/c/4144/19/testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test File testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test: PS19, Line 242: # IMPALA-1670: Revoke privileges, so that user would not have privileges to run ALTER TABLE nit: long line http://gerrit.cloudera.org:8080/#/c/4144/19/tests/metadata/test_hms_integration.py File tests/metadata/test_hms_integration.py: PS19, Line 733: rows nit: with_data -- 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: 19 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-HasComments: Yes
