GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java #35034

noahfrn · 2023-04-11T10:28:29Z

I'd like to add support for multi-file datasets (in different directories) in the Java bindings.
This PR adds to the existing Datasets API to allow an array of URIs to be passed in. The logic for whether the URIs passed are valid or not exists in the JNI layer.

Closes: [Java] [Datasets] Add support for multi-file datasets from Java #35033

github-actions · 2023-04-11T10:28:48Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2023-04-11T10:31:01Z

Closes: [Java] [Datasets] Add support for multi-file datasets from Java #35033

noahfrn · 2023-04-11T11:02:09Z

Unsure why the flight-core test is failing, doesn't seem related to my change.

lidavidm · 2023-04-11T11:22:49Z

@davisusanibar

java/dataset/src/main/cpp/jni_wrapper.cc

davisusanibar · 2023-04-11T22:03:21Z

java/dataset/src/main/cpp/jni_wrapper.cc

+        [] (const auto& p1, const auto& p2) {
+          return p1.first->type_name() == p2.first->type_name();
+        }) - filesystems.begin() != 1) {
+    JniThrow("Different filesystems are not supported in a multi-file dataset.");


Could be possible to add the message that contains detail about what different filesystem the user are trying to use?

We should be using #34420

Agreed on both counts, I'll wait for #34420 to clean this up a bit. and then create a better exception.

@noahfournier FYI #34420 is merged now

java/dataset/src/main/java/org/apache/arrow/dataset/file/JniWrapper.java

davisusanibar · 2023-04-11T22:06:39Z

java/dataset/src/test/java/org/apache/arrow/dataset/file/TestFileSystemDataset.java

+
+    AutoCloseables.close(datum);
+
+  }


Could be possible to add testing for exception throws: (1) Invalid URI , (2) ensure that they all share a FileSystem type

java/dataset/src/main/java/org/apache/arrow/dataset/file/JniWrapper.java

java/dataset/src/main/cpp/jni_wrapper.cc

lidavidm · 2023-04-12T11:15:06Z

java/dataset/src/main/cpp/jni_wrapper.cc

+        [] (const auto& p1, const auto& p2) {
+          return p1.first->type_name() == p2.first->type_name();
+        }) - filesystems.begin() != 1) {
+    JniThrow("Different filesystems are not supported in a multi-file dataset.");


We should be using #34420

noahfrn · 2023-06-02T11:27:55Z

Sorry for the delay on this one. I've added the test as requested, and cleaned up the implementation using #34420

lidavidm

Thank you!

ursabot · 2023-06-03T20:35:34Z

Benchmark runs are scheduled for baseline = 018e7d3 and contender = cd42895. cd42895 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.62% ⬆️0.09%] test-mac-arm
[Failed] ursa-i9-9960x
[Finished ⬇️0.6% ⬆️0.06%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] cd42895a ec2-t3-xlarge-us-east-2
[Finished] cd42895a test-mac-arm
[Failed] cd42895a ursa-i9-9960x
[Finished] cd42895a ursa-thinkcentre-m75q
[Finished] 018e7d3f ec2-t3-xlarge-us-east-2
[Finished] 018e7d3f test-mac-arm
[Finished] 018e7d3f ursa-i9-9960x
[Finished] 018e7d3f ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

… from Java (apache#35034) I'd like to add support for multi-file datasets (in different directories) in the Java bindings. This PR adds to the existing Datasets API to allow an array of URIs to be passed in. The logic for whether the URIs passed are valid or not exists in the JNI layer. * Closes: apache#35033 Lead-authored-by: NoahFournier <fourniernoah6@gmail.com> Co-authored-by: NoahFournier <63198198+NoahFournier@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com>

Add support for multi-file datasets from Java

d48991e

noahfrn requested a review from lidavidm as a code owner April 11, 2023 10:28

github-actions bot added Component: Java awaiting review Awaiting review labels Apr 11, 2023

noahfrn changed the title ~~35033: [Java] [Datasets] Add support for multi-file datasets from Java~~ GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java Apr 11, 2023

davisusanibar reviewed Apr 11, 2023

View reviewed changes

java/dataset/src/main/cpp/jni_wrapper.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 11, 2023

davisusanibar reviewed Apr 11, 2023

View reviewed changes

java/dataset/src/main/java/org/apache/arrow/dataset/file/JniWrapper.java Show resolved Hide resolved

davisusanibar reviewed Apr 11, 2023

View reviewed changes

Better error message for unrecognized URI + test

f006299

lidavidm mentioned this pull request Apr 12, 2023

GH-34386: [C++] Add a PathFromUriOrPath method #34420

Merged

lidavidm reviewed Apr 12, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 12, 2023

noahfrn added 2 commits April 13, 2023 09:42

Handle empty URI array

f2dcdf8

Fix lint

3e71957

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Apr 13, 2023

noahfrn added 2 commits May 20, 2023 18:45

Merge remote-tracking branch 'origin' into multifile-datasets-java

34c68a9

Additional test, and clean up implementation

8b7ffdb

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 2, 2023

Merge branch 'main' into multifile-datasets-java

920be7e

lidavidm approved these changes Jun 2, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 2, 2023

lidavidm merged commit cd42895 into apache:main Jun 2, 2023

noahfrn deleted the multifile-datasets-java branch July 5, 2023 14:17

GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java #35034

GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java #35034

Uh oh!

Conversation

noahfrn commented Apr 11, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 11, 2023

Uh oh!

github-actions bot commented Apr 11, 2023

Uh oh!

noahfrn commented Apr 11, 2023

Uh oh!

lidavidm commented Apr 11, 2023

Uh oh!

Uh oh!

davisusanibar Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

lidavidm Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

noahfrn Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

ianmcook May 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davisusanibar Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lidavidm Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

noahfrn commented Jun 2, 2023

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

ursabot commented Jun 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

noahfrn commented Apr 11, 2023 •

edited by github-actions bot

Loading