Dimitris Tsirogiannis 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 default. You may want to focus on the special cases here: 1. partition exists in cache, ignore 2. partition exists in HMS but not in cache, reload partition from HMS Also remove the comment about HDFS caching, it's not part of resolving conflicts, it's the expected behavior, no? PS12, Line 1657: spec remove Line 1683: Here you may want to add a brief comment explaining that if if_not_exists is true, we may fail to add all the partitions to HMS because some of them may already exist there. So, we need to load in the catalog the partitions that already exist in HMS but aren't in the catalog. PS12, Line 1685: addedHmsPartitions How do you know that the order of the partitions as returned by the add_partitions call is the same as the order of the partitions and their associated cache op specified it in L1668? You can keep the map that you initially had, my original comment was that you don't need to populate it with all the partitions, just with those that have a cache op specified. PS12, Line 1718: hmsPartitions hmsPartitionsToAdd Line 1720: Map<List<String>, Partition> addedPartitionMap = Add a preconditions check that hmsPartitions.size() > addedHmsPartitions.size(). PS12, Line 1726: p nit: Plz refrain from using short names for variables that show up several statements away from the place they are defined. For example, using 'ap' in L1722 is ok. PS12, Line 1746: Store remove PS12, Line 1747: 'partitionCachingOpMap' maps lists of partition values to their corresponding : * HDFS caching ops. Comment is not up-to-date. PS12, Line 1758: cacheHmsPartitions maybe rename to 'hmsPartitionsToCache'? -- 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
