You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jinchengchenghh (via GitHub)" <gi...@apache.org> on 2024/05/14 08:15:54 UTC
[PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
jinchengchenghh opened a new pull request, #41646:
URL: https://github.com/apache/arrow/pull/41646
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
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}
-->
### Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Support to add ArrowSchema to specify C++ CsvFragmentScanOptions.convert_options.column_types
### Are these changes tested?
new added UT.
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
No.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2143123020
> @jinchengchenghh will re-review this later today.
@jinchengchenghh sorry about the unexpected delay. I have added a few comments today.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642308540
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
That shouldn't affect this, right? This is internal to the Dataset library. Also, proto2 and proto3 are supported by the same runtime and compiler as far as I am aware.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642298348
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
The downstream project Gluten uses proto2, just for the compatibility.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642521146
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
Then we should shade the protobuf-java to avoid version conflict, do you think we should do that?
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642064378
##########
java/dataset/pom.xml:
##########
@@ -25,6 +25,8 @@
<arrow.cpp.build.dir>../../../cpp/release-build/</arrow.cpp.build.dir>
<parquet.version>1.13.1</parquet.version>
<avro.version>1.11.3</avro.version>
+ <substrait.version>0.31.0</substrait.version>
+ <protobuf.version>3.25.3</protobuf.version>
Review Comment:
Hmm, I won't force it here. @vibhatha that would be an idea for future cleanup though - try to make sure we use dependencyManagement and BOM (where available) for all dependencies instead of declaring dependencies ad-hoc inside each individual module.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2261768563
I have created a PR: https://github.com/apache/arrow/pull/43503
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2255053127
@github-actions crossbow submit java-jars
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1609160640
##########
cpp/src/arrow/dataset/file_csv.cc:
##########
@@ -52,6 +53,9 @@ using internal::Executor;
using internal::SerialExecutor;
namespace dataset {
+namespace {
+inline bool parseBool(const std::string& value) { return value == "true" ? true : false; }
Review Comment:
note: please follow the naming convention (UpperCamelCase functions/classes, snake_case variable names)
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #41646:
URL: https://github.com/apache/arrow/pull/41646
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2260796232
Sure I can.
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2109560455
:warning: GitHub issue #28866 **has been automatically assigned in GitHub** to PR creator.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1643592181
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
I have shaded protobuf-java, if their protobuf version conflicts, they can use
```
<groupId>org.apache.arrow</groupId>
<artifactId>arrow-dataset</artifactId>
<version>${arrow-gluten.version}</version>
<classifier>shaded</classifier>
```
All the issues have been solved.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642055941
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -153,6 +154,19 @@ arrow::Result<std::shared_ptr<arrow::dataset::FileFormat>> GetFileFormat(
}
}
+arrow::Result<std::shared_ptr<arrow::dataset::FragmentScanOptions>>
+GetFragmentScanOptions(jint file_format_id,
+ const std::unordered_map<std::string, std::string>& configs) {
+ switch (file_format_id) {
+#ifdef ARROW_CSV
+ case 3:
Review Comment:
No, It is only used here, https://github.com/apache/arrow/blob/main/java/dataset/src/main/cpp/jni_wrapper.cc#L133
And it is enum class `FileFormat` mapping to Java https://github.com/apache/arrow/blob/main/java/dataset/src/main/cpp/jni_wrapper.cc#L133
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642107792
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
I try to move it, but meet this exception, can you give me some clue?
```
[ 83%] Building CXX object dataset/CMakeFiles/arrow_java_jni_dataset.dir/src/main/cpp/jni_util.cc.o
/mnt/DP_disk1/code/arrow/java/dataset/src/main/cpp/jni_wrapper.cc:42:10: fatal error: substrait/algebra.pb.h: No such file or directory
42 | #include "substrait/algebra.pb.h" // IWYU pragma: export
| ^~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
```
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2114776299
CC @vibhatha
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1605736744
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/FragmentScanOptions.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.dataset.scanner;
+
+import java.nio.ByteBuffer;
+import java.util.Map;
+
+import org.apache.arrow.dataset.substrait.StringMapNode;
+
+import com.google.protobuf.Any;
+
+import io.substrait.proto.AdvancedExtension;
+
+public interface FragmentScanOptions {
+ String typeName();
+
+ int fileFormatId();
+
+ ByteBuffer serialize();
+
+ /**
+ * serialize the map.
+ *
+ * @param config config map
+ * @return bufer to jni call argument, should be DirectByteBuffer
+ */
+ default ByteBuffer serializeMap(Map<String, String> config) {
Review Comment:
Yes
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2144158895
@jinchengchenghh shall we rebase and see if we can run the CIs?
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1643597360
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
That's an optional alternative artifact, and one that we may not want to keep around as it causes problems for distribution.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2244398709
Thanks for your kindly review. @vibhatha
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1643608747
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
Standard Protobuf dependency + Protobuf well-known-type, or don't use Protobuf at all and find some other serialization method that doesn't carry this heavy of a dependency. (Some parts of the JNI code just directly pass a Java Map reference and use JNI to extract the key/value pairs on the C++ side, for instance.)
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2255167532
Revision: 98a3beb7da91166fc362f7c32b984049f362e581
Submitted crossbow builds: [ursacomputing/crossbow @ actions-f053a0e3dd](https://github.com/ursacomputing/crossbow/branches/all?query=actions-f053a0e3dd)
|Task|Status|
|----|------|
|java-jars|[](https://github.com/ursacomputing/crossbow/actions/runs/10139976155/job/28034259023)|
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2255304351
Sigh, well, another update broke that pipeline again..
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1623038748
##########
java/dataset/src/main/java/org/apache/arrow/dataset/file/JniWrapper.java:
##########
@@ -43,7 +45,8 @@ private JniWrapper() {
* @return the native pointer of the arrow::dataset::FileSystemDatasetFactory instance.
* @see FileFormat
*/
- public native long makeFileSystemDatasetFactory(String uri, int fileFormat);
+ public native long makeFileSystemDatasetFactory(String uri, int fileFormat,
Review Comment:
Update Java docs?
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1623708687
##########
java/dataset/src/test/java/org/apache/arrow/dataset/substrait/TestAceroSubstraitConsumer.java:
##########
@@ -457,4 +465,42 @@ private static ByteBuffer getByteBuffer(String base64EncodedSubstrait) {
substraitExpression.put(decodedSubstrait);
return substraitExpression;
}
+
+ @Test
+ public void testCsvConvertOptions() throws Exception {
+ final Schema schema = new Schema(Arrays.asList(
+ Field.nullable("Id", new ArrowType.Int(32, true)),
+ Field.nullable("Name", new ArrowType.Utf8()),
+ Field.nullable("Language", new ArrowType.Utf8())
+ ), null);
+ String path = "file://" + getClass().getResource("/").getPath() + "/data/student.csv";
+ BufferAllocator allocator = rootAllocator();
+ try (ArrowSchema cSchema = ArrowSchema.allocateNew(allocator);
+ CDataDictionaryProvider provider = new CDataDictionaryProvider()) {
+ Data.exportSchema(allocator, schema, provider, cSchema);
+ CsvConvertOptions convertOptions = new CsvConvertOptions(ImmutableMap.of("delimiter", ";"));
+ convertOptions.setArrowSchema(cSchema);
+ CsvFragmentScanOptions fragmentScanOptions = new CsvFragmentScanOptions(
+ convertOptions, ImmutableMap.of(), ImmutableMap.of());
+ ScanOptions options = new ScanOptions.Builder(/*batchSize*/ 32768)
+ .columns(Optional.empty())
+ .fragmentScanOptions(fragmentScanOptions)
+ .build();
+ try (
+ DatasetFactory datasetFactory = new FileSystemDatasetFactory(allocator, NativeMemoryPool.getDefault(),
+ FileFormat.CSV, path);
+ Dataset dataset = datasetFactory.finish();
+ Scanner scanner = dataset.newScan(options);
+ ArrowReader reader = scanner.scanBatches()
+ ) {
+ assertEquals(schema.getFields(), reader.getVectorSchemaRoot().getSchema().getFields());
+ int rowCount = 0;
+ while (reader.loadNextBatch()) {
+ assertEquals("[1, 2, 3]", reader.getVectorSchemaRoot().getVector("Id").toString());
Review Comment:
No. the `Id` column is enough, the test tests delimiter config and schema of `Id` change from default int64 to int32.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642107792
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
I try to move it, but meet this exception, can you give me some clue?
```
[ 83%] Building CXX object dataset/CMakeFiles/arrow_java_jni_dataset.dir/src/main/cpp/jni_util.cc.o
/mnt/DP_disk1/code/arrow/java/dataset/src/main/cpp/jni_wrapper.cc:42:10: fatal error: substrait/algebra.pb.h: No such file or directory
42 | #include "substrait/algebra.pb.h" // IWYU pragma: export
| ^~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
```
This file already generated at `/mnt/DP_disk1/code/arrow# find . -name "algebra.pb.h"
./cpp-jni/substrait_ep-generated/substrait/algebra.pb.h`
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1605604614
##########
cpp/src/arrow/dataset/file_csv.cc:
##########
@@ -503,5 +507,31 @@ Future<> CsvFileWriter::FinishInternal() {
return Status::OK();
}
+Result<std::shared_ptr<FragmentScanOptions>> CsvFragmentScanOptions::from(
+ const std::unordered_map<std::string, std::string>& configs) {
+ std::shared_ptr<CsvFragmentScanOptions> options =
+ std::make_shared<CsvFragmentScanOptions>();
+ for (auto const& it : configs) {
+ auto& key = it.first;
+ auto& value = it.second;
+ if (key == "delimiter") {
+ options->parse_options.delimiter = value.data()[0];
+ } else if (key == "quoting") {
+ options->parse_options.quoting = parseBool(value);
+ } else if (key == "column_type") {
+ int64_t schema_address = std::stol(value);
Review Comment:
should we check for possible `-1` ?
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1605609153
##########
java/dataset/src/main/java/org/apache/arrow/dataset/substrait/StringMapNode.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.dataset.substrait;
+
+import java.io.Serializable;
+import java.util.HashMap;
+import java.util.Map;
+
+import io.substrait.proto.Expression;
+
+public class StringMapNode implements Serializable {
Review Comment:
I also have doubts about having a separate class for this purpose though.
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1609208008
##########
cpp/src/arrow/dataset/file_csv.cc:
##########
@@ -503,5 +507,31 @@ Future<> CsvFileWriter::FinishInternal() {
return Status::OK();
}
+Result<std::shared_ptr<FragmentScanOptions>> CsvFragmentScanOptions::from(
+ const std::unordered_map<std::string, std::string>& configs) {
+ std::shared_ptr<CsvFragmentScanOptions> options =
+ std::make_shared<CsvFragmentScanOptions>();
+ for (auto const& it : configs) {
+ auto& key = it.first;
+ auto& value = it.second;
+ if (key == "delimiter") {
+ options->parse_options.delimiter = value.data()[0];
+ } else if (key == "quoting") {
+ options->parse_options.quoting = parseBool(value);
+ } else if (key == "column_type") {
+ int64_t schema_address = std::stol(value);
Review Comment:
I changed it in Java side to not add invalid schema address
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2144752269
> @vibhatha #41903
>
> In general, you can search existing issue when you find a CI failure that may be unrelated to a PR change. If you don't find an existing issue, you can open a new issue for it.
Thanks @kou will do that.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1624376446
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -153,6 +154,19 @@ arrow::Result<std::shared_ptr<arrow::dataset::FileFormat>> GetFileFormat(
}
}
+arrow::Result<std::shared_ptr<arrow::dataset::FragmentScanOptions>>
+GetFragmentScanOptions(jint file_format_id,
+ const std::unordered_map<std::string, std::string>& configs) {
+ switch (file_format_id) {
+#ifdef ARROW_CSV
+ case 3:
Review Comment:
Is there some enum/constant that defines the possible values?
##########
cpp/src/arrow/dataset/file_csv.h:
##########
@@ -85,6 +85,10 @@ class ARROW_DS_EXPORT CsvFileFormat : public FileFormat {
struct ARROW_DS_EXPORT CsvFragmentScanOptions : public FragmentScanOptions {
std::string type_name() const override { return kCsvTypeName; }
+ /// \brief Construct FragmentScanOptions from config map
Review Comment:
Ideally we'd document the possible options? But also, this also feels like something that should be inside the JNI bindings specifically and not the public API.
##########
java/dataset/pom.xml:
##########
@@ -25,6 +25,8 @@
<arrow.cpp.build.dir>../../../cpp/release-build/</arrow.cpp.build.dir>
<parquet.version>1.13.1</parquet.version>
<avro.version>1.11.3</avro.version>
+ <substrait.version>0.31.0</substrait.version>
+ <protobuf.version>3.25.3</protobuf.version>
Review Comment:
Is this not already defined by dependencyManagement elsewhere?
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
This all feels very specific to the internals of the JNI bindings. I don't think this or the new FromProto should be in the public API or the Arrow libraries, ideally, but in the JNI bindings instead.
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2138480890
Do you have further comments? @vibhatha @lidavidm
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2144746000
@vibhatha #41903
In general, you can search existing issue when you find a CI failure that may be unrelated to a PR change. If you don't find an existing issue, you can open a new issue for it.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642109487
##########
cpp/src/arrow/dataset/file_csv.h:
##########
@@ -85,6 +85,10 @@ class ARROW_DS_EXPORT CsvFileFormat : public FileFormat {
struct ARROW_DS_EXPORT CsvFragmentScanOptions : public FragmentScanOptions {
std::string type_name() const override { return kCsvTypeName; }
+ /// \brief Construct FragmentScanOptions from config map
Review Comment:
Which document would you suggest to add the option description?
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2261748599
Thanks @jinchengchenghh
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2230262618
Do you have further comments? @vibhatha
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2144488758
I suppose not, the newly added commit https://github.com/apache/arrow/pull/41646/commits/1360221ea50b9a02c53c85eb5b302f363e385301 only involves comments.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1643609997
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
OK, I will use string[] to transfer
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642057478
##########
java/dataset/pom.xml:
##########
@@ -25,6 +25,8 @@
<arrow.cpp.build.dir>../../../cpp/release-build/</arrow.cpp.build.dir>
<parquet.version>1.13.1</parquet.version>
<avro.version>1.11.3</avro.version>
+ <substrait.version>0.31.0</substrait.version>
+ <protobuf.version>3.25.3</protobuf.version>
Review Comment:
No, this is only defined in gandiva like this format, not exists in dependencyManagement, I could move it to dependencyManagement if you think we should do that.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642111281
##########
cpp/src/arrow/dataset/file_csv.h:
##########
@@ -85,6 +85,10 @@ class ARROW_DS_EXPORT CsvFileFormat : public FileFormat {
struct ARROW_DS_EXPORT CsvFragmentScanOptions : public FragmentScanOptions {
std::string type_name() const override { return kCsvTypeName; }
+ /// \brief Construct FragmentScanOptions from config map
Review Comment:
I think it's best to put this inside the JNI bindings specifically. It's not really a public API, is it?
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2244370388
@jinchengchenghh I don't have any special comments at the moment. Overall looks fine to me.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2255056451
Revision: 04e4390be2bb640c6b8579627b1990b277d98fcf
Submitted crossbow builds: [ursacomputing/crossbow @ actions-4da86e64a2](https://github.com/ursacomputing/crossbow/branches/all?query=actions-4da86e64a2)
|Task|Status|
|----|------|
|java-jars|[](https://github.com/ursacomputing/crossbow/actions/runs/10139341213/job/28032480765)|
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2255477315
> Sigh, well, another update broke that pipeline again..
Hmm, seems like that. I will take a look.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2258643317
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit fd69e5ef9f2d60e3b7bc7fc80208e04994834b9e.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/28113034006) has more details. It also includes information about 24 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1605608836
##########
java/dataset/src/main/java/org/apache/arrow/dataset/substrait/StringMapNode.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.dataset.substrait;
+
+import java.io.Serializable;
+import java.util.HashMap;
+import java.util.Map;
+
+import io.substrait.proto.Expression;
+
+public class StringMapNode implements Serializable {
Review Comment:
Just looking at the functionality, I think what we have here is a util class which converts a particular map config to a particular Substrait protobuf message. Since this can be used in other cases, it could come under `substrait.util` package. And the `toProtobuf` could be `mapToExpressionLiteral()` ?
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1602895634
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/csv/CsvFragmentScanOptions.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.dataset.scanner.csv;
+
+import java.io.Serializable;
+import java.nio.ByteBuffer;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.apache.arrow.dataset.file.FileFormat;
+import org.apache.arrow.dataset.scanner.FragmentScanOptions;
+
+public class CsvFragmentScanOptions implements Serializable, FragmentScanOptions {
+ private final CsvConvertOptions convertOptions;
+ private final Map<String, String> readOptions;
+ private final Map<String, String> parseOptions;
+
+
+ /**
+ * csv scan options, map to CPP struct CsvFragmentScanOptions.
+ *
+ * @param convertOptions same struct in CPP
+ * @param readOptions same struct in CPP
+ * @param parseOptions same struct in CPP
+ */
+ public CsvFragmentScanOptions(CsvConvertOptions convertOptions,
+ Map<String, String> readOptions,
+ Map<String, String> parseOptions) {
+ this.convertOptions = convertOptions;
+ this.readOptions = readOptions;
+ this.parseOptions = parseOptions;
+ }
+
+ public String typeName() {
+ return FileFormat.CSV.name().toLowerCase(Locale.ROOT);
+ }
+
+ /**
+ * File format id.
+ *
+ * @return id
+ */
+ public int fileFormatId() {
+ return FileFormat.CSV.id();
+ }
+
+ /**
+ * Serialize this class to ByteBuffer and then called by jni call.
+ *
+ * @return DirectByteBuffer
+ */
+ public ByteBuffer serialize() {
+ Map<String, String> options = Stream.concat(Stream.concat(readOptions.entrySet().stream(),
+ parseOptions.entrySet().stream()),
Review Comment:
Insert all the options to a map because it is a easy implement, and now we don't have same option name in CPP parse_options and read_options, but to further extend, we may need to serialize more accurately. I'm open to here if you think we should serialize each option
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2114531692
Can you help review this PR? If the framework is OK, I will add more common config in this PR. Thanks! @westonpace
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2138497557
@jinchengchenghh will re-review this later today.
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1623039586
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/FragmentScanOptions.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.dataset.scanner;
+
+import java.nio.ByteBuffer;
+import java.util.Map;
+
+import org.apache.arrow.dataset.substrait.util.ConvertUtil;
+
+import io.substrait.proto.Expression;
+
+public interface FragmentScanOptions {
+ String typeName();
+
+ int fileFormatId();
+
+ ByteBuffer serialize();
+
+ /**
+ * serialize the map.
Review Comment:
nit:
```suggestion
* Serialize the map.
```
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2261745160
I create a issue https://github.com/apache/arrow/issues/43502 @vibhatha
CC @lidavidm
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1687533928
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/MapUtil.java:
##########
Review Comment:
Ideally this would be in a separate namespace that we do not export in module-info.java
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/csv/CsvFragmentScanOptions.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.
+ */
+package org.apache.arrow.dataset.scanner.csv;
+
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.arrow.dataset.file.FileFormat;
+import org.apache.arrow.dataset.scanner.FragmentScanOptions;
+import org.apache.arrow.dataset.scanner.MapUtil;
+
+public class CsvFragmentScanOptions implements FragmentScanOptions {
+ private final CsvConvertOptions convertOptions;
+ private final Map<String, String> readOptions;
+ private final Map<String, String> parseOptions;
+
+ /**
+ * CSV scan options, map to CPP struct CsvFragmentScanOptions. The key in config map is the field
+ * name of mapping cpp struct
+ *
+ * @param convertOptions similar to CsvFragmentScanOptions#convert_options in CPP, the ArrowSchema
+ * represents column_types, convert data option such as null value recognition.
+ * @param readOptions similar to CsvFragmentScanOptions#read_options in CPP, specify how to read
+ * the file such as block_size
+ * @param parseOptions similar to CsvFragmentScanOptions#parse_options in CPP, parse file option
+ * such as delimiter
+ */
+ public CsvFragmentScanOptions(
+ CsvConvertOptions convertOptions,
+ Map<String, String> readOptions,
+ Map<String, String> parseOptions) {
+ this.convertOptions = convertOptions;
+ this.readOptions = readOptions;
+ this.parseOptions = parseOptions;
+ }
+
+ /**
+ * File format id.
Review Comment:
docstring is out of date
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -363,6 +366,65 @@ std::shared_ptr<arrow::Buffer> LoadArrowBufferFromByteBuffer(JNIEnv* env, jobjec
return buffer;
}
+inline bool ParseBool(const std::string& value) { return value == "true" ? true : false; }
+
+/// \brief Construct FragmentScanOptions from config map
+#ifdef ARROW_CSV
+arrow::Result<std::shared_ptr<arrow::dataset::FragmentScanOptions>>
+ToCsvFragmentScanOptions(const std::unordered_map<std::string, std::string>& configs) {
+ std::shared_ptr<arrow::dataset::CsvFragmentScanOptions> options =
+ std::make_shared<arrow::dataset::CsvFragmentScanOptions>();
+ for (auto const& it : configs) {
+ auto& key = it.first;
+ auto& value = it.second;
Review Comment:
```suggestion
for (const auto& [key, value] : configs) {
```
##########
java/dataset/src/test/java/org/apache/arrow/dataset/TestFragmentScanOptions.java:
##########
Review Comment:
Can we test that it fails without the options set?
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/MapUtil.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.
+ */
+package org.apache.arrow.dataset.scanner;
+
+import java.util.Map;
+
+/** The utility class for Map. */
+public class MapUtil {
Review Comment:
Util classes should be final and have a private constructor
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/csv/CsvFragmentScanOptions.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.
+ */
+package org.apache.arrow.dataset.scanner.csv;
+
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.arrow.dataset.file.FileFormat;
+import org.apache.arrow.dataset.scanner.FragmentScanOptions;
+import org.apache.arrow.dataset.scanner.MapUtil;
+
+public class CsvFragmentScanOptions implements FragmentScanOptions {
+ private final CsvConvertOptions convertOptions;
+ private final Map<String, String> readOptions;
+ private final Map<String, String> parseOptions;
+
+ /**
+ * CSV scan options, map to CPP struct CsvFragmentScanOptions. The key in config map is the field
+ * name of mapping cpp struct
+ *
+ * @param convertOptions similar to CsvFragmentScanOptions#convert_options in CPP, the ArrowSchema
+ * represents column_types, convert data option such as null value recognition.
+ * @param readOptions similar to CsvFragmentScanOptions#read_options in CPP, specify how to read
+ * the file such as block_size
+ * @param parseOptions similar to CsvFragmentScanOptions#parse_options in CPP, parse file option
+ * such as delimiter
+ */
+ public CsvFragmentScanOptions(
+ CsvConvertOptions convertOptions,
+ Map<String, String> readOptions,
+ Map<String, String> parseOptions) {
+ this.convertOptions = convertOptions;
+ this.readOptions = readOptions;
+ this.parseOptions = parseOptions;
+ }
+
+ /**
+ * File format id.
+ *
+ * @return id
+ */
+ @Override
+ public FileFormat fileFormat() {
+ return FileFormat.CSV;
+ }
+
+ /**
+ * Serialize this class to string array and then called by JNI call.
+ *
+ * @return string array as Map JNI bridge format.
+ */
Review Comment:
Can we make it clearer that this is not part of the public API?
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2255137113
Sorry, do you mind rebasing again so we can validate the JNI job?
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642055941
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -153,6 +154,19 @@ arrow::Result<std::shared_ptr<arrow::dataset::FileFormat>> GetFileFormat(
}
}
+arrow::Result<std::shared_ptr<arrow::dataset::FragmentScanOptions>>
+GetFragmentScanOptions(jint file_format_id,
+ const std::unordered_map<std::string, std::string>& configs) {
+ switch (file_format_id) {
+#ifdef ARROW_CSV
+ case 3:
Review Comment:
No
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1645283246
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
Can't we just pass the Java Map and reconstruct that using JNI utils in C++?
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -363,6 +364,64 @@ std::shared_ptr<arrow::Buffer> LoadArrowBufferFromByteBuffer(JNIEnv* env, jobjec
return buffer;
}
+inline bool ParseBool(const std::string& value) { return value == "true" ? true : false; }
+
+/// \brief Construct FragmentScanOptions from config map
+#ifdef ARROW_CSV
+arrow::Result<std::shared_ptr<arrow::dataset::FragmentScanOptions>>
+ToCsvFragmentScanOptions(const std::unordered_map<std::string, std::string>& configs) {
+ std::shared_ptr<arrow::dataset::CsvFragmentScanOptions> options =
+ std::make_shared<arrow::dataset::CsvFragmentScanOptions>();
+ for (auto const& it : configs) {
+ auto& key = it.first;
+ auto& value = it.second;
+ if (key == "delimiter") {
+ options->parse_options.delimiter = value.data()[0];
+ } else if (key == "quoting") {
+ options->parse_options.quoting = ParseBool(value);
+ } else if (key == "column_types") {
+ int64_t schema_address = std::stol(value);
+ ArrowSchema* cSchema = reinterpret_cast<ArrowSchema*>(schema_address);
+ ARROW_ASSIGN_OR_RAISE(auto schema, arrow::ImportSchema(cSchema));
+ auto& column_types = options->convert_options.column_types;
+ for (auto field : schema->fields()) {
+ column_types[field->name()] = field->type();
+ }
+ } else if (key == "strings_can_be_null") {
+ options->convert_options.strings_can_be_null = ParseBool(value);
+ } else {
+ return arrow::Status::Invalid("Config " + it.first + " is not supported.");
+ }
+ }
+ return options;
+}
+#endif
+
+arrow::Result<std::shared_ptr<arrow::dataset::FragmentScanOptions>>
+GetFragmentScanOptions(jint file_format_id,
+ const std::unordered_map<std::string, std::string>& configs) {
+ switch (file_format_id) {
+#ifdef ARROW_CSV
+ case 3:
+ return ToCsvFragmentScanOptions(configs);
+#endif
+ default:
+ return arrow::Status::Invalid("Illegal file format id: ", file_format_id);
+ }
+}
+
+std::unordered_map<std::string, std::string> ToStringMap(JNIEnv* env,
Review Comment:
Maybe we can just take the Java Map and create the C++ Map using JNI, rather than using String[]?
cc @lidavidm
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/csv/CsvFragmentScanOptions.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.
+ */
+package org.apache.arrow.dataset.scanner.csv;
+
+import java.io.Serializable;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.arrow.dataset.file.FileFormat;
+import org.apache.arrow.dataset.scanner.FragmentScanOptions;
+
+public class CsvFragmentScanOptions implements Serializable, FragmentScanOptions {
+ private final CsvConvertOptions convertOptions;
+ private final Map<String, String> readOptions;
+ private final Map<String, String> parseOptions;
+
+ /**
+ * csv scan options, map to CPP struct CsvFragmentScanOptions.
+ *
+ * @param convertOptions same struct in CPP
Review Comment:
Maybe we should use different wording than `same struct in CPP` but describe the variable?
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/csv/CsvFragmentScanOptions.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.
+ */
+package org.apache.arrow.dataset.scanner.csv;
+
+import java.io.Serializable;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.arrow.dataset.file.FileFormat;
+import org.apache.arrow.dataset.scanner.FragmentScanOptions;
+
+public class CsvFragmentScanOptions implements Serializable, FragmentScanOptions {
+ private final CsvConvertOptions convertOptions;
+ private final Map<String, String> readOptions;
+ private final Map<String, String> parseOptions;
+
+ /**
+ * csv scan options, map to CPP struct CsvFragmentScanOptions.
+ *
+ * @param convertOptions same struct in CPP
+ * @param readOptions same struct in CPP
+ * @param parseOptions same struct in CPP
+ */
+ public CsvFragmentScanOptions(
+ CsvConvertOptions convertOptions,
+ Map<String, String> readOptions,
+ Map<String, String> parseOptions) {
+ this.convertOptions = convertOptions;
+ this.readOptions = readOptions;
+ this.parseOptions = parseOptions;
+ }
+
+ public String typeName() {
+ return FileFormat.CSV.name().toLowerCase(Locale.ROOT);
+ }
+
+ /**
+ * File format id.
+ *
+ * @return id
+ */
+ public int fileFormatId() {
+ return FileFormat.CSV.id();
+ }
+
+ /**
+ * Serialize this class to ByteBuffer and then called by jni call.
Review Comment:
```suggestion
* Serialize this class to ByteBuffer and then called by JNI call.
```
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/csv/CsvFragmentScanOptions.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.
+ */
+package org.apache.arrow.dataset.scanner.csv;
+
+import java.io.Serializable;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.arrow.dataset.file.FileFormat;
+import org.apache.arrow.dataset.scanner.FragmentScanOptions;
+
+public class CsvFragmentScanOptions implements Serializable, FragmentScanOptions {
+ private final CsvConvertOptions convertOptions;
+ private final Map<String, String> readOptions;
+ private final Map<String, String> parseOptions;
+
+ /**
+ * csv scan options, map to CPP struct CsvFragmentScanOptions.
+ *
+ * @param convertOptions same struct in CPP
+ * @param readOptions same struct in CPP
+ * @param parseOptions same struct in CPP
+ */
+ public CsvFragmentScanOptions(
+ CsvConvertOptions convertOptions,
+ Map<String, String> readOptions,
+ Map<String, String> parseOptions) {
+ this.convertOptions = convertOptions;
+ this.readOptions = readOptions;
+ this.parseOptions = parseOptions;
+ }
+
+ public String typeName() {
+ return FileFormat.CSV.name().toLowerCase(Locale.ROOT);
+ }
+
+ /**
+ * File format id.
+ *
+ * @return id
+ */
+ public int fileFormatId() {
+ return FileFormat.CSV.id();
+ }
+
+ /**
+ * Serialize this class to ByteBuffer and then called by jni call.
+ *
+ * @return DirectByteBuffer
Review Comment:
We are returning a `String[]` right?
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/FragmentScanOptions.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.
+ */
+package org.apache.arrow.dataset.scanner;
+
+import java.util.Map;
+
+public interface FragmentScanOptions {
+ String typeName();
+
+ int fileFormatId();
+
+ String[] serialize();
+
+ /**
+ * Serialize the map to string array.
+ *
+ * @param config config map
+ * @return string array for serialization
+ */
+ default String[] serializeMap(Map<String, String> config) {
Review Comment:
Then, maybe `convertMapToStringArray` or something?
##########
java/dataset/src/test/java/org/apache/arrow/dataset/TestFragmentScanOptions.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.
+ */
+package org.apache.arrow.dataset;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Arrays;
+import java.util.Optional;
+import org.apache.arrow.c.ArrowSchema;
+import org.apache.arrow.c.CDataDictionaryProvider;
+import org.apache.arrow.c.Data;
+import org.apache.arrow.dataset.file.FileFormat;
+import org.apache.arrow.dataset.file.FileSystemDatasetFactory;
+import org.apache.arrow.dataset.jni.NativeMemoryPool;
+import org.apache.arrow.dataset.scanner.ScanOptions;
+import org.apache.arrow.dataset.scanner.Scanner;
+import org.apache.arrow.dataset.scanner.csv.CsvConvertOptions;
+import org.apache.arrow.dataset.scanner.csv.CsvFragmentScanOptions;
+import org.apache.arrow.dataset.source.Dataset;
+import org.apache.arrow.dataset.source.DatasetFactory;
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.vector.ipc.ArrowReader;
+import org.apache.arrow.vector.types.pojo.ArrowType;
+import org.apache.arrow.vector.types.pojo.Field;
+import org.apache.arrow.vector.types.pojo.Schema;
+import org.junit.jupiter.api.Test;
+
+public class TestFragmentScanOptions {
+
+ @Test
+ public void testCsvConvertOptions() throws Exception {
+ final Schema schema =
+ new Schema(
+ Arrays.asList(
+ Field.nullable("Id", new ArrowType.Int(32, true)),
+ Field.nullable("Name", new ArrowType.Utf8()),
+ Field.nullable("Language", new ArrowType.Utf8())),
+ null);
+ String path = "file://" + getClass().getResource("/").getPath() + "/data/student.csv";
+ BufferAllocator allocator = new RootAllocator(Long.MAX_VALUE);
+ try (ArrowSchema cSchema = ArrowSchema.allocateNew(allocator);
+ CDataDictionaryProvider provider = new CDataDictionaryProvider()) {
+ Data.exportSchema(allocator, schema, provider, cSchema);
+ CsvConvertOptions convertOptions = new CsvConvertOptions(ImmutableMap.of("delimiter", ";"));
+ convertOptions.setArrowSchema(cSchema);
+ CsvFragmentScanOptions fragmentScanOptions =
Review Comment:
should we also test with some configs for `CsvFragmentScanOptions`?
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2259855172
I find arrow-dataset tests don't run in github Action, is it right? only arrow-c-data test runs.
because environment `ARROW_DATASET` not set. @lidavidm @vibhatha
https://github.com/apache/arrow/blob/main/ci/scripts/java_test.sh#L41
https://github.com/apache/arrow/blob/main/ci/docker/java-jni-manylinux-201x.dockerfile#L53
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2255159172
@github-actions crossbow submit java-jars
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1623718432
##########
java/dataset/src/main/java/org/apache/arrow/dataset/substrait/util/ConvertUtil.java:
##########
@@ -24,10 +24,12 @@
public class ConvertUtil {
/**
- * Convert map to substrait Expression.
+ * Convert map to substrait Expression Literal.
+ * @param values config map
*
- * @return Substrait Expression
+ * @return substrait Expression Literal
Review Comment:
I find the brief comments starts with uppercase, but param and returns starts with lower case, which is the expected syntax? https://github.com/apache/arrow/blob/main/java/dataset/src/main/java/org/apache/arrow/dataset/jni/JniWrapper.java#L80
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1605602100
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/FragmentScanOptions.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.dataset.scanner;
+
+import java.nio.ByteBuffer;
+import java.util.Map;
+
+import org.apache.arrow.dataset.substrait.StringMapNode;
+
+import com.google.protobuf.Any;
+
+import io.substrait.proto.AdvancedExtension;
+
+public interface FragmentScanOptions {
+ String typeName();
+
+ int fileFormatId();
+
+ ByteBuffer serialize();
+
+ /**
+ * serialize the map.
+ *
+ * @param config config map
+ * @return bufer to jni call argument, should be DirectByteBuffer
+ */
+ default ByteBuffer serializeMap(Map<String, String> config) {
Review Comment:
Is this function just written to pass a Java Map to C++ via JNI?
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642218902
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
Maybe `MapEntryLite<K, V> ` is a better choice.
But we use proto2, Struct and MapEntryLite is introduced in proto3, now it is hard for us to upgrade protobuf.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1643605320
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
So which way would you prefer to implement this feature?
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642057478
##########
java/dataset/pom.xml:
##########
@@ -25,6 +25,8 @@
<arrow.cpp.build.dir>../../../cpp/release-build/</arrow.cpp.build.dir>
<parquet.version>1.13.1</parquet.version>
<avro.version>1.11.3</avro.version>
+ <substrait.version>0.31.0</substrait.version>
+ <protobuf.version>3.25.3</protobuf.version>
Review Comment:
No, this is only defined in gandiva like this format, not exists in dependencyManagement
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642611813
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
Either way: I'm not comfortable with the current approach. It exposes new public APIs that are specific to Dataset and should not be public, and I'm skeptical of encoding our custom Dataset options into Substrait in the first place.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1643592421
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
I'm not willing to shade Protobuf for this...
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1605595656
##########
cpp/src/arrow/dataset/file_csv.cc:
##########
@@ -503,5 +507,31 @@ Future<> CsvFileWriter::FinishInternal() {
return Status::OK();
}
+Result<std::shared_ptr<FragmentScanOptions>> CsvFragmentScanOptions::from(
+ const std::unordered_map<std::string, std::string>& configs) {
+ std::shared_ptr<CsvFragmentScanOptions> options =
+ std::make_shared<CsvFragmentScanOptions>();
+ for (auto const& it : configs) {
+ auto& key = it.first;
+ auto& value = it.second;
+ if (key == "delimiter") {
+ options->parse_options.delimiter = value.data()[0];
+ } else if (key == "quoting") {
+ options->parse_options.quoting = parseBool(value);
+ } else if (key == "column_type") {
+ int64_t schema_address = std::stol(value);
+ ArrowSchema* cSchema = reinterpret_cast<ArrowSchema*>(schema_address);
+ ARROW_ASSIGN_OR_RAISE(auto schema, arrow::ImportSchema(cSchema));
+ auto column_types = options->convert_options.column_types;
+ for (auto field : schema->fields()) {
+ column_types[field->name()] = field->type();
+ }
+ } else {
+ return Status::Invalid("Not support this config " + it.first);
Review Comment:
Maybe:
```suggestion
return Status::Invalid("Config " + it.first + " is not supported.");
```
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1605596872
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -154,6 +155,21 @@ arrow::Result<std::shared_ptr<arrow::dataset::FileFormat>> GetFileFormat(
}
}
+arrow::Result<std::shared_ptr<arrow::dataset::FragmentScanOptions>>
+GetFragmentScanOptions(jint file_format_id,
+ const std::unordered_map<std::string, std::string>& configs) {
+ switch (file_format_id) {
+#ifdef ARROW_CSV
+ case 3:
+ return arrow::dataset::CsvFragmentScanOptions::from(configs);
+#endif
+ default:
+ std::string error_message =
+ "illegal file format id: " + std::to_string(file_format_id);
Review Comment:
nit:
```suggestion
"Illegal file format id: " + std::to_string(file_format_id);
```
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1623038920
##########
java/dataset/src/main/java/org/apache/arrow/dataset/file/JniWrapper.java:
##########
@@ -54,7 +57,8 @@ private JniWrapper() {
* @return the native pointer of the arrow::dataset::FileSystemDatasetFactory instance.
* @see FileFormat
*/
- public native long makeFileSystemDatasetFactory(String[] uris, int fileFormat);
+ public native long makeFileSystemDatasetFactoryWithFiles(String[] uris, int fileFormat,
Review Comment:
Update Java docs
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1623036737
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -154,6 +155,19 @@ arrow::Result<std::shared_ptr<arrow::dataset::FileFormat>> GetFileFormat(
}
}
+arrow::Result<std::shared_ptr<arrow::dataset::FragmentScanOptions>>
+GetFragmentScanOptions(jint file_format_id,
+ const std::unordered_map<std::string, std::string>& configs) {
+ switch (file_format_id) {
+#ifdef ARROW_CSV
+ case 3:
+ return arrow::dataset::CsvFragmentScanOptions::From(configs);
+#endif
+ default:
+ return arrow::Status::Invalid("Illegal file format id: " ,file_format_id);
Review Comment:
nit: I think we don't have proper C++ linting @lidavidm ?
```suggestion
return arrow::Status::Invalid("Illegal file format id: ", file_format_id);
```
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1623040761
##########
java/dataset/src/test/java/org/apache/arrow/dataset/substrait/TestAceroSubstraitConsumer.java:
##########
@@ -457,4 +465,42 @@ private static ByteBuffer getByteBuffer(String base64EncodedSubstrait) {
substraitExpression.put(decodedSubstrait);
return substraitExpression;
}
+
+ @Test
+ public void testCsvConvertOptions() throws Exception {
+ final Schema schema = new Schema(Arrays.asList(
+ Field.nullable("Id", new ArrowType.Int(32, true)),
+ Field.nullable("Name", new ArrowType.Utf8()),
+ Field.nullable("Language", new ArrowType.Utf8())
+ ), null);
+ String path = "file://" + getClass().getResource("/").getPath() + "/data/student.csv";
+ BufferAllocator allocator = rootAllocator();
+ try (ArrowSchema cSchema = ArrowSchema.allocateNew(allocator);
+ CDataDictionaryProvider provider = new CDataDictionaryProvider()) {
+ Data.exportSchema(allocator, schema, provider, cSchema);
+ CsvConvertOptions convertOptions = new CsvConvertOptions(ImmutableMap.of("delimiter", ";"));
+ convertOptions.setArrowSchema(cSchema);
+ CsvFragmentScanOptions fragmentScanOptions = new CsvFragmentScanOptions(
+ convertOptions, ImmutableMap.of(), ImmutableMap.of());
+ ScanOptions options = new ScanOptions.Builder(/*batchSize*/ 32768)
+ .columns(Optional.empty())
+ .fragmentScanOptions(fragmentScanOptions)
+ .build();
+ try (
+ DatasetFactory datasetFactory = new FileSystemDatasetFactory(allocator, NativeMemoryPool.getDefault(),
+ FileFormat.CSV, path);
+ Dataset dataset = datasetFactory.finish();
+ Scanner scanner = dataset.newScan(options);
+ ArrowReader reader = scanner.scanBatches()
+ ) {
+ assertEquals(schema.getFields(), reader.getVectorSchemaRoot().getSchema().getFields());
+ int rowCount = 0;
+ while (reader.loadNextBatch()) {
+ assertEquals("[1, 2, 3]", reader.getVectorSchemaRoot().getVector("Id").toString());
Review Comment:
nit: should we check all columns?
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1604242932
##########
cpp/thirdparty/versions.txt:
##########
@@ -108,7 +108,7 @@ ARROW_SUBSTRAIT_BUILD_SHA256_CHECKSUM=f989a862f694e7dbb695925ddb7c4ce06aa6c51aca
ARROW_S2N_TLS_BUILD_VERSION=v1.3.35
ARROW_S2N_TLS_BUILD_SHA256_CHECKSUM=9d32b26e6bfcc058d98248bf8fc231537e347395dd89cf62bb432b55c5da990d
ARROW_THRIFT_BUILD_VERSION=0.16.0
-ARROW_THRIFT_BUILD_SHA256_CHECKSUM=f460b5c1ca30d8918ff95ea3eb6291b3951cf518553566088f3f2be8981f6209
+ARROW_THRIFT_BUILD_SHA256_CHECKSUM=df2931de646a366c2e5962af679018bca2395d586e00ba82d09c0379f14f8e7b
Review Comment:
Occasional change, for my local environment, will remove it
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642219380
##########
cpp/src/arrow/dataset/file_csv.h:
##########
@@ -85,6 +85,10 @@ class ARROW_DS_EXPORT CsvFileFormat : public FileFormat {
struct ARROW_DS_EXPORT CsvFragmentScanOptions : public FragmentScanOptions {
std::string type_name() const override { return kCsvTypeName; }
+ /// \brief Construct FragmentScanOptions from config map
Review Comment:
yes
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1645284541
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
If we use map, it will be very hard to reconstruct, https://stackoverflow.com/questions/4844022/jni-create-hashmap
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/FragmentScanOptions.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.
+ */
+package org.apache.arrow.dataset.scanner;
+
+import java.util.Map;
+
+public interface FragmentScanOptions {
+ String typeName();
+
+ int fileFormatId();
+
+ String[] serialize();
+
+ /**
+ * Serialize the map to string array.
+ *
+ * @param config config map
+ * @return string array for serialization
+ */
+ default String[] serializeMap(Map<String, String> config) {
Review Comment:
just converting
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1645275950
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -363,6 +364,64 @@ std::shared_ptr<arrow::Buffer> LoadArrowBufferFromByteBuffer(JNIEnv* env, jobjec
return buffer;
}
+inline bool ParseBool(const std::string& value) { return value == "true" ? true : false; }
+
+/// \brief Construct FragmentScanOptions from config map
+#ifdef ARROW_CSV
+arrow::Result<std::shared_ptr<arrow::dataset::FragmentScanOptions>>
+ToCsvFragmentScanOptions(const std::unordered_map<std::string, std::string>& configs) {
+ std::shared_ptr<arrow::dataset::CsvFragmentScanOptions> options =
+ std::make_shared<arrow::dataset::CsvFragmentScanOptions>();
+ for (auto const& it : configs) {
+ auto& key = it.first;
+ auto& value = it.second;
+ if (key == "delimiter") {
+ options->parse_options.delimiter = value.data()[0];
+ } else if (key == "quoting") {
+ options->parse_options.quoting = ParseBool(value);
+ } else if (key == "column_types") {
+ int64_t schema_address = std::stol(value);
+ ArrowSchema* cSchema = reinterpret_cast<ArrowSchema*>(schema_address);
+ ARROW_ASSIGN_OR_RAISE(auto schema, arrow::ImportSchema(cSchema));
+ auto& column_types = options->convert_options.column_types;
+ for (auto field : schema->fields()) {
+ column_types[field->name()] = field->type();
+ }
+ } else if (key == "strings_can_be_null") {
+ options->convert_options.strings_can_be_null = ParseBool(value);
+ } else {
+ return arrow::Status::Invalid("Config " + it.first + " is not supported.");
+ }
+ }
+ return options;
+}
+#endif
+
+arrow::Result<std::shared_ptr<arrow::dataset::FragmentScanOptions>>
+GetFragmentScanOptions(jint file_format_id,
+ const std::unordered_map<std::string, std::string>& configs) {
+ switch (file_format_id) {
+#ifdef ARROW_CSV
+ case 3:
+ return ToCsvFragmentScanOptions(configs);
+#endif
+ default:
+ return arrow::Status::Invalid("Illegal file format id: ", file_format_id);
+ }
+}
+
+std::unordered_map<std::string, std::string> ToStringMap(JNIEnv* env,
+ jobjectArray& str_array) {
+ int length = env->GetArrayLength(str_array);
+ std::unordered_map<std::string, std::string> map;
+ for (int i = 0; i < length; i += 2) {
Review Comment:
Since we do `i =+2`, should we ensure the length is even? And maybe reserve the map since we know the length?
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/FragmentScanOptions.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.
+ */
+package org.apache.arrow.dataset.scanner;
+
+import java.util.Map;
+
+public interface FragmentScanOptions {
+ String typeName();
+
+ int fileFormatId();
+
+ String[] serialize();
+
+ /**
+ * Serialize the map to string array.
+ *
+ * @param config config map
+ * @return string array for serialization
+ */
+ default String[] serializeMap(Map<String, String> config) {
Review Comment:
Are we actually doing any serialization here or just converting a map to a String[] such that even indexes are keys and odd indexes are values? Maybe a different name?
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2172915488
I checked the ( and { is right, but still meet this exception, do you have some clue? @lidavidm
```
dataset/CMakeFiles/arrow_java_jni_dataset.dir/src/main/cpp/jni_wrapper.cc.o -c /arrow/java/dataset/src/main/cpp/jni_wrapper.cc
/arrow/java/dataset/src/main/cpp/jni_wrapper.cc:388:57: error: expected ‘,’ or ‘...’ before ‘struct’
388 | arrow::Status FromProto(const google::protobuf::Struct& struct,
| ^~~~~~
/arrow/java/dataset/src/main/cpp/jni_wrapper.cc: In function ‘arrow::Status FromProto(const google::protobuf::Struct&)’:
/arrow/java/dataset/src/main/cpp/jni_wrapper.cc:390:7: error: expected primary-expression before ‘struct’
390 | if (struct.fields().empty()) {
| ^~~~~~
/arrow/java/dataset/src/main/cpp/jni_wrapper.cc:390:7: error: expected ‘)’ before ‘struct’
390 | if (struct.fields().empty()) {
| ~^~~~~~
| )
/arrow/java/dataset/src/main/cpp/jni_wrapper.cc:393:36: error: expected primary-expression before ‘struct’
393 | for (const auto& [name, value] : struct.fields()) {
| ^~~~~~
/arrow/java/dataset/src/main/cpp/jni_wrapper.cc:393:35: error: expected ‘)’ before ‘struct’
393 | for (const auto& [name, value] : struct.fields()) {
| ~ ^~~~~~~
| )
/arrow/java/dataset/src/main/cpp/jni_wrapper.cc:393:36: error: expected primary-expression before ‘struct’
393 | for (const auto& [name, value] : struct.fields()) {
| ^~~~~~
In file included from /tmp/local/include/arrow/device.h:26,
from /tmp/local/include/arrow/buffer.h:29,
from /tmp/local/include/arrow/array/data.h:27,
from /tmp/local/include/arrow/array/array_base.h:26,
from /tmp/local/include/arrow/array.h:41,
from /arrow/java/dataset/src/main/cpp/jni_wrapper.cc:24:
/arrow/java/dataset/src/main/cpp/jni_wrapper.cc: In function ‘arrow::Status DeserializeMap(const arrow::Buffer&, std::unordered_map<std::basic_string<char>, std::basic_string<char> >&)’:
/arrow/java/dataset/src/main/cpp/jni_wrapper.cc:405:25: error: expected primary-expression before ‘auto’
405 | ARROW_ASSIGN_OR_RAISE(auto struct, ParseFromBuffer<google::protobuf::Struct>(buf));
| ^~~~
/arrow/java/dataset/src/main/cpp/jni_wrapper.cc:406:20: error: expected primary-expression before ‘struct’
406 | return FromProto(struct, out);
| ^~~~~~
```
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2244398836
@lidavidm Do you have further comments?
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2259860892
@lidavidm [this](https://github.com/apache/arrow/blob/62fd98704dbe2684018707a7b135751fa7bfbe5a/ci/scripts/java_test.sh#L42) seems to be wrong :thinking:
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2144483184
@kou there is a C GLib & Ruby CI failing. I am not sure if this related though.
Also there is a failure in continuous integration which is due to a conda issue.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642225600
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
(1) We should be using proto3. Where are we using proto2? (2) MapEntryLite is purely an implementation detail; the wire message should be google.protobuf.Struct or similar
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1623860046
##########
java/dataset/src/main/java/org/apache/arrow/dataset/substrait/util/ConvertUtil.java:
##########
@@ -24,10 +24,12 @@
public class ConvertUtil {
/**
- * Convert map to substrait Expression.
+ * Convert map to substrait Expression Literal.
+ * @param values config map
*
- * @return Substrait Expression
+ * @return substrait Expression Literal
Review Comment:
Oh sorry, you're right.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1624214651
##########
java/dataset/src/main/java/org/apache/arrow/dataset/substrait/util/ConvertUtil.java:
##########
@@ -24,10 +24,12 @@
public class ConvertUtil {
/**
- * Convert map to substrait Expression.
+ * Convert map to substrait Expression Literal.
+ * @param values config map
*
- * @return Substrait Expression
+ * @return substrait Expression Literal
Review Comment:
"Substrait" is a proper noun, though
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642064130
##########
java/dataset/src/main/cpp/jni_wrapper.cc:
##########
@@ -153,6 +154,19 @@ arrow::Result<std::shared_ptr<arrow::dataset::FileFormat>> GetFileFormat(
}
}
+arrow::Result<std::shared_ptr<arrow::dataset::FragmentScanOptions>>
+GetFragmentScanOptions(jint file_format_id,
+ const std::unordered_map<std::string, std::string>& configs) {
+ switch (file_format_id) {
+#ifdef ARROW_CSV
+ case 3:
Review Comment:
Ah, I see. Maybe something to be fixed but not a big deal for me then. Thanks!
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642111646
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
It seems the generated code is only available to the Substrait library which makes this rather painful. Can we choose a serialization that does not depend on Substrait definitions? It's just a string-string map, right?
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642116526
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
For example, google.protobuf.Struct? https://protobuf.dev/reference/protobuf/google.protobuf/#struct
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1642610863
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
Er, we're using Protobuf 3 already in this PR. What's the issue?
If Protobuf will cause this many issues then I would like to drop Protobuf here entirely.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1643593928
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -183,6 +184,13 @@ ARROW_ENGINE_EXPORT Result<BoundExpressions> DeserializeExpressions(
const ConversionOptions& conversion_options = {},
ExtensionSet* ext_set_out = NULLPTR);
+/// \brief Deserialize a Substrait Literal message to the map
+///
+/// \param[in] buf a buffer containing the protobuf serialization of a Substrait Literal
+/// \param[out] out deserialize to this map.
+ARROW_ENGINE_EXPORT Status
+DeserializeMap(const Buffer& buf, std::unordered_map<std::string, std::string>& out);
Review Comment:
Why? flight-core also shade protobuf.
--
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
Re: [PR] GH-28866: [Java] Java Dataset API ScanOptions expansion [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #41646:
URL: https://github.com/apache/arrow/pull/41646#issuecomment-2260592221
Can you make a PR?
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1623039132
##########
java/dataset/src/main/java/org/apache/arrow/dataset/jni/JniWrapper.java:
##########
@@ -80,7 +80,8 @@ private JniWrapper() {
* @return the native pointer of the arrow::dataset::Scanner instance.
*/
public native long createScanner(long datasetId, String[] columns, ByteBuffer substraitProjection,
Review Comment:
update Java docs?
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1623716785
##########
java/dataset/src/main/java/org/apache/arrow/dataset/substrait/util/ConvertUtil.java:
##########
@@ -24,10 +24,12 @@
public class ConvertUtil {
/**
- * Convert map to substrait Expression.
+ * Convert map to substrait Expression Literal.
+ * @param values config map
*
- * @return Substrait Expression
+ * @return substrait Expression Literal
Review Comment:
nit:
```suggestion
* @return Substrait Expression Literal
```
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1605605746
##########
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/csv/CsvFragmentScanOptions.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.dataset.scanner.csv;
+
+import java.io.Serializable;
+import java.nio.ByteBuffer;
+import java.util.Locale;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.apache.arrow.dataset.file.FileFormat;
+import org.apache.arrow.dataset.scanner.FragmentScanOptions;
+
+public class CsvFragmentScanOptions implements Serializable, FragmentScanOptions {
+ private final CsvConvertOptions convertOptions;
+ private final Map<String, String> readOptions;
+ private final Map<String, String> parseOptions;
+
+
+ /**
+ * csv scan options, map to CPP struct CsvFragmentScanOptions.
+ *
+ * @param convertOptions same struct in CPP
+ * @param readOptions same struct in CPP
+ * @param parseOptions same struct in CPP
+ */
+ public CsvFragmentScanOptions(CsvConvertOptions convertOptions,
+ Map<String, String> readOptions,
+ Map<String, String> parseOptions) {
+ this.convertOptions = convertOptions;
+ this.readOptions = readOptions;
+ this.parseOptions = parseOptions;
+ }
+
+ public String typeName() {
+ return FileFormat.CSV.name().toLowerCase(Locale.ROOT);
+ }
+
+ /**
+ * File format id.
+ *
+ * @return id
+ */
+ public int fileFormatId() {
+ return FileFormat.CSV.id();
+ }
+
+ /**
+ * Serialize this class to ByteBuffer and then called by jni call.
+ *
+ * @return DirectByteBuffer
+ */
+ public ByteBuffer serialize() {
+ Map<String, String> options = Stream.concat(Stream.concat(readOptions.entrySet().stream(),
+ parseOptions.entrySet().stream()),
Review Comment:
I believe having a better serialize option for each would be better. But I see your point, maybe we could do it in a follow up PR.
cc @lidavidm @westonpace
--
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
Re: [PR] GH-28866: [JAVA] Java Dataset API ScanOptions expansion [arrow]
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #41646:
URL: https://github.com/apache/arrow/pull/41646#discussion_r1605595990
##########
cpp/src/arrow/engine/substrait/extension_internal.cc:
##########
@@ -0,0 +1,51 @@
+// 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.
+
+// This API is EXPERIMENTAL.
+
+#include "arrow/engine/substrait/extension_internal.h"
+
+#include "substrait/algebra.pb.h"
+
+namespace arrow {
+namespace engine {
+
+Status FromProto(const substrait::extensions::AdvancedExtension& extension,
+ std::unordered_map<std::string, std::string>& out) {
+ if (!extension.has_enhancement()) {
+ return Status::Invalid("AdvancedExtension does not have enhancement");
+ }
+ const auto& enhancement = extension.enhancement();
+ substrait::Expression_Literal literal;
+
+ if (!enhancement.UnpackTo(&literal)) {
+ return Status::Invalid("Unpack the literal failed");
+ }
+
+ if (!literal.has_map()) {
+ return Status::Invalid("Literal does not have map");
Review Comment:
nit:
```suggestion
return Status::Invalid("Literal does not have a map");
```
--
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