impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Attila Jeges (Code Review)" <ger...@cloudera.org>
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:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4144/14/fe/src/main/java/com/cloudera/impala/analysis/PartitionParams.java
File fe/src/main/java/com/cloudera/impala/analysis/PartitionParams.java:

PS14, Line 89:  = false;
> No need for the assignment as this variable is assigned in all paths to lin
Done


http://gerrit.cloudera.org:8080/#/c/4144/14/fe/src/main/java/com/cloudera/impala/analysis/PartitionSpec.java
File fe/src/main/java/com/cloudera/impala/analysis/PartitionSpec.java:

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.


http://gerrit.cloudera.org:8080/#/c/4144/13/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java
File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java:

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
Done


PS13, Line 1720: 
> nit: long line
Done


-- 
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: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message