Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/10543 )
Change subject: IMPALA-6119: Fix issue with multiple partitions sharing same location ...................................................................... Patch Set 8: (1 comment) Thanks for your comments, Sailesh! This change doesn't cover the case when 2 partitions from different tables point to the same location. I'm not sure that it's a valid real world scenario. I mean I tried and it's feasible to do this but form me it doesn't make sense to point one partition to a different table's directory in the file system. Do you think we should make this documented? I think that it also has to be documented that all the other partitions are dropped on a same location when you drop one of them. http://gerrit.cloudera.org:8080/#/c/10543/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/10543/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1167 PS3, Line 1167: removePartitionFromLocationMapping(partition); > I see, so it's on the caller to decide what all partitions to drop. Excellent point! I assumed that in updatePartitionsFromHms() this would work fine because when it populates msPartitionNames from Hive then it won't contain the dropped partitions and later on when populating partitionsToRemove those will be simply dropped. This in fact is only appears partially true: When you drop a partition with Hive then the next time Impala gets the valid partitions from Hive the dropped one won't show up. However, if there are more partitions that share the same location then only the one that was dropped explicitly will be missing from the list but the other will still show up. That's a bit weird, I'll check with the Hive team to get a better understanding why this happens. However, from Impala side my assumption was incorrect and I have to call getPartitionsWithSameLocation() on partitionsToRemove to fix this. I also added a test to have coverage. Thanks for pointing this out! -- To view, visit http://gerrit.cloudera.org:8080/10543 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a54bc8224bcefe65b83de2df58bb84629f2aa4a Gerrit-Change-Number: 10543 Gerrit-PatchSet: 8 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 07 Jun 2018 13:21:40 +0000 Gerrit-HasComments: Yes
