Dimitris Tsirogiannis has posted comments on this change.

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


Patch Set 10:

(11 comments)

Flushing some comments. Will focus on testing next.

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

PS10, Line 75: params.setLocation
Why are you setting the location if it's null? isn't this an optional field?


PS10, Line 90: table_ instanceof HdfsTable)
Is PartitionParams class applicable to other table types? If not, set a 
Preconditions check when the table is set and remove the if (hashTable == null) 
checks from below.


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

PS10, Line 210: new ArrayList<PartitionKeyValue>(partitionSpec_);
= Lists.newArrayList(partitionSpec_)?


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

PS8, Line 1733: tPartition(tableName.getDb(), tableName.getTb
> A unique id would work, but I couldn't find one in the documentation of org
Sorry I was thinking of the HdfsPartition class. Never mind...


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

PS10, Line 1640: for partitions not yet in catalog cache, add cache directives
I don't think the comment is accurate. We shouldn't by default add cache 
directives for all the new partitions, no? Only if the table is cached or for 
those partitions that explicitly have the CACHED clause.


PS10, Line 1644: TAlterTableAddPartitionParams addPartParams)
               :       throws ImpalaException {
nit: can these two be in the same line?


PS10, Line 1651: hmsPartitions
Maybe rename to "hmsPartitionsToAdd"?


PS10, Line 1653: new HashMap<List<String>, THdfsCachingOp>();
Maps.newHashMap()?


PS10, Line 1657: partitionSpec)) {
formatting nit: 2 more spaces are needed since this is part of a statement


PS10, Line 1673: partitionCachingOpMap.put(hmsPartition.getValues(), cacheOp);
Why do you populate this map with all the new partitions? Not all the 
partitions necessarily have non-null cacheOp. Can't you simply create a list of 
Partitions that have non-null cacheOp and pass that instead to 
alterTableCachePartitions()?


PS10, Line 1689: hmsPartitions = getPartitionsFromHMS(msClient, tableName, 
hmsPartitions,
               :         addedHmsPartitions);
Do we always need to call this function? What if the size of addedHmsPartitions 
is the same as hmsPartitionsToAdd? Can't we avoid this call in this case?


-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <atti...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to