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,IMPALA-4141: Support multiple partitions in ALTER TABLE ADD PARTITION
Date Mon, 10 Oct 2016 10:04:47 GMT
Attila Jeges has posted comments on this change.

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

Patch Set 14:

File fe/src/main/java/com/cloudera/impala/analysis/

PS14, Line 89:  = false;
> No need for the assignment as this variable is assigned in all paths to lin
File fe/src/main/java/com/cloudera/impala/analysis/

PS14, Line 211: new Comparator<PartitionKeyValue>() {
              :       @Override
              :       public int compare(PartitionKeyValue t, PartitionKeyValue o) {
              :         return t.getColName().compareTo(o.getColName());
              :       }
> What do you think about implementing the Comparable interface for Partition
I was thinking about that too, but comparing the value part of PartitionKeyValue objects is
not that straightforward. To simplify things, I could just compare the PartitionKeyValue.toString()
values, but that might not be the ordering that users of PartitionKeyValue expect.

In PartitionSpec class, analyze() ensures that there are no duplicate keys, therefore comparing
keys only is sufficient and the issue of comparing values can be avoided.
File fe/src/main/java/com/cloudera/impala/service/

PS13, Line 1634: IF NOT EXISTS is used, conflicts are hand
> Same comment as above. I suppose this one is "IF NOT EXISTS is used" and th

PS13, Line 1720: 
> nit: long line

To view, visit
To unsubscribe, visit

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