impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Attila Jeges (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-1670: Support multiple partitions in ALTER TABLE ADD PARTITION
Date Wed, 14 Sep 2016 16:48:42 GMT
Attila Jeges has posted comments on this change.

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

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.
File fe/src/main/java/com/cloudera/impala/analysis/

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
            :    * 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 comment.

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

View raw message