GH-34252: [Java] Support ScannerBuilder::Project or ScannerBuilder::Filter as a Substrait proto extended expression #35570

davisusanibar · 2023-05-12T12:59:01Z

Rationale for this change

To close #34252

What changes are included in this PR?

This is a proposal to try to solve:

Receive a list of Substrait scalar expressions and use them to Project a Dataset

Draft a Substrait Extended Expression to test (this will be generated by 3rd party project such as Isthmus)
Use C++ draft PR to Serialize/Deserialize Extended Expression proto messages
Create JNI Wrapper for ScannerBuilder::Project
Create JNI API
Testing coverage
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:

Receive a Boolean-valued Substrait scalar expression and use it to filter a Dataset

Working to identify activities

Are these changes tested?

Initial unit test added.

Are there any user-facing changes?

No

Closes: [Java] Filter and project Datasets with Substrait expressions #34252

…ary)

…on the class path

Co-authored-by: David Li <li.davidm96@gmail.com>

…SubstraitConsumer.java Co-authored-by: David Li <li.davidm96@gmail.com>

…to read artifact

danepitkin · 2023-09-07T20:53:51Z

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

+
+  private static ByteBuffer getByteBuffer(String base64EncodedSubstrait) {
+    byte[] substraitFilter = Base64.getDecoder().decode(base64EncodedSubstrait);
+    ByteBuffer substraitExpressionFilter = ByteBuffer.allocateDirect(substraitFilter.length);


Don't forget about the rest of this function! e.g. substraitExpressionFilter and substraitFilter

danepitkin · 2023-09-12T14:50:01Z

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

+     * @param columns Projected columns. Empty for scanning all columns.
+     * @return the ScanOptions configured.
+     */
+    public Builder columns(Optional<String[]> columns) {


One more thing: We don't need Optional<> parameters for the builder APIs. We should expect the user to pass us a valid object. Same with substraitProjection and substraitFilter

The values for substraitProjection and substraitFilter have been changed.

The rule definition for columns mentions that empty means scanning all columns, so that is how it works.

Ah I see. I assumed a user would prefer to only use the columns API when they want to project a subset of columns because, if left blank, the builder will build an empty Optional<> columns object automatically. I'm okay with leaving as-is. Thanks for the udpates!

lidavidm · 2023-09-14T18:48:34Z

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

+        assertTrue(reader.getVectorSchemaRoot().getVector("id").toString().equals("[19, 1, 11]"));
+        assertTrue(reader.getVectorSchemaRoot().getVector("name").toString()
+            .equals("[value_19, value_1, value_11]"));


I think we should consider what to do here; possibly an Iterator/Iterable/Stream that gives Java objects would be sufficient (and you could collect into a Java collection and then use standard assertions). Can you file a follow-up task?

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

lidavidm · 2023-09-14T18:50:17Z

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

+    for(arrow::engine::NamedExpression& named_expression :
+                                        bounded_expression.named_expressions) {
+      if (named_expression.expression.type()->id() == arrow::Type::BOOL) {
+        if (filter_expr.has_value()) {
+          JniThrow("Only one filter expression may be provided");
+        }
+        filter_expr = named_expression.expression;
+      }
+    }
+    JniAssertOkOrThrow(scanner_builder->Filter(*filter_expr));


This will crash if you provide an empty list of expressions.

lidavidm · 2023-09-14T18:50:49Z

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

+      if (named_expression.expression.type()->id() == arrow::Type::BOOL) {
+        if (filter_expr.has_value()) {
+          JniThrow("Only one filter expression may be provided");
+        }
+        filter_expr = named_expression.expression;
+      }


Throw if the expression is not of type BOOL.

lidavidm · 2023-09-14T18:52:23Z

docs/source/java/substrait.rst

+            } catch (Exception e) {
+                e.printStackTrace();
+            }


just declare everything as throws Exception

lidavidm · 2023-09-14T18:53:13Z

docs/source/java/substrait.rst

+        private static ByteBuffer getSubstraitExpressionProjection() {
+            // Expression: N_REGIONKEY + 10 = col 3 + 10
+            Expression.Builder selectionBuilderProjectOne = Expression.newBuilder().
+                    setSelection(
+                            Expression.FieldReference.newBuilder().
+                                    setDirectReference(
+                                            Expression.ReferenceSegment.newBuilder().
+                                                    setStructField(
+                                                            Expression.ReferenceSegment.StructField.newBuilder().setField(
+                                                                    2)
+                                                    )
+                                    )
+                    );


nit: how are you formatting examples? I think it would make sense to just use google-java-format which has a more compact style. In our docs, going too far to the right is unreadable.

Co-authored-by: David Li <li.davidm96@gmail.com>

…pacheGH-34252

lidavidm · 2023-09-15T20:58:14Z

docs/source/java/substrait.rst

+        //String uri = "file:///Users/dsusanibar/data/tpch_parquet/nation.parquet";
+        String uri = "file:////Users/dsusanibar/voltron/fork/consumer-testing/tests/data/tpch_parquet/nation.parquet";


Can we not put company names in strings? And remove the redundant comment.

Sorry, second time same error

Just updated

conbench-apache-arrow · 2023-09-20T23:35:24Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 00481a2.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

…der::Filter as a Substrait proto extended expression (apache#35570) ### Rationale for this change To close apache#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>

davisusanibar and others added 30 commits March 13, 2023 11:36

feat: consume Substrait Plan

a0aac46

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

fix: detected both log4j-over-slf4j.jar AND bound slf4j-reload4j.jar …

223ddef

…on the class path

fix: rollback changes on orc

795e619

Merge branch 'main' into poc-substrait

3bd18f1

fix: able to compile main source with jdk8 and test with jdk11

088a101

fix: able to compile main source with jdk8 and test with jdk11

ba23e44

fix: JAVA_HOME_11_X64: command not found

8655815

fix: partial comments fix

d22d6b1

Update java/dataset/src/main/cpp/jni_util.h

f0d8a25

Co-authored-by: David Li <li.davidm96@gmail.com>

Update java/dataset/src/main/java/org/apache/arrow/dataset/substrait/…

632f90d

…SubstraitConsumer.java Co-authored-by: David Li <li.davidm96@gmail.com>

fix: comments

9437f4e

fix: comments

61d6ee7

fix: comments

64c7607

fix: hash boost_1_81_0 does not match expected value

721fe01

fix: maven-shade-plugin:jar:3.1.1 -> org.ow2.asm:asm:jar:6.0: Failed …

b3c2e1e

…to read artifact

Merge branch 'main' into poc-substrait

f5596c9

Merge branch 'main' into poc-substrait

388446b

fix: clean unit test, fix comments

ead80a8

fix: clean substrait method to get plan

0446453

fix: clean sout

8c57c16

fix: rollback maven-shade-plugin

766b383

fix: failures test

5e8b887

fix: delete methods not needed, create files of substrait plan

7f59fbd

fix: npe read resources

0d2bcf8

fix: add resources files for nosuchfile error

4380932

fix: code review

ddd1ad0

danepitkin reviewed Sep 7, 2023

View reviewed changes

fix: code review

9900315

danepitkin mentioned this pull request Sep 11, 2023

ARROW-17351: [C++][Compute] Implement a parser for Expressions #14287

Closed

danepitkin reviewed Sep 12, 2023

View reviewed changes

davisusanibar added 2 commits September 12, 2023 10:02

Merge branch 'main' into apacheGH-34252

5a4f462

fix: expect valid objects for projection and filter

4de37a3

lidavidm requested changes Sep 14, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 14, 2023

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

173dbec

Co-authored-by: David Li <li.davidm96@gmail.com>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 14, 2023

davisusanibar added 3 commits September 14, 2023 15:20

Merge branch 'main' into apacheGH-34252

a3ddae6

Merge branch 'apacheGH-34252' of github.com:davisusanibar/arrow into a…

dadc809

…pacheGH-34252

fix: code review

0c292cf

davisusanibar requested a review from lidavidm September 15, 2023 13:07

lidavidm reviewed Sep 15, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 15, 2023

fix: code review

faadc50

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 15, 2023

lidavidm approved these changes Sep 20, 2023

View reviewed changes

lidavidm merged commit 00481a2 into apache:main Sep 20, 2023
17 checks passed

lidavidm removed the awaiting change review Awaiting change review label Sep 20, 2023

github-actions bot added the awaiting merge Awaiting merge label Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-34252: [Java] Support ScannerBuilder::Project or ScannerBuilder::Filter as a Substrait proto extended expression #35570

GH-34252: [Java] Support ScannerBuilder::Project or ScannerBuilder::Filter as a Substrait proto extended expression #35570

davisusanibar commented May 12, 2023 •

edited

Loading

danepitkin Sep 7, 2023

davisusanibar Sep 7, 2023

danepitkin Sep 12, 2023 •

edited

Loading

davisusanibar Sep 12, 2023

danepitkin Sep 12, 2023 •

edited

Loading

lidavidm Sep 14, 2023

lidavidm Sep 14, 2023

davisusanibar Sep 15, 2023

lidavidm Sep 14, 2023

davisusanibar Sep 15, 2023

lidavidm Sep 14, 2023

davisusanibar Sep 15, 2023

lidavidm Sep 14, 2023

davisusanibar Sep 15, 2023

lidavidm Sep 15, 2023

davisusanibar Sep 15, 2023

davisusanibar Sep 15, 2023

conbench-apache-arrow bot commented Sep 20, 2023

		//String uri = "file:///Users/dsusanibar/data/tpch_parquet/nation.parquet";
		String uri = "file:////Users/dsusanibar/voltron/fork/consumer-testing/tests/data/tpch_parquet/nation.parquet";

GH-34252: [Java] Support ScannerBuilder::Project or ScannerBuilder::Filter as a Substrait proto extended expression #35570

GH-34252: [Java] Support ScannerBuilder::Project or ScannerBuilder::Filter as a Substrait proto extended expression #35570

Conversation

davisusanibar commented May 12, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danepitkin Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danepitkin Sep 12, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Sep 20, 2023

davisusanibar commented May 12, 2023 •

edited

Loading

danepitkin Sep 12, 2023 •

edited

Loading

danepitkin Sep 12, 2023 •

edited

Loading