impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-1670,IMPALA-4141: Support multiple partitions in ALTER TABLE ADD PARTITION
Date Thu, 27 Oct 2016 18:33:43 GMT
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


Minor fixes in the tests. I'll ask Alex to see if he can make a final pass.
File fe/src/main/java/org/apache/impala/analysis/

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 :)
File fe/src/main/java/org/apache/impala/analysis/

PS19, Line 211:  
nit: this is just for comparison purposes, no need to pretty print (save a few bytes :)).
File fe/src/main/java/org/apache/impala/service/

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.
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.
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
nit: long line
File tests/metadata/

PS19, Line 733: rows
nit: with_data

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iddbc951f2931f488f7048c9780260f6b49100750
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <>
Gerrit-Reviewer: Attila Jeges <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-HasComments: Yes

View raw message