Introduce PathFilter support in DirectoryLister by bhasudha · Pull Request #13590 · prestodb/presto

bhasudha · 2019-10-23T18:48:24Z

This PR fixes the issue described here - Support for PathFilter in DirectoryLister

== RELEASE NOTES ==

General Changes
* ...
* ...

Hive Changes
* ...
* ...

If release note is NOT required, use:

== NO RELEASE NOTE ==

facebook-github-bot · 2019-10-23T18:48:32Z

Hi bhasudha! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2019-10-23T19:01:36Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

shixuan-fan

Looks good in general, some minor comments

presto-hive/src/main/java/com/facebook/presto/hive/BackgroundHiveSplitLoader.java

shixuan-fan · 2019-10-28T17:06:56Z

presto-hive/src/main/java/com/facebook/presto/hive/DirectoryLister.java

nit: extra line between methods.

I think we could just use the same interface and default other use cases to be Optional.empty(), but I also realized that this might break other private connectors. @wenleix , @arhimondr do you have a suggestion here?

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

shixuan-fan · 2019-10-28T17:09:49Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

nit: How about making this Optional? Or even, can we create PathFilter here?

We want to create a PathFilter instance per query. Thats why I thought creating inside BackgroundHiveSplitLoader would be apt. I made this Optional as you suggested. Let me know what you think.

presto-hive/src/main/java/com/facebook/presto/hive/util/HiveFileIterator.java

presto-hive/src/test/java/com/facebook/presto/hive/TestBackgroundHiveSplitLoader.java

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveClientConfig.java

presto-hive/src/test/java/com/facebook/presto/hive/util/TestHiveFileIterator.java

bhasudha · 2019-10-29T17:16:39Z

Thanks for the detailed review @shixuan-fan . I have responded inline.

shixuan-fan

Looks good to me. I'm not a huge fan of reflection but I don't have a better suggestion here given we want to have different instances. I'll ask @arhimondr / @wenleix to take another pass. Thanks for the good work!

presto-hive/src/main/java/com/facebook/presto/hive/util/HiveFileIterator.java

arhimondr

Some comments

arhimondr · 2019-10-30T18:10:25Z

presto-hive/src/test/java/com/facebook/presto/hive/util/TestHiveFileIterator.java

+    private int fileCountForIgnoredPolicyPathFilterOff;
+    private int fileCountForRecursePolicyPathFilterOff;
+    private Configuration hadoopConf;
+    private Random random;


Use ThreadLocalRandom instead

arhimondr · 2019-10-30T18:10:43Z

presto-hive/src/test/java/com/facebook/presto/hive/util/TestHiveFileIterator.java

+    private Random random;
+    private static final String RANDOM_FILE_NAME_SALT_STRING = "abcdefghijklmnopqrstuvwxyz";
+
+    @AfterClass


nit: put it after @BeforeClass

arhimondr · 2019-10-30T18:11:05Z

presto-hive/src/test/java/com/facebook/presto/hive/util/TestHiveFileIterator.java

+    private Random random;
+    private static final String RANDOM_FILE_NAME_SALT_STRING = "abcdefghijklmnopqrstuvwxyz";
+
+    @AfterClass


Also please nullify the fields to avoid memory leaks

arhimondr · 2019-10-30T18:11:35Z

presto-hive/src/test/java/com/facebook/presto/hive/util/TestHiveFileIterator.java

+        }
+    }
+
+    private void delete(File file)


nit: put this method after it's usage

arhimondr · 2019-10-30T18:12:14Z

presto-hive/src/test/java/com/facebook/presto/hive/util/TestHiveFileIterator.java

+
+public class TestHiveFileIterator
+{
+    private NamenodeStats namenodeStats;


Are all this variables have to be on the class level? Can some of them be local?

yes. I ll refactor them.

arhimondr · 2019-10-30T18:14:52Z

presto-hive/src/test/java/com/facebook/presto/hive/util/TestHiveFileIterator.java

+    @Test
+    public void testPathFilterWithRecursion()
+    {
+        hiveFileIterator = new HiveFileIterator(rootPath, listDirectoryOperation, namenodeStats, RECURSE, Optional.of(pathFilter));


arhimondr · 2019-10-30T18:15:42Z

presto-hive/src/test/java/com/facebook/presto/hive/util/TestHiveFileIterator.java

+    {
+        hiveFileIterator = new HiveFileIterator(rootPath, listDirectoryOperation, namenodeStats, IGNORED, Optional.empty());
+        int actualCount = getFileCount(hiveFileIterator);
+        assertEquals(actualCount, fileCountForIgnoredPolicyPathFilterOff);


Why fileCountForIgnoredPolicyPathFilterOff (and other similar) cannot be simply hardcoded?

By introducing something like hive.input-path-filter-class we are explicitly encouraging jar drop-ins. This is something that we were historically trying to avoid (however it is still possible,e.g.: with storage formats). The "presto way" would be to extend the presto-hive module, and have an interface in presto-hive that allows to extend this behaviour.

@bhasudha What is your usecase? Do you use presto-hive connector as is? Or do you have some wrapper on top of it?

@arhimondr I understand. We are using presto-hive-hadoop2 with drop-ins for custom storage format. The pathfilter class in this case will also be coming from Hudi jars- HoodieROTablePathFilter.

Like @shixuan-fan suggested let me try to introduce a PathFilterProvider interface.

@bhasudha Did you consider having a presto-hive-hoodie connector wrapper in your proprietary codebase? This way you can verify jar dependency compatibility during the build time. Also you can inject whatever extensions you need without introducing runtime class resolutions that can be very error prone.

We use this very approach for the Facebook proprietary extension. You can have a look at DirectoryLister interface. When it doesn't make sense in the scope of presto-hive connector, it allows us to plug in our extensions.

@arhimondr Wrapper connector is a good idea. I can try that. Just wanted to clarify that this is not proprietary. Presto users in Hudi open source community also face this issue today. Since we have already solved this at Uber by patching Presto, thought generalizing that would be useful for everyone.
I can refactor this change into a separate module called 'presto-hudi' connector and update the PR. Let me know if you are okay with that.

Sounds good

arhimondr · 2019-10-30T18:18:53Z

presto-hive/src/test/java/com/facebook/presto/hive/util/TestHiveFileIterator.java

+        List<File> dirs = createDirs(basePath, 2);
+
+        // create files in each subdir along with couple files and one nested directory
+        for (int i = 0; i < dirs.size(); i++) {


suggestion: This might be somehow hard to follow for someone who is not familiar with this test. What do you think about having 4 directories that are created in a straightforward way for each test case? And then hardcode the expected count. Also we can create them inside the test (per test), and not in BeforeClass

sure. i ll refactor the test this way.

arhimondr · 2019-10-30T18:22:39Z

presto-hive/src/main/java/com/facebook/presto/hive/util/HiveFileIterator.java

+            }
            try {
-                return fileStatusIterator.hasNext();
+                while (fileStatusIterator.hasNext()) {


How about wrapping the FileStatusIterator in Iterators#filter from guava? So we don't have to implement filtering logic here?

Hmmm. I tried doing this. It looks like this accepts only Iterator as the input parameter - https://github.com/google/guava/blob/49f5a6332a63737dff70cf77472f9867bc7ca6eb/guava/src/com/google/common/collect/Iterators.java#L629 However the ListDirectoryOperation.list(path) return RemoteIterator.

arhimondr · 2019-10-30T18:23:31Z

presto-hive/src/main/java/com/facebook/presto/hive/DirectoryLister.java

 {
    Iterator<HiveFileInfo> list(FileSystem fileSystem, Path path, NamenodeStats namenodeStats, NestedDirectoryPolicy nestedDirectoryPolicy);
+
+    Iterator<HiveFileInfo> list(FileSystem fileSystem, Path path, NamenodeStats namenodeStats, NestedDirectoryPolicy nestedDirectoryPolicy, Optional<PathFilter> pathFilter);


Let's leave only a single method in the interface (remove the old one)

arhimondr · 2019-10-30T18:28:42Z

By introducing something like hive.input-path-filter-class we are explicitly encouraging jar drop-ins. This is something that we were historically trying to avoid (however it is still possible,e.g.: with storage formats). The "presto way" would be to extend the presto-hive module, and have an interface in presto-hive that allows to extend this behaviour.

@bhasudha What is your usecase? Do you use presto-hive connector as is? Or do you have some wrapper on top of it?

shixuan-fan · 2019-10-30T18:33:02Z

Actually as I was thinking, is it possible to have a PathFilterProvider interface so that the internal use case could provide this, and the default behavior is provide an Optional.empty()?

bhasudha · 2020-02-11T18:10:53Z

Closing this as redundant. PR - #13818 covers these changes.

bhasudha mentioned this pull request Oct 23, 2019

Support for PathFilter in DirectoryLister #13511

Closed

facebook-github-bot added the CLA Signed label Oct 23, 2019

arhimondr requested a review from shixuan-fan October 25, 2019 14:20

arhimondr assigned shixuan-fan Oct 25, 2019

shixuan-fan reviewed Oct 28, 2019

View reviewed changes

bhasudha force-pushed the presto-hudi-optimizations branch from 06ea92e to facc233 Compare October 29, 2019 17:58

Introduce PathFilter support in DirectoryLister

3a4d1c0

bhasudha force-pushed the presto-hudi-optimizations branch from facc233 to 3a4d1c0 Compare October 29, 2019 18:38

shixuan-fan approved these changes Oct 30, 2019

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/util/HiveFileIterator.java Show resolved Hide resolved

arhimondr reviewed Oct 30, 2019

View reviewed changes

bhasudha mentioned this pull request Dec 7, 2019

Support PathFilter when fetching splits for Hudi tables #13818

Merged

bhasudha closed this Feb 11, 2020

Conversation

bhasudha commented Oct 23, 2019

Uh oh!

facebook-github-bot commented Oct 23, 2019

Uh oh!

facebook-github-bot commented Oct 23, 2019

Uh oh!

shixuan-fan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bhasudha commented Oct 29, 2019

Uh oh!

shixuan-fan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arhimondr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arhimondr Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arhimondr commented Oct 30, 2019

arhimondr Nov 1, 2019 •

edited

Loading

shixuan-fan commented Oct 30, 2019 •

edited

Loading