feat: substrait builder extra api by mbwhite · Pull Request #773 · substrait-io/substrait-java

mbwhite · 2026-03-23T11:56:35Z

Follow on to pr #767 to fill some missing gaps in the substrait builder. We've recently used the Substrait Builder and found that there some variants and missing gaps in the APIs. We'd subclassed it to add these, so I'm now wanting to push these to the main codebase.

There wasn't a unit test already so I've added a basic one here - which I appreciate will is quite large.

github-actions · 2026-03-23T11:56:53Z

ACTION NEEDED

Substrait follows the Conventional Commits
specification for
release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

Signed-off-by: MBWhite <whitemat@uk.ibm.com>

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java

Signed-off-by: MBWhite <whitemat@uk.ibm.com>

benbellick

Just a few comments. Thanks!

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java

mbwhite · 2026-03-25T09:21:22Z

@benbellick updated as suggested :-)

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java

Signed-off-by: MBWhite <whitemat@uk.ibm.com>

nielspardon · 2026-04-10T08:25:53Z

@benbellick since you started this review, do you have time for another look?

benbellick

Left a few more comments. Thanks!

benbellick · 2026-04-10T13:12:10Z

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java

+   * @param expression the boolean expression to negate
+   * @return a scalar function invocation representing the logical NOT of the input expression
+   */
+  public Expression not(Expression expression) {


Slightly relevant issue I just found FYI substrait-io/substrait#1041

Also and and or above return Expression.ScalarFunctionInvocation, but this one returns the more general Expression. Can we return Expression.ScalarFunctionInvocation to be consistent?

Also the nullability should be assumed to be MIRROR rather than DECLARED_OUTPUT to be consistent with and and or above. So I think we should check nullability here and only mark the output as nullable if the input is.

benbellick · 2026-04-10T13:13:42Z

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java

+  public Expression scalarFn(
+      String urn,
+      String key,
+      Type returnType,


Thinking out loud here, but does this builder validate that the returnType is actually possible for this function?

benbellick · 2026-04-10T13:15:40Z

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java

  }

+  /**
+   * Creates a scalar function invocation with function options.


This says

with function options

but only the above method accepts options AFAICT via optionsList, and this one does not. Maybe we just get rid of this whole one to keep things simple?

benbellick · 2026-04-10T13:16:30Z

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java

+   * @return a new {@link Plan}
+   */
+  public Plan.Root root(final Rel input, final List<String> names) {
+    return Plan.Root.builder().input(input).names(names).build();


Also curious if this one validates that the length of names matches the schema of input.

benbellick · 2026-04-10T13:17:39Z

core/src/test/java/io/substrait/dsl/SubstraitBuilderTest.java

+      final Expression.I32Literal input = builder.i32(1);
+      final Expression cast = builder.cast(input, R.I64);
+      assertNotNull(cast);
+      assertTrue(cast instanceof Expression.Cast);


Might be better to actually check that the type of this expression is now R.I64.

benbellick · 2026-04-10T13:43:23Z

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java

+        DefaultExtensionCatalog.FUNCTIONS_COMPARISON,
+        "is_null:any",
+        TypeCreator.REQUIRED.BOOLEAN,
+        args,


nit: could just simplify this to the below and drop the above construction of an ArrayList

Suggested change

args,

List.of(expression),

benbellick · 2026-04-10T13:43:56Z

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java

+   * @param input the root relational expression of the query plan
+   * @param names the ordered list of output column names corresponding to the input relation's
+   *     output schema
+   * @return a new {@link Plan}


Suggested change

* @return a new {@link Plan}

* @return a new {@link Plan.root}

benbellick · 2026-04-10T13:44:22Z

core/src/test/java/io/substrait/dsl/SubstraitBuilderTest.java

+
+      final AggregateFunctionInvocation afi1 =
+          builder.aggregateFn(
+              "extension:io.substrait:functions_aggregate_generic",


nit:

Suggested change

"extension:io.substrait:functions_aggregate_generic",

DefaultExtensionCatalog.FUNCTIONS_AGGREGATE_GENERIC,

benbellick · 2026-04-10T13:44:55Z

core/src/test/java/io/substrait/dsl/SubstraitBuilderTest.java

+      Fetch limit = builder.limit(10, scan);
+      assertEquals(10, limit.getCount().getAsLong());
+      assertEquals(0, limit.getOffset());
+      limit = builder.limit(10, Remap.of(Arrays.asList(new Integer[] {0, 1})), scan);


nit:

Suggested change

limit = builder.limit(10, Remap.of(Arrays.asList(new Integer[] {0, 1})), scan);

limit = builder.limit(10, Remap.of(List.of(0, 1)), scan);

benbellick · 2026-04-10T13:50:13Z

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java

+   * @param value value to create
+   * @return i64 instance
+   */
+  public Expression.I64Literal i64(long value) {


nit: all of the other existing methods use v for the parameter name. Should we do the same for all of the new ones to be consistent?

mbwhite force-pushed the additional-substrait-builder branch from bd62615 to d0cc543 Compare March 23, 2026 11:58

mbwhite mentioned this pull request Mar 23, 2026

docs: update substrait builder javadoc and spelling #767

Merged

mbwhite changed the title ~~Additional substrait builder~~ feat: substrait builder extra api Mar 23, 2026

mbwhite force-pushed the additional-substrait-builder branch from d0cc543 to 6c079f0 Compare March 23, 2026 12:50

Update substrait builder

af52a69

Signed-off-by: MBWhite <whitemat@uk.ibm.com>

mbwhite force-pushed the additional-substrait-builder branch 2 times, most recently from 8618365 to 434df46 Compare March 24, 2026 10:29

mbwhite marked this pull request as ready for review March 24, 2026 10:34

feat: substrait builder extra apifeat: substrait builder extra api

83fa865

Signed-off-by: MBWhite <whitemat@uk.ibm.com>

mbwhite force-pushed the additional-substrait-builder branch from 434df46 to 83fa865 Compare March 24, 2026 11:50

benbellick reviewed Mar 24, 2026

View reviewed changes

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java Outdated Show resolved Hide resolved

mbwhite force-pushed the additional-substrait-builder branch 2 times, most recently from 56aaa2a to 5bdace0 Compare March 24, 2026 13:09

review comments

ef5e526

Signed-off-by: MBWhite <whitemat@uk.ibm.com>

mbwhite force-pushed the additional-substrait-builder branch from 5bdace0 to ef5e526 Compare March 24, 2026 13:10

benbellick requested changes Mar 24, 2026

View reviewed changes

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java Outdated Show resolved Hide resolved

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java Show resolved Hide resolved

mbwhite requested a review from benbellick March 25, 2026 11:52

benbellick reviewed Mar 25, 2026

View reviewed changes

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java Outdated Show resolved Hide resolved

mbwhite force-pushed the additional-substrait-builder branch from efabd93 to fe68836 Compare March 25, 2026 13:15

mbwhite requested a review from benbellick March 26, 2026 09:28

benbellick reviewed Mar 26, 2026

View reviewed changes

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java Outdated Show resolved Hide resolved

review comments

1ca14b2

Signed-off-by: MBWhite <whitemat@uk.ibm.com>

mbwhite force-pushed the additional-substrait-builder branch from fe68836 to 1ca14b2 Compare March 27, 2026 11:51

Merge branch 'main' into additional-substrait-builder

acd20a0

mbwhite requested a review from benbellick April 2, 2026 10:27

benbellick reviewed Apr 10, 2026

View reviewed changes

	* @return a new {@link Plan}
	* @return a new {@link Plan.root}

	"extension:io.substrait:functions_aggregate_generic",
	DefaultExtensionCatalog.FUNCTIONS_AGGREGATE_GENERIC,

	limit = builder.limit(10, Remap.of(Arrays.asList(new Integer[] {0, 1})), scan);
	limit = builder.limit(10, Remap.of(List.of(0, 1)), scan);

Conversation

mbwhite commented Mar 23, 2026

Uh oh!

github-actions bot commented Mar 23, 2026

Uh oh!

Uh oh!

benbellick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mbwhite commented Mar 25, 2026

Uh oh!

Uh oh!

Uh oh!

nielspardon commented Apr 10, 2026

Uh oh!

benbellick left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants