GH-34223: [Java] Java Substrait Consumer JNI call to ACERO C++ by davisusanibar · Pull Request #34227 · apache/arrow

davisusanibar · 2023-02-16T22:42:48Z

Closes: [Java] Proposal for Java Substrait Consumers #34223

The purpose of this PR is to implement:

JNI Wrappers to consume Acero capabilities that execute Substrait Plans.
Java base code to offer API that consume Substrait Plans.
Initial Substrait documentation

github-actions · 2023-02-16T22:43:12Z

Closes: [Java] Proposal for Java Substrait Consumers #34223

github-actions · 2023-02-16T22:43:14Z

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

davisusanibar · 2023-02-21T15:26:20Z

@github-actions crossbow submit java-jars

github-actions · 2023-02-21T15:28:59Z

Revision: 7500dd7c5e8ce6b0fbd3a0c800439980e508bfd8

Submitted crossbow builds: ursacomputing/crossbow @ actions-1555224742

Task	Status
java-jars

github-actions · 2023-02-21T15:37:57Z

Revision: 7500dd7c5e8ce6b0fbd3a0c800439980e508bfd8

Submitted crossbow builds: ursacomputing/crossbow @ actions-c5c0135c13

Task	Status
java-jars

lidavidm · 2023-02-22T20:56:35Z

Seems like this should be an interface?

lidavidm · 2023-02-22T20:56:43Z

Plans should be byte[] or ByteBuffer?

It seems this assumes JSON plans which is probably not what people will be working with

Please if yu could help me, What will be my options to exchange a Substrait Plan (Java byte) to std::shared_ptr<arrow::Buffer> called by JNI?

JNI functions can read byte arrays, you can then copy into an Arrow buffer (this is probably good anyways to avoid too many cross-boundary dependencies)

I just create this ByteBuffer with Substrait Plan
ByteBuffer directByteBuffer = ByteBuffer.allocateDirect(64); directByteBuffer.put("DEMO_SUBSTRAIT_PLAN".getBytes(StandardCharsets.UTF_8)); // protoPlan.toByteArray();
JNI Wrapper:
Recover ByteArray with:
jbyte *buff = (jbyte *) env->GetDirectBufferAddress(plan);
How I could transform that jbyte into std::shared_ptr<arrow::Buffer>

Get the capacity, allocate a mutable buffer, memcpy the result

lidavidm · 2023-02-22T20:58:16Z

This has to always match ARROW_DATASET so a separate variable won't make sense

…ary)

davisusanibar · 2023-03-15T22:15:03Z

@github-actions crossbow submit java-jars

github-actions · 2023-03-15T22:17:36Z

Revision: e5594f8

Submitted crossbow builds: ursacomputing/crossbow @ actions-bf821d607d

Task	Status
java-jars

…on the class path

davisusanibar · 2023-03-16T03:03:07Z

@github-actions crossbow submit java-jars

github-actions · 2023-03-16T03:05:36Z

Revision: 223ddef

Submitted crossbow builds: ursacomputing/crossbow @ actions-17d27841d2

Task	Status
java-jars

kou · 2023-03-16T03:18:06Z

Could you rebase on main to use #34480?

lidavidm · 2023-05-12T18:35:32Z

@github-actions crossbow submit java-jars

github-actions · 2023-05-12T18:37:54Z

Revision: ce7800b

Submitted crossbow builds: ursacomputing/crossbow @ actions-02cc882fff

Task	Status
java-jars

lidavidm · 2023-05-12T18:42:35Z

+ Map<String, String> metadataName = new HashMap<>();
+ metadataName.put("ARROW:extension:name", "varchar");
+ metadataName.put("ARROW:extension:metadata", "varchar{length:150}");


Is the metadata actually necessary?

Yes, it is needed, in the case of local tables we are seeing this metadata information in the response.

lidavidm · 2023-05-12T18:44:28Z

+ * @param planInput the JSON Substrait plan.
+ * @param memoryAddressOutput the memory address where RecordBatchReader is exported.
+ */
+ public native void executeSerializedPlanLocalFiles(String planInput, long memoryAddressOutput);


This was never addressed?

lidavidm · 2023-05-12T18:45:45Z

+ try {
+ AutoCloseables.close(arrowArrayStream);
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }


Why the extra catches?

lidavidm · 2023-05-24T12:10:39Z

+ try {
+ AutoCloseables.close(arrowArrayStream);
+ } catch (Exception e) {
+ throw new RuntimeException(e);


This method is declared to throw Exception already. Why are we catching and re-throwing this?

Why are we catching it in the first place? Let it propagate.

ok, changed

lidavidm · 2023-05-24T12:10:51Z

- } catch (RuntimeException e) {
- throw e;
 } catch (Exception e) {
 throw new RuntimeException(e);


lidavidm · 2023-05-24T12:11:22Z

- builder.replace(builder.indexOf("FILENAME_PLACEHOLDER"),
- builder.indexOf("FILENAME_PLACEHOLDER") + "FILENAME_PLACEHOLDER".length(), uri);
- return builder.toString();
+ return plan.replace("FILENAME_PLACEHOLDER", uri);


Again: why is this a whole method? Just inline it; it's only used once.

lidavidm

The last comments are still unaddressed.

ursabot · 2023-05-30T23:58:43Z

Benchmark runs are scheduled for baseline = c4ea194 and contender = 95c33d8. 95c33d8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.84% ⬆️0.0%] test-mac-arm
[Failed ⬇️3.22% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️0.0% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 95c33d82 ec2-t3-xlarge-us-east-2
[Failed] 95c33d82 test-mac-arm
[Failed] 95c33d82 ursa-i9-9960x
[Failed] 95c33d82 ursa-thinkcentre-m75q
[Finished] c4ea194c ec2-t3-xlarge-us-east-2
[Failed] c4ea194c test-mac-arm
[Finished] c4ea194c ursa-i9-9960x
[Failed] c4ea194c ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

ursabot · 2023-05-31T11:16:26Z

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

…ilter as a Substrait proto extended expression (#35570) ### Rationale for this change To close#34252 ### What changes are included in this PR? This is a proposal to try to solve: 1. Receive a list of Substrait scalar expressions and use them to Project a Dataset - [x] Draft a Substrait Extended Expression to test (this will be generated by 3rd party project such as Isthmus) - [x] Use C++ draft PR to Serialize/Deserialize Extended Expression proto messages - [x] Create JNI Wrapper for ScannerBuilder::Project - [x] Create JNI API - [x] Testing coverage - [x] Documentation Current problem is: `java.lang.RuntimeException: Inferring column projection from FieldRef FieldRef.FieldPath(0)`. Not able to infer by column position by able to infer by colum name. This problem is solved by #35798 This PR needs/use this PRs/Issues: - #34834 - #34227 - #35579 2. Receive a Boolean-valued Substrait scalar expression and use it to filter a Dataset - [x] Working to identify activities ### Are these changes tested? Initial unit test added. ### Are there any user-facing changes? No * Closes: #34252 Lead-authored-by: david dali susanibar arce <davi.sarces@gmail.com> Co-authored-by: Weston Pace <weston.pace@gmail.com> Co-authored-by: benibus <bpharks@gmx.com> Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Dane Pitkin <48041712+danepitkin@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com>

…der::Filter as a Substrait proto extended expression (apache#35570) ### Rationale for this change To closeapache#34252 ### What changes are included in this PR? This is a proposal to try to solve: 1. Receive a list of Substrait scalar expressions and use them to Project a Dataset - [x] Draft a Substrait Extended Expression to test (this will be generated by 3rd party project such as Isthmus) - [x] Use C++ draft PR to Serialize/Deserialize Extended Expression proto messages - [x] Create JNI Wrapper for ScannerBuilder::Project - [x] Create JNI API - [x] Testing coverage - [x] Documentation Current problem is: `java.lang.RuntimeException: Inferring column projection from FieldRef FieldRef.FieldPath(0)`. Not able to infer by column position by able to infer by colum name. This problem is solved by apache#35798 This PR needs/use this PRs/Issues: - apache#34834 - apache#34227 - apache#35579 2. Receive a Boolean-valued Substrait scalar expression and use it to filter a Dataset - [x] Working to identify activities ### Are these changes tested? Initial unit test added. ### Are there any user-facing changes? No * Closes: apache#34252 Lead-authored-by: david dali susanibar arce <davi.sarces@gmail.com> Co-authored-by: Weston Pace <weston.pace@gmail.com> Co-authored-by: benibus <bpharks@gmx.com> Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Dane Pitkin <48041712+danepitkin@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com>

github-actionsBot added the Component: Java label Feb 16, 2023

lidavidm requested changes Feb 22, 2023
View reviewed changes

github-actionsBot added the awaiting committer review Awaiting committer review label Mar 13, 2023

davisusanibar force-pushed the poc-substrait branch from 7500dd7 to 92cb8d2Compare March 13, 2023 15:42

github-actionsBot added the Component: Documentation label Mar 13, 2023

feat: consume Substrait Plan
a0aac46

davisusanibar force-pushed the poc-substrait branch from 92cb8d2 to a0aac46Compare March 13, 2023 16:37

davisusanibar added 5 commits March 13, 2023 12:23

fix: solving maven-dependency-plugin
0d91f09

feat: add support for execution of Substrait binary plans also
0599dc2

Upgrade to Java 11 to be able to consume Isthmus library
c794ae5

fix: profile to Java test with JDK11 (be able to consume Isthmus libr…
8cc5443
…ary)

fix: solve error to call Isthmus by Dataset that use JDK8
e5594f8

davisusanibar changed the title ~~GH-34223: [Java] Proposal for Java Substrait Consumer~~GH-34223: [Java] Java Substrait Consumer JNI call to ACERO C++Mar 15, 2023

davisusanibar marked this pull request as ready for review March 15, 2023 22:14

davisusanibar requested review from assignUser, kou and raulcd as code owners March 15, 2023 22:14

This was referenced Mar 15, 2023
[Java] Development Java JNI Wrappers for Substrait (*.h , *.cc) apache/arrow-java#217
Open
[Java] Development Java Substrait to call JNI Wrappers apache/arrow-java#216
Open

fix: detected both log4j-over-slf4j.jar AND bound slf4j-reload4j.jar …
223ddef
…on the class path

lidavidm reviewed May 12, 2023
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels May 12, 2023

davisusanibar added 3 commits May 21, 2023 22:08

Merge branch 'main' into poc-substrait
fdd042b

fix: solve code review comments
3dddea0

fix: solve code review comments
6bdae18

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 23, 2023

lidavidm reviewed May 24, 2023
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels May 24, 2023

fix: solve code review comments
2d9fc84

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 24, 2023

lidavidm requested changes May 24, 2023
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels May 24, 2023

fix: solve code review comments
9b5f0cb

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 24, 2023

lidavidm approved these changes May 24, 2023
View reviewed changes

github-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 24, 2023

lidavidm merged commit 95c33d8 into apache:mainMay 24, 2023

Uh oh!

Conversation

davisusanibar commented Feb 16, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actionsBot commented Feb 16, 2023

Uh oh!

github-actionsBot commented Feb 16, 2023

Uh oh!

davisusanibar commented Feb 21, 2023

Uh oh!

github-actionsBot commented Feb 21, 2023

Uh oh!

github-actionsBot commented Feb 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davisusanibar commented Mar 15, 2023

Uh oh!

github-actionsBot commented Mar 15, 2023

Uh oh!

davisusanibar commented Mar 16, 2023

Uh oh!

github-actionsBot commented Mar 16, 2023

Uh oh!

kou commented Mar 16, 2023

Uh oh!

lidavidm commented May 12, 2023

Uh oh!

github-actionsBot commented May 12, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

davisusanibar commented Feb 16, 2023•
edited
Loading