You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "NoahFournier (via GitHub)" <gi...@apache.org> on 2023/04/11 10:28:31 UTC
[GitHub] [arrow] NoahFournier opened a new pull request, #35034: 35033: [Java] [Datasets] Add support for multi-file datasets from Java
NoahFournier opened a new pull request, #35034:
URL: https://github.com/apache/arrow/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] NoahFournier commented on pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "NoahFournier (via GitHub)" <gi...@apache.org>.
NoahFournier commented on PR #35034:
URL: https://github.com/apache/arrow/pull/35034#issuecomment-1503117054
Unsure why the flight-core test is failing, doesn't seem related to my change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a diff in pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #35034:
URL: https://github.com/apache/arrow/pull/35034#discussion_r1189162195
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -533,6 +535,59 @@ Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory(
JNI_METHOD_END(-1L)
}
+/*
+ * Class: org_apache_arrow_dataset_file_JniWrapper
+ * Method: makeFileSystemDatasetFactory
+ * Signature: ([Ljava/lang/String;II)J
+ */
+JNIEXPORT jlong JNICALL
+Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory___3Ljava_lang_String_2I(
+ JNIEnv* env, jobject, jobjectArray uris, jint file_format_id) {
+ JNI_METHOD_START
+
+ using FsPathPair = std::pair<std::shared_ptr<arrow::fs::FileSystem>, std::string>;
+
+ std::shared_ptr<arrow::dataset::FileFormat> file_format =
+ JniGetOrThrow(GetFileFormat(file_format_id));
+ arrow::dataset::FileSystemFactoryOptions options;
+
+ std::vector<std::string> uri_vec = ToStringVector(env, uris);
+
+ // If not all URIs, throw exception
+ if (!std::all_of(uri_vec.begin(), uri_vec.end(), arrow::fs::internal::IsLikelyUri)) {
+ JniThrow("All sources must be valid URIs.");
+ }
+
+ std::vector<FsPathPair> filesystems;
+ filesystems.reserve(uri_vec.size());
+ std::transform(uri_vec.begin(), uri_vec.end(), std::back_inserter(filesystems),
+ [](const auto& s) -> FsPathPair {
+ std::string output_path;
+ auto fs = JniGetOrThrow(arrow::fs::FileSystemFromUri(s, &output_path));
+ return {fs, output_path};
+ });
+
+ // If all URIs, ensure that they all share a FileSystem type
+ if (std::unique(filesystems.begin(), filesystems.end(),
+ [] (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.");
Review Comment:
@NoahFournier FYI https://github.com/apache/arrow/pull/34420 is merged now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #35034:
URL: https://github.com/apache/arrow/pull/35034#discussion_r1166419671
##########
java/dataset/src/main/java/org/apache/arrow/dataset/file/JniWrapper.java:
##########
@@ -45,6 +45,17 @@ private JniWrapper() {
*/
public native long makeFileSystemDatasetFactory(String uri, int fileFormat);
+ /**
+ * Create FileSystemDatasetFactory and return its native pointer. The pointer is pointing to a
+ * intermediate shared_ptr of the factory instance.
+ *
+ * @param uris List of file uris to read, each path pointing to an individual file
Review Comment:
Aha, thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] davisusanibar commented on a diff in pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #35034:
URL: https://github.com/apache/arrow/pull/35034#discussion_r1163376784
##########
java/dataset/src/main/java/org/apache/arrow/dataset/file/JniWrapper.java:
##########
@@ -45,6 +45,17 @@ private JniWrapper() {
*/
public native long makeFileSystemDatasetFactory(String uri, int fileFormat);
+ /**
+ * Create FileSystemDatasetFactory and return its native pointer. The pointer is pointing to a
+ * intermediate shared_ptr of the factory instance.
+ *
+ * @param uris List of file uris to read, each path pointing to an individual file
Review Comment:
Is it working only for specific individual file or it is also working for a directory path that contains many individual files?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] NoahFournier commented on pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "NoahFournier (via GitHub)" <gi...@apache.org>.
NoahFournier commented on PR #35034:
URL: https://github.com/apache/arrow/pull/35034#issuecomment-1573582792
Sorry for the delay on this one. I've added the test as requested, and cleaned up the implementation using #34420
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #35034:
URL: https://github.com/apache/arrow/pull/35034#issuecomment-1503151393
@davisusanibar
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] davisusanibar commented on a diff in pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #35034:
URL: https://github.com/apache/arrow/pull/35034#discussion_r1163375000
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -533,6 +535,59 @@ Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory(
JNI_METHOD_END(-1L)
}
+/*
+ * Class: org_apache_arrow_dataset_file_JniWrapper
+ * Method: makeFileSystemDatasetFactory
+ * Signature: ([Ljava/lang/String;II)J
+ */
+JNIEXPORT jlong JNICALL
+Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory___3Ljava_lang_String_2I(
+ JNIEnv* env, jobject, jobjectArray uris, jint file_format_id) {
+ JNI_METHOD_START
+
+ using FsPathPair = std::pair<std::shared_ptr<arrow::fs::FileSystem>, std::string>;
+
+ std::shared_ptr<arrow::dataset::FileFormat> file_format =
+ JniGetOrThrow(GetFileFormat(file_format_id));
+ arrow::dataset::FileSystemFactoryOptions options;
+
+ std::vector<std::string> uri_vec = ToStringVector(env, uris);
+
+ // If not all URIs, throw exception
+ if (!std::all_of(uri_vec.begin(), uri_vec.end(), arrow::fs::internal::IsLikelyUri)) {
+ JniThrow("All sources must be valid URIs.");
+ }
+
+ std::vector<FsPathPair> filesystems;
+ filesystems.reserve(uri_vec.size());
+ std::transform(uri_vec.begin(), uri_vec.end(), std::back_inserter(filesystems),
+ [](const auto& s) -> FsPathPair {
+ std::string output_path;
+ auto fs = JniGetOrThrow(arrow::fs::FileSystemFromUri(s, &output_path));
+ return {fs, output_path};
+ });
+
+ // If all URIs, ensure that they all share a FileSystem type
+ if (std::unique(filesystems.begin(), filesystems.end(),
+ [] (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.");
Review Comment:
Could be possible to add the message that contains detail about what different filesystem the user are trying to use?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] NoahFournier commented on a diff in pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "NoahFournier (via GitHub)" <gi...@apache.org>.
NoahFournier commented on code in PR #35034:
URL: https://github.com/apache/arrow/pull/35034#discussion_r1163892759
##########
java/dataset/src/main/java/org/apache/arrow/dataset/file/JniWrapper.java:
##########
@@ -45,6 +45,17 @@ private JniWrapper() {
*/
public native long makeFileSystemDatasetFactory(String uri, int fileFormat);
+ /**
+ * Create FileSystemDatasetFactory and return its native pointer. The pointer is pointing to a
+ * intermediate shared_ptr of the factory instance.
+ *
+ * @param uris List of file uris to read, each path pointing to an individual file
Review Comment:
It will only work for specific individual files, and confirmed this by testing locally. I believe this matches the behaviour of the arrow C++ libraries. We would need to use a selector in order to crawl a directory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] davisusanibar commented on a diff in pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #35034:
URL: https://github.com/apache/arrow/pull/35034#discussion_r1163378277
##########
java/dataset/src/test/java/org/apache/arrow/dataset/file/TestFileSystemDataset.java:
##########
@@ -101,6 +101,29 @@ public void testBaseParquetRead() throws Exception {
AutoCloseables.close(factory);
}
+ @Test
+ public void testMultipleParquetReadFromUris() throws Exception {
+ ParquetWriteSupport writeSupport1 = ParquetWriteSupport.writeTempFile(AVRO_SCHEMA_USER, TMP.newFolder(),
+ 1, "a");
+ ParquetWriteSupport writeSupport2 = ParquetWriteSupport.writeTempFile(AVRO_SCHEMA_USER, TMP.newFolder(),
+ 2, "b");
+ String expectedJsonUnordered = "[[1,\"a\"],[2,\"b\"]]";
+
+ ScanOptions options = new ScanOptions(1);
+ FileSystemDatasetFactory factory = new FileSystemDatasetFactory(rootAllocator(), NativeMemoryPool.getDefault(),
+ FileFormat.PARQUET, new String[]{writeSupport1.getOutputURI(), writeSupport2.getOutputURI()});
+ Schema schema = inferResultSchemaFromFactory(factory, options);
+ List<ArrowRecordBatch> datum = collectResultFromFactory(factory, options);
+
+ assertScanBatchesProduced(factory, options);
+ assertEquals(2, datum.size());
+ datum.forEach(batch -> assertEquals(1, batch.getLength()));
+ checkParquetReadResult(schema, expectedJsonUnordered, datum);
+
+ AutoCloseables.close(datum);
+
+ }
Review Comment:
Could be possible to add testing for exception throws: (1) Invalid URI , (2) ensure that they all share a FileSystem type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] NoahFournier commented on a diff in pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "NoahFournier (via GitHub)" <gi...@apache.org>.
NoahFournier commented on code in PR #35034:
URL: https://github.com/apache/arrow/pull/35034#discussion_r1165193690
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -533,6 +535,59 @@ Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory(
JNI_METHOD_END(-1L)
}
+/*
+ * Class: org_apache_arrow_dataset_file_JniWrapper
+ * Method: makeFileSystemDatasetFactory
+ * Signature: ([Ljava/lang/String;II)J
+ */
+JNIEXPORT jlong JNICALL
+Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory___3Ljava_lang_String_2I(
+ JNIEnv* env, jobject, jobjectArray uris, jint file_format_id) {
+ JNI_METHOD_START
+
+ using FsPathPair = std::pair<std::shared_ptr<arrow::fs::FileSystem>, std::string>;
+
+ std::shared_ptr<arrow::dataset::FileFormat> file_format =
+ JniGetOrThrow(GetFileFormat(file_format_id));
+ arrow::dataset::FileSystemFactoryOptions options;
+
+ std::vector<std::string> uri_vec = ToStringVector(env, uris);
+
+ // If not all URIs, throw exception
+ if (!std::all_of(uri_vec.begin(), uri_vec.end(), arrow::fs::internal::IsLikelyUri)) {
+ JniThrow("All sources must be valid URIs.");
+ }
+
+ std::vector<FsPathPair> filesystems;
+ filesystems.reserve(uri_vec.size());
+ std::transform(uri_vec.begin(), uri_vec.end(), std::back_inserter(filesystems),
+ [](const auto& s) -> FsPathPair {
+ std::string output_path;
+ auto fs = JniGetOrThrow(arrow::fs::FileSystemFromUri(s, &output_path));
+ return {fs, output_path};
+ });
+
+ // If all URIs, ensure that they all share a FileSystem type
+ if (std::unique(filesystems.begin(), filesystems.end(),
+ [] (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.");
Review Comment:
Agreed on both counts, I'll wait for #34420 to clean this up a bit. and then create a better exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm merged pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #35034:
URL: https://github.com/apache/arrow/pull/35034
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35034: 35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35034:
URL: https://github.com/apache/arrow/pull/35034#issuecomment-1503075491
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). 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](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) 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:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] NoahFournier commented on a diff in pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "NoahFournier (via GitHub)" <gi...@apache.org>.
NoahFournier commented on code in PR #35034:
URL: https://github.com/apache/arrow/pull/35034#discussion_r1165197518
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -533,6 +535,60 @@ Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory(
JNI_METHOD_END(-1L)
}
+/*
+ * Class: org_apache_arrow_dataset_file_JniWrapper
+ * Method: makeFileSystemDatasetFactory
+ * Signature: ([Ljava/lang/String;II)J
+ */
+JNIEXPORT jlong JNICALL
+Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory___3Ljava_lang_String_2I(
+ JNIEnv* env, jobject, jobjectArray uris, jint file_format_id) {
+ JNI_METHOD_START
+
+ using FsPathPair = std::pair<std::shared_ptr<arrow::fs::FileSystem>, std::string>;
+
+ std::shared_ptr<arrow::dataset::FileFormat> file_format =
+ JniGetOrThrow(GetFileFormat(file_format_id));
+ arrow::dataset::FileSystemFactoryOptions options;
+
+ std::vector<std::string> uri_vec = ToStringVector(env, uris);
+
+ // If not all URIs, throw exception
Review Comment:
Agreed, nice catch !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35034:
URL: https://github.com/apache/arrow/pull/35034#issuecomment-1503079925
* Closes: #35033
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] davisusanibar commented on a diff in pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #35034:
URL: https://github.com/apache/arrow/pull/35034#discussion_r1163371093
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -533,6 +535,59 @@ Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory(
JNI_METHOD_END(-1L)
}
+/*
+ * Class: org_apache_arrow_dataset_file_JniWrapper
+ * Method: makeFileSystemDatasetFactory
+ * Signature: ([Ljava/lang/String;II)J
+ */
+JNIEXPORT jlong JNICALL
+Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory___3Ljava_lang_String_2I(
+ JNIEnv* env, jobject, jobjectArray uris, jint file_format_id) {
+ JNI_METHOD_START
+
+ using FsPathPair = std::pair<std::shared_ptr<arrow::fs::FileSystem>, std::string>;
+
+ std::shared_ptr<arrow::dataset::FileFormat> file_format =
+ JniGetOrThrow(GetFileFormat(file_format_id));
+ arrow::dataset::FileSystemFactoryOptions options;
+
+ std::vector<std::string> uri_vec = ToStringVector(env, uris);
+
+ // If not all URIs, throw exception
+ if (!std::all_of(uri_vec.begin(), uri_vec.end(), arrow::fs::internal::IsLikelyUri)) {
+ JniThrow("All sources must be valid URIs.");
+ }
Review Comment:
What about to only delegate the validation to `arrow::fs::internal::IsLikelyUri`, they could response with the invalid URI error message `java.lang.RuntimeException: Unrecognized filesystem type in URI: http://example.com`
```suggestion
std::all_of(uri_vec.begin(), uri_vec.end(), arrow::fs::internal::IsLikelyUri);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #35034:
URL: https://github.com/apache/arrow/pull/35034#discussion_r1163983752
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -533,6 +535,60 @@ Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory(
JNI_METHOD_END(-1L)
}
+/*
+ * Class: org_apache_arrow_dataset_file_JniWrapper
+ * Method: makeFileSystemDatasetFactory
+ * Signature: ([Ljava/lang/String;II)J
+ */
+JNIEXPORT jlong JNICALL
+Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory___3Ljava_lang_String_2I(
+ JNIEnv* env, jobject, jobjectArray uris, jint file_format_id) {
+ JNI_METHOD_START
+
+ using FsPathPair = std::pair<std::shared_ptr<arrow::fs::FileSystem>, std::string>;
+
+ std::shared_ptr<arrow::dataset::FileFormat> file_format =
+ JniGetOrThrow(GetFileFormat(file_format_id));
+ arrow::dataset::FileSystemFactoryOptions options;
+
+ std::vector<std::string> uri_vec = ToStringVector(env, uris);
+
+ // If not all URIs, throw exception
Review Comment:
We should also be checking if no URIs were passed
##########
java/dataset/src/main/java/org/apache/arrow/dataset/file/JniWrapper.java:
##########
@@ -45,6 +45,17 @@ private JniWrapper() {
*/
public native long makeFileSystemDatasetFactory(String uri, int fileFormat);
+ /**
+ * Create FileSystemDatasetFactory and return its native pointer. The pointer is pointing to a
+ * intermediate shared_ptr of the factory instance.
+ *
+ * @param uris List of file uris to read, each path pointing to an individual file
Review Comment:
Is there any purpose in keeping the `String, int` overload if we have `String[], int` which should be strictly more general?
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -533,6 +535,59 @@ Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory(
JNI_METHOD_END(-1L)
}
+/*
+ * Class: org_apache_arrow_dataset_file_JniWrapper
+ * Method: makeFileSystemDatasetFactory
+ * Signature: ([Ljava/lang/String;II)J
+ */
+JNIEXPORT jlong JNICALL
+Java_org_apache_arrow_dataset_file_JniWrapper_makeFileSystemDatasetFactory___3Ljava_lang_String_2I(
+ JNIEnv* env, jobject, jobjectArray uris, jint file_format_id) {
+ JNI_METHOD_START
+
+ using FsPathPair = std::pair<std::shared_ptr<arrow::fs::FileSystem>, std::string>;
+
+ std::shared_ptr<arrow::dataset::FileFormat> file_format =
+ JniGetOrThrow(GetFileFormat(file_format_id));
+ arrow::dataset::FileSystemFactoryOptions options;
+
+ std::vector<std::string> uri_vec = ToStringVector(env, uris);
+
+ // If not all URIs, throw exception
+ if (!std::all_of(uri_vec.begin(), uri_vec.end(), arrow::fs::internal::IsLikelyUri)) {
+ JniThrow("All sources must be valid URIs.");
+ }
+
+ std::vector<FsPathPair> filesystems;
+ filesystems.reserve(uri_vec.size());
+ std::transform(uri_vec.begin(), uri_vec.end(), std::back_inserter(filesystems),
+ [](const auto& s) -> FsPathPair {
+ std::string output_path;
+ auto fs = JniGetOrThrow(arrow::fs::FileSystemFromUri(s, &output_path));
+ return {fs, output_path};
+ });
+
+ // If all URIs, ensure that they all share a FileSystem type
+ if (std::unique(filesystems.begin(), filesystems.end(),
+ [] (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.");
Review Comment:
We should be using https://github.com/apache/arrow/pull/34420
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] NoahFournier commented on a diff in pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "NoahFournier (via GitHub)" <gi...@apache.org>.
NoahFournier commented on code in PR #35034:
URL: https://github.com/apache/arrow/pull/35034#discussion_r1165196008
##########
java/dataset/src/main/java/org/apache/arrow/dataset/file/JniWrapper.java:
##########
@@ -45,6 +45,17 @@ private JniWrapper() {
*/
public native long makeFileSystemDatasetFactory(String uri, int fileFormat);
+ /**
+ * Create FileSystemDatasetFactory and return its native pointer. The pointer is pointing to a
+ * intermediate shared_ptr of the factory instance.
+ *
+ * @param uris List of file uris to read, each path pointing to an individual file
Review Comment:
The `String, int` overload correctly handles the user passing a directory, which the `String[], int` overload does not (Each `String` needs to be a complete uri to a source). I'll look into whether I could handle both directories and complete URIs in the `String[], int` implementation, in which case we wouldn't need `String, int`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #35034: GH-35033: [Java] [Datasets] Add support for multi-file datasets from Java
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35034:
URL: https://github.com/apache/arrow/pull/35034#issuecomment-1575180438
Benchmark runs are scheduled for baseline = 018e7d3f9c4bcacce716dd607994486a31ee71bb and contender = cd42895a0a8b6fa21be5bc06d13d771bef0d4fb3. cd42895a0a8b6fa21be5bc06d13d771bef0d4fb3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/c55f25d0633943619cc10858e299e6bb...fc14070dcb2645679a85f421f48fa032/)
[Finished :arrow_down:0.62% :arrow_up:0.09%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/3725d6d47fd14ebab787cdb21d184e1a...1d8a29a5c2a1432b9df2f060b7bffe54/)
[Failed] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/e6953777bb48473ea97ab0c354582baf...dff44a41d0b6439285805a071c79a9a0/)
[Finished :arrow_down:0.6% :arrow_up:0.06%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/bfcf868aa0094e8e97d0f6b0ff3800e6...c94ed995a5b545039740aaa81a04cff0/)
Buildkite builds:
[Finished] [`cd42895a` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2973)
[Finished] [`cd42895a` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/3009)
[Failed] [`cd42895a` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2974)
[Finished] [`cd42895a` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2999)
[Finished] [`018e7d3f` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2972)
[Finished] [`018e7d3f` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/3008)
[Finished] [`018e7d3f` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2973)
[Finished] [`018e7d3f` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2998)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org