Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13334 )
Change subject: acid: Filter unwanted files based on ACID state. ...................................................................... Patch Set 14: (4 comments) I think Sudhanshu's AFK this afternoon so will try to rev the patch in his absence. http://gerrit.cloudera.org:8080/#/c/13334/14/fe/src/main/java/org/apache/impala/util/AcidUtils.java File fe/src/main/java/org/apache/impala/util/AcidUtils.java: http://gerrit.cloudera.org:8080/#/c/13334/14/fe/src/main/java/org/apache/impala/util/AcidUtils.java@48 PS14, Line 48: (?:/.*)? > I have deltas like this produced by compaction: Good point. I'll see if I can add a test case for this one. http://gerrit.cloudera.org:8080/#/c/13334/14/fe/src/main/java/org/apache/impala/util/AcidUtils.java@54 PS14, Line 54: // Optional path suffix. : "(?:/.*)?"); > Is this optional? We should only match files inside directories as far as I yea, I guess in contrast to base patterns, the existence of an empty delta directory doesn't actually matter, so we could skip over the directories and only match on the files. That said, I don't think there's harm in matching the directories and it seems less surprising if this regex matches the dir (same as above) http://gerrit.cloudera.org:8080/#/c/13334/14/fe/src/main/java/org/apache/impala/util/AcidUtils.java@105 PS14, Line 105: public boolean test(String dirPath) { > To support upgraded tables this should also accept any path without "/", so Good point. Mind if I add a TODO to support upgraded tables? I'm not sure how to test this at the moment and this patch is already large. http://gerrit.cloudera.org:8080/#/c/13334/14/fe/src/main/java/org/apache/impala/util/AcidUtils.java@182 PS14, Line 182: // If the table was not transaction table then return all fds. : if (validDescriptors.isEmpty()) { : return fileDescriptors; : } > Can't we enter this if the table was truncated, but the old deltas are stil Yea, I think that's true. Unfortunately right now due to https://issues.apache.org/jira/browse/HIVE-20137 and https://issues.apache.org/jira/browse/HIVE-21750 there doesn't seem to be a way to test this case at the moment. I'll fix it anyway and add a unit test. -- To view, visit http://gerrit.cloudera.org:8080/13334 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf0aeb36e10c827ead59ed7f67e731199394fe8e Gerrit-Change-Number: 13334 Gerrit-PatchSet: 14 Gerrit-Owner: Sudhanshu Arora <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sudhanshu Arora <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Yongzhi Chen <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 22 May 2019 20:34:46 +0000 Gerrit-HasComments: Yes
