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

jinchengchenghh · 2024-05-30T00:57:22Z

Do you have further comments? @vibhatha @lidavidm

vibhatha · 2024-05-30T01:15:59Z

@jinchengchenghh will re-review this later today.

vibhatha · 2024-05-31T23:33:30Z

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

+      return arrow::dataset::CsvFragmentScanOptions::From(configs);
+#endif
+    default:
+      return arrow::Status::Invalid("Illegal file format id: " ,file_format_id);


nit: I think we don't have proper C++ linting @lidavidm ?

Suggested change

return arrow::Status::Invalid("Illegal file format id: " ,file_format_id);

return arrow::Status::Invalid("Illegal file format id: ", file_format_id);

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?

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.

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

support dataset option

1223f36

vibhatha reviewed May 17, 2024

View reviewed changes

address comments

0f35c70

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

jinchengchenghh added 3 commits May 20, 2024 17:23

change to serialize ExpressionLiteral

608f568

fix column type

c1b1119

fix check style

ceb95fe

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

jinchengchenghh added 2 commits May 22, 2024 08:55

fix

acde1ea

fix code style

97e591b

jinchengchenghh mentioned this pull request May 23, 2024

[GLUTEN-5414] [VL] Support arrow csv option and schema apache/incubator-gluten#5850

Open

fix compile without gandiva

b335c34

vibhatha reviewed May 31, 2024

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jinchengchenghh commented May 14, 2024 •

edited

github-actions bot commented May 14, 2024

jinchengchenghh commented May 16, 2024

jinchengchenghh May 16, 2024

vibhatha May 17, 2024

lidavidm commented May 16, 2024

jinchengchenghh May 17, 2024

vibhatha May 17, 2024

vibhatha May 17, 2024

vibhatha May 17, 2024

vibhatha May 17, 2024

jinchengchenghh May 18, 2024

vibhatha May 17, 2024

jinchengchenghh May 22, 2024

vibhatha May 17, 2024

vibhatha May 17, 2024

vibhatha left a comment

jinchengchenghh commented May 30, 2024

vibhatha commented May 30, 2024

vibhatha May 31, 2024

vibhatha May 31, 2024

vibhatha May 31, 2024

vibhatha May 31, 2024

vibhatha May 31, 2024

vibhatha May 31, 2024

vibhatha left a comment

vibhatha commented May 31, 2024

	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 arrow::Status::Invalid("Illegal file format id: " ,file_format_id);
	return arrow::Status::Invalid("Illegal file format id: ", file_format_id);

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

jinchengchenghh commented May 30, 2024

vibhatha commented May 30, 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

vibhatha left a comment

Choose a reason for hiding this comment

vibhatha commented May 31, 2024

jinchengchenghh commented May 14, 2024 •

edited