[Java] Java Dataset API ScanOptions expansion

[asfimport]

Currently, there are very few scanning options which we can set in the Java Dataset API [1].

Additionally, the options that exist now always must be set from Java, without the possibility to use sensible default values from core Arrow.

For my use-case, I want to be able to set the fragment_readahead option from the Java-side.

 

It would be great if:
 + ScanOptions.java would be expanded to allow us to set more, potentially all options related to scanner creation.
 + Java users can omit options to use the default values, e.g. [2].

It would be good to know what others think, and whether a PR for this is useful.

[1]https://github.com/apache/arrow/blob/master/java/dataset/src/main/java/org/apache/arrow/dataset/scanner/ScanOptions.java
[2]

arrow/cpp/src/arrow/dataset/scanner.h

Lines 51 to 53 in ad5dc82

	constexpr int64_t kDefaultBatchSize = 1 << 20;
	constexpr int32_t kDefaultBatchReadahead = 32;
	constexpr int32_t kDefaultFragmentReadahead = 8;

Reporter: Sebastiaan Alvarez Rodriguez

_{Note: This issue was originally created as ARROW-13166. Please see the migration documentation for further details.}

[asfimport]

Hongze Zhang / @zhztheplayer:
Thanks Sebastiaan! As long as the option is defined in C++ side I think it would be no problem to add it to Java side. Probably we should add a builder API to Java ScanOptions when we add many new parameters.

[asfimport]

Sebastiaan Alvarez Rodriguez:
Hey, I later on heard that the scanning API is going to change. It might be easier to hold off a bit, before we find ourselves in a position were we have to reimplement stuff again.

[asfimport]

Weston Pace / @westonpace:
Can you expand a bit more on what changes you think are coming up?  I think if you use the ScannerBuilder to set the options then I wouldn't expect any changes to the API coming up soon.  There is https://issues.apache.org/jira/browse/ARROW-12311 but it is more about simplifying the user API (so there aren't two ways, ScanOptions and ScannerBuilder, to do the same thing) than it is about removing or changing functionality.

[asfimport]

Sebastiaan Alvarez Rodriguez:
Oh nevermind, this change I was thinking about is actually about the syncscanner, asyncscanner, not of the scanner builder/options (https://issues.apache.org/jira/browse/ARROW-13161?focusedCommentId=17368831&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17368831)

I think it is good to go then.
I already have an implementation around that uses a builder, but don't have time to develop it further. Still, it might be a good base. Shall I open a PR with the base code?

[jinchengchenghh]

I also have the same requirement. I use dataset scan CSV, but cannot set the parseOptions and readOptions by java ScanOptions, I find ScanBuilder can set CsvFragmentScanOptions in C++ side, so the question is how can we serialize and deserialize CsvFragmentScanOptions.

I have 2 proposals:

1. Like the projection and filter serialization, we could use substrait ExtendedExpression::AdvancedExtension to serialize CsvFragmentScanOptions
2. Use protobuf message to serialize directly

We can do as this PR does.
https://github.com/apache/incubator-gluten/pull/3393/files

The first proposal will be ambiguous, because user does not know how to generate the serialized binary, they should be familliar with the AdvancedExtension detail implementation, I would prefer the second.

I can help to implement this feature, can you help to confirm the proposal? @westonpace

[westonpace]

Is the JNI -> dataset API using substrait today? If so, then I think 1 is preferred. However, the user should not provide AdvancedExtension, this should be taken care of internally.

For example, the user Java API for "scan CSV" could take in HashMap<String, String>. The Java implementation can convert this into a AdvancedExtension similar to the one described in incubator-gluten. Acero could be updated to detect this and configure the fragment options appropriately.

I will add, for consideration, a third proposal, which is that we could add CSV as a read type in Substrait (e.g. make a PR on the Substrait repo). There was some work started here but it was never finished.

That third option will be slower though. It would require Substrait alignment on what CSV options are common. This will take time and energy. It would be the most useful long term option.

[jinchengchenghh]

No, now it just uses the binary serialized substrait as jni input. https://github.com/apache/arrow/blob/main/java/dataset/src/test/java/org/apache/arrow/dataset/substrait/TestAceroSubstraitConsumer.java#L176

HashMap is OK for most of the options, but the CsvFragmentScanOptions include other fields, I'm not sure if the ConvertOptions may have same config name with ReadOptions in the future. And ConvertOptions
has complex config value like std::unordered_map<std::string, std::shared_ptr<DataType>> column_types; it should also be serialized, but we can support the basic option by HashMap, I'm not sure if their is requirement for the complex options in the future. What do you think? @westonpace
https://github.com/apache/arrow/blob/main/cpp/src/arrow/dataset/file_csv.h#L85

[westonpace]

No, now it just uses the binary serialized substrait as jni input.

If it is using serialized substrait as input then I don't understand how the C++ consumer would be able to apply the CSV options onto the plan.

Would this be a second pass that transforms the plan once it is deserialized? E.g. something like:

def run_plan(substrait_plan, extra_options):
  plan = deserialize_plan(substrait_plan)
  plan = transform_plan(plan, extra_options)
  run_deserialized_plan(plan)

No, now it just uses the binary serialized substrait as jni input

I still think approach 1 is preferred. The extra options can be supplied as part of ReadRel::advanced_extension.

And ConvertOptions
has complex config value like std::unordered_map<std::string, std::shared_ptr> column_types; it should also be serialized, but we can support the basic option by HashMap, I'm not sure if their is requirement for the complex options in the future.

I think HashMap should be sufficient. Passing shared_ptr<DataType> across the JNI boundary is going to be error-prone. If we need advanced configuration like that then it should be done with a more complicated Substrait mesasge (e.g. use CsvOptions substrait message instead of a simple map. There are Substrait messages for types already).

[lidavidm]

Issue resolved by pull request 41646
#41646