Attila Jeges has posted comments on this change.

Change subject: IMPALA-1670: Support multiple partitions in ALTER TABLE ADD 

Patch Set 7:

File fe/src/main/cup/sql-parser.cup:

PS7, Line 879: ArrayList
> If possible, can you switch to using the interface (List) instead?

Line 882:   | partition_params_list:list
> is the new line needed here ? Does it work if I have:
It works, I used newline for readability. 

Since the line is short enough, I decided to get rid of the newline, though.

PS7, Line 43: @param tableName Name of the table to add partitions to
            :    * @param ifNotExists If true, no error is raised if a 
partition with the same spec
            :    * already exists. If multiple partitions are specified, the 
statement will ignore
            :    * those that exist and add the rest.
            :    * @param partitions The list of partitions to add to the table.
> As you've probably realized, we don't use Javadoc. Can you plz remove this?

PS7, Line 87: Set<Set<String>> partitionSpecs = Sets.newHashSet();
> I wonder if this is the most efficient way to ensure unique partition specs
File fe/src/main/java/com/cloudera/impala/analysis/

PS7, Line 40: // Set during analysis
> I don't see this being set in the analyze() fn. Actually, in that function 

PS7, Line 52: public void setTable(Table table) {
            :     table_ = table;
            :   }
> single line

PS7, Line 55:   public void setTableName(TableName tableName) {
            :     partitionSpec_.setTableName(tableName);
            :   }
            :   public void setPartitionShouldNotExist() {
            :     partitionSpec_.setPartitionShouldNotExist();
> Can't we just make PartitionParams subclass PartitionSpec and avoid duplica
To make that work, the return value of PartitionSpec.toThrift() should be 
changed to TPartitionParams, which then would cause other issues.

Besides these technical problems, I feel that the relationship between 
PartitionParams and PartitionSpec objects is a "has a" rather than an "is a".
File fe/src/main/java/com/cloudera/impala/service/

Line 375:           // already exist in Hive, then throw ImpalaRuntimeException.
> You may want to comment on the consistency guarantees (if any) here. For ex
I've refactored the code to use the add_partitions() call and clarified the 

alterTableAddPartitions() is still not completely safe though: what happens 
when add_partitions() succeeds, but the subsequent alter_partitions() call 
fails to add caching? In this case, partitions will be added to HMS (without 
caching), but not to catalog cache. These kinds of issues were not handled in 
the previous implementation either.

Line 375:           // already exist in Hive, then throw ImpalaRuntimeException.
> Skimming through the code, looks like there can be partial updates and inco
See my response to Dimitris

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iddbc951f2931f488f7048c9780260f6b49100750
Gerrit-PatchSet: 7
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

Reply via email to