GH-28866: [Java] Java Dataset API ScanOptions expansion #41646

jinchengchenghh · 2024-05-14T08:15:51Z

Rationale for this change

What changes are included in this PR?

Support to add ArrowSchema to specify C++ CsvFragmentScanOptions.convert_options.column_types
And use Map to set the config, serialize in java and deserialize in C++ for CsvFragmentScanOptions

Are these changes tested?

new added UT.

Are there any user-facing changes?

No.

GitHub Issue: [Java] Java Dataset API ScanOptions expansion #28866

github-actions · 2024-05-14T08:16:15Z

⚠️ GitHub issue #28866 has been automatically assigned in GitHub to PR creator.

jinchengchenghh · 2024-05-16T08:45:10Z

Can you help review this PR? If the framework is OK, I will add more common config in this PR. Thanks! @westonpace

jinchengchenghh · 2024-05-16T08:48:05Z

java/dataset/src/main/java/org/apache/arrow/dataset/scanner/csv/CsvFragmentScanOptions.java

+   */
+  public ByteBuffer serialize() {
+    Map<String, String> options = Stream.concat(Stream.concat(readOptions.entrySet().stream(),
+            parseOptions.entrySet().stream()),


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

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

lidavidm · 2024-05-16T10:06:46Z

CC @vibhatha

jinchengchenghh · 2024-05-17T01:47:30Z

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


Occasional change, for my local environment, will remove it

vibhatha · 2024-05-17T22:39:41Z

cpp/src/arrow/dataset/file_csv.cc

+        column_types[field->name()] = field->type();
+      }
+    } else {
+      return Status::Invalid("Not support this config " + it.first);


Maybe:

Suggested change

return Status::Invalid("Not support this config " + it.first);

return Status::Invalid("Config " + it.first + " is not supported.");

vibhatha · 2024-05-17T22:40:31Z

cpp/src/arrow/engine/substrait/extension_internal.cc

+  }
+
+  if (!literal.has_map()) {
+    return Status::Invalid("Literal does not have map");


nit:

Suggested change

return Status::Invalid("Literal does not have map");

return Status::Invalid("Literal does not have a map");

vibhatha · 2024-05-17T22:42:28Z

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

+#endif
+    default:
+      std::string error_message =
+          "illegal file format id: " + std::to_string(file_format_id);


nit:

Suggested change

"illegal file format id: " + std::to_string(file_format_id);

"Illegal file format id: " + std::to_string(file_format_id);

vibhatha · 2024-05-17T22:56:16Z

java/dataset/src/main/java/org/apache/arrow/dataset/scanner/FragmentScanOptions.java

+   * @param config config map
+   * @return bufer to jni call argument, should be DirectByteBuffer
+   */
+  default ByteBuffer serializeMap(Map<String, String> config) {


Is this function just written to pass a Java Map to C++ via JNI?

vibhatha · 2024-05-17T23:04:11Z

cpp/src/arrow/dataset/file_csv.cc

+    } else if (key == "quoting") {
+      options->parse_options.quoting = parseBool(value);
+    } else if (key == "column_type") {
+      int64_t schema_address = std::stol(value);


should we check for possible -1 ?

I changed it in Java side to not add invalid schema address

vibhatha · 2024-05-17T23:17:16Z

java/dataset/src/main/java/org/apache/arrow/dataset/substrait/StringMapNode.java

+
+import io.substrait.proto.Expression;
+
+public class StringMapNode implements Serializable {


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() ?

I also have doubts about having a separate class for this purpose though.

vibhatha

I only added a few comments. But I am going to go through the content once more.

cpp/src/arrow/dataset/file_csv.cc

vibhatha · 2024-05-31T23:40:07Z

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,


Update Java docs?

vibhatha · 2024-05-31T23:40:28Z

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,


Update Java docs

vibhatha · 2024-05-31T23:41:07Z

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,


update Java docs?

vibhatha · 2024-05-31T23:42:21Z

java/dataset/src/main/java/org/apache/arrow/dataset/scanner/FragmentScanOptions.java

+  ByteBuffer serialize();
+
+  /**
+   * serialize the map.


nit:

Suggested change

* serialize the map.

* Serialize the map.

vibhatha · 2024-05-31T23:46:53Z

java/dataset/src/test/java/org/apache/arrow/dataset/substrait/TestAceroSubstraitConsumer.java

+        assertEquals(schema.getFields(), reader.getVectorSchemaRoot().getSchema().getFields());
+        int rowCount = 0;
+        while (reader.loadNextBatch()) {
+          assertEquals("[1, 2, 3]", reader.getVectorSchemaRoot().getVector("Id").toString());


nit: should we check all columns?

No. the Id column is enough, the test tests delimiter config and schema of Id change from default int64 to int32.

vibhatha

@jinchengchenghh Added a few more comments.

vibhatha · 2024-05-31T23:48:05Z

@jinchengchenghh will re-review this later today.

@jinchengchenghh sorry about the unexpected delay. I have added a few comments today.

Support basic option now, will support more options after arrow patch merged. apache/arrow#41646 Before this patch, if the required schema is different with file schema, csv read will fallback. And changed to use index in file instead of check the file column name considering case sensitive. Add a new common test function when the rule applies to Logical plan. Compile arrow with version 15.0.0-gluten, upgrade arrow-dataset and arrow-c-data version from 15.0.0 to 15.0.0-gluten.

vibhatha · 2024-06-03T02:15:21Z

java/dataset/src/main/java/org/apache/arrow/dataset/substrait/util/ConvertUtil.java

   *
-   * @return Substrait Expression
+   * @return substrait Expression Literal


nit:

Suggested change

* @return substrait Expression Literal

* @return Substrait Expression Literal

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

Oh sorry, you're right.

"Substrait" is a proper noun, though

vibhatha · 2024-06-03T02:17:22Z

@jinchengchenghh shall we rebase and see if we can run the CIs?

vibhatha · 2024-06-03T07:36:27Z

@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.

jinchengchenghh · 2024-06-03T07:39:37Z

I suppose not, the newly added commit 1360221 only involves comments.

kou · 2024-06-03T09:40:24Z

@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.

vibhatha · 2024-06-03T09:43:33Z

@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.

lidavidm

I know we don't maintain a high standard of docs, but it would be appreciated to have basic docstrings for new classes/methods

lidavidm · 2024-06-03T12:20:24Z

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>


Is this not already defined by dependencyManagement elsewhere?

lidavidm · 2024-06-03T12:29:09Z

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

+                       const std::unordered_map<std::string, std::string>& configs) {
+  switch (file_format_id) {
+#ifdef ARROW_CSV
+    case 3:


Is there some enum/constant that defines the possible values?

lidavidm · 2024-06-03T12:34:44Z

cpp/src/arrow/engine/substrait/serde.h

+/// \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);


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.

lidavidm · 2024-06-03T12:35:44Z

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


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.

jinchengchenghh requested a review from lidavidm as a code owner May 14, 2024 08:15

jinchengchenghh marked this pull request as draft May 14, 2024 08:15

github-actions bot added Component: Java awaiting review Awaiting review labels May 14, 2024

jinchengchenghh force-pushed the option branch from 883417a to 5423c7e Compare May 14, 2024 23:28

github-actions bot added the Component: C++ label May 15, 2024

jinchengchenghh force-pushed the option branch from c90f078 to df5705e Compare May 15, 2024 22:37

jinchengchenghh marked this pull request as ready for review May 16, 2024 08:43

jinchengchenghh requested a review from westonpace as a code owner May 16, 2024 08:43

jinchengchenghh commented May 16, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 16, 2024

jinchengchenghh commented May 17, 2024

View reviewed changes

jinchengchenghh force-pushed the option branch from a48638b to 1223f36 Compare May 17, 2024 08:16

vibhatha reviewed May 17, 2024

View reviewed changes

jinchengchenghh force-pushed the option branch from 6d97d44 to 608f568 Compare May 20, 2024 10:00

lidavidm reviewed May 22, 2024

View reviewed changes

cpp/src/arrow/dataset/file_csv.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting committer review Awaiting committer review awaiting changes Awaiting changes labels May 22, 2024

vibhatha reviewed May 31, 2024

View reviewed changes

vibhatha requested a review from lidavidm May 31, 2024 23:48

vibhatha reviewed Jun 3, 2024

View reviewed changes

jinchengchenghh force-pushed the option branch from f98063a to 1360221 Compare June 3, 2024 02:21

kou changed the title ~~GH-28866: [JAVA] Java Dataset API ScanOptions expansion~~ GH-28866: [Java] Java Dataset API ScanOptions expansion Jun 3, 2024

jinchengchenghh added 10 commits June 3, 2024 09:44

support dataset option

c9979bc

address comments

511b804

change to serialize ExpressionLiteral

055b7b1

fix column type

9787adc

fix check style

a66ac63

fix

9b0c48b

fix code style

55a0cdf

fix compile without gandiva

04f2145

address comments

1360221

fix code style

164a23b

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 3, 2024

lidavidm reviewed Jun 3, 2024

View reviewed changes

	return Status::Invalid("Not support this config " + it.first);
	return Status::Invalid("Config " + it.first + " is not supported.");

	return Status::Invalid("Literal does not have map");
	return Status::Invalid("Literal does not have a map");

	"illegal file format id: " + std::to_string(file_format_id);
	"Illegal file format id: " + std::to_string(file_format_id);


		import io.substrait.proto.Expression;

		public class StringMapNode implements Serializable {

	* @return substrait Expression Literal
	* @return Substrait Expression Literal

GH-28866: [Java] Java Dataset API ScanOptions expansion #41646

Are you sure you want to change the base?

GH-28866: [Java] Java Dataset API ScanOptions expansion #41646

Conversation

jinchengchenghh commented May 14, 2024 • edited

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented May 14, 2024

jinchengchenghh commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vibhatha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vibhatha left a comment

Choose a reason for hiding this comment

vibhatha commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vibhatha commented Jun 3, 2024

vibhatha commented Jun 3, 2024

jinchengchenghh commented Jun 3, 2024

kou commented Jun 3, 2024

vibhatha commented Jun 3, 2024

lidavidm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh commented May 14, 2024 •

edited