fix(core)!: correct SUM and SUM0 helper results

[bvolpato]

The field overload of sum0 currently emits sum, so an empty input returns null rather than zero. SUM also retains narrow input types instead of using the declared wider result, and floating-point SUM0 incorrectly declares i64.

For an i32 input, these helpers must emit nullable i64 for SUM and the sum0 function with required i64 for SUM0:

SubstraitBuilder builder = new SubstraitBuilder();
NamedScan scan = builder.namedScan(
    List.of("t"), List.of("v"), List.of(TypeCreator.REQUIRED.I32));
builder.sum(scan, 0).getFunction().outputType();
builder.sum0(scan, 0).getFunction().declaration().name();

Delegate the field overload to sum0 and derive the output type in the shared arithmetic helper from its function declaration. This follows spec v0.102.0: integer sums use i64, floating-point sums use fp64, SUM is nullable, and SUM0 is required. Standard MIN/MAX/AVG results retain their existing types.

BREAKING CHANGE: All five helpers (min, max, avg, sum, and sum0) now derive output types from the supplied extension catalog, so custom catalog return declarations can change the result schema. With the default catalog, SUM over i8/i16/i32 or fp32 now reports the wider type, and floating-point SUM0 reports fp64 instead of i64. Callers that depend on the previous output schemas must use the returned types. The sum0(Rel, int) overload now emits sum0 with its required result type instead of sum with a nullable result. The Spark module currently lacks a sum0 mapping, so plans using this overload cannot be converted to Spark until that mapping is added.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 52 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: substrait-io/substrait-java/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: f57e5c85-622f-4585-801e-687719233c92

📥 Commits

Reviewing files that changed from the base of the PR and between bc050d3 and 02da910.

📒 Files selected for processing (3)

• core/src/main/java/io/substrait/dsl/SubstraitBuilder.java
• core/src/test/java/io/substrait/dsl/SubstraitBuilderAggregateTest.java
• isthmus/src/test/java/io/substrait/isthmus/AggregationFunctionsTest.java

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[nielspardon]

The builder change matches the spec, but floating-point sum0 now breaks isthmus SQL output — see the inline comment. Everything else is minor and can follow once that's settled.

[bvolpato]

Picking up the upstream fix resolved the SUM0 conversion issue. I added floating-point coverage for both helper overloads through Calcite, PostgreSQL, and Spark SQL conversion, checking that they retain COALESCE(SUM(...), 0).

[nielspardon]

Thanks — the SUM0 fix via #1347 works. Please extend the BREAKING CHANGE: footer, since it's the only part of the body that reaches the release notes. It should say three things: all five helpers now take their output type from the supplied catalog (the commit footer said this; the PR body moved it under Downsides), sum0(Rel, int) changes from a nullable sum to a required sum0, and the Spark module has no sum0 mapping, so builder plans using that overload no longer convert to Spark.

[bvolpato]

Thanks for the detailed pass. I added explicit min/max/avg function-name assertions and routed return-type evaluation through FunctionBindingResolver, so failures include the function anchor. The breaking-change note now covers all five catalog-driven helpers, the required sum0 result, and the current Spark mapping limitation.