Attila Jeges has posted comments on this change.

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


Patch Set 12:

(10 comments)

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

PS12, Line 1635: add partitions not yet in catalog cache and HMS to HMS
> That is not a conflict resolution process, this is expected to happen by de
Correct, I fixed the comment.


PS12, Line 1657: spec
> remove
Done


Line 1683: 
> Here you may want to add a brief comment explaining that if if_not_exists i
Done


PS12, Line 1685: addedHmsPartitions
> How do you know that the order of the partitions as returned by the add_par
I checked the implementation of add_partitions() (in MetaStoreClient.java) and 
add_partitions_core() (in MetaStore.java) and found that add_partitions() 
returns partitions in the same order as it receives them.

On the other hand it is not documented anywhere, so probably it is not safe to 
take this behavior for granted. I moved back to using the map.


PS12, Line 1718: hmsPartitions
> hmsPartitionsToAdd
Done


Line 1720:     Map<List<String>, Partition> addedPartitionMap =
> Add a preconditions check that hmsPartitions.size() > addedHmsPartitions.si
Done


PS12, Line 1726:  p
> nit: Plz refrain from using short names for variables that show up several 
Done


PS12, Line 1746: Store
> remove
Done


PS12, Line 1747: 'partitionCachingOpMap' maps lists of partition values to 
their corresponding
               :    * HDFS caching ops.
> Comment is not up-to-date.
Moved back to using partitionCachingOpMap map.


PS12, Line 1758: cacheHmsPartitions
> maybe rename to 'hmsPartitionsToCache'?
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: 12
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