fix: Set Substrait output_type on aggregate functions by namanjain24-sudo · Pull Request #25090 · apache/datafusion

namanjain24-sudo · 2026-09-08T19:42:47Z

Which issue does this PR close?

Rationale for this change

The Substrait producer exported aggregate calls with output_type: None. Per spec,
output_type should carry the function's return type — this mirrors the fix already
done for scalar functions in #20597.

Verified against substrait-java 0.103.0: it rejects an aggregate measure with no
output_type (UnsupportedOperationException: Type is not set) and accepts the same
plan once this field is set, recovering the correct type.

What changes are included in this PR?

from_aggregate_function now derives the output field via
Expr::AggregateFunction(..).to_field(schema) and sets it with
to_substrait_type_from_field, the same path scalar functions already use.

What is the testing strategy for this PR?

Added aggregate_function_output_type unit test covering count, sum, avg, min.
Fails on main, passes with this change. Existing datafusion-substrait suite passes
unchanged.

Are there any user-facing changes?

Substrait plans now declare output_type on aggregate calls. No public API changes.

codecov-commenter · 2026-09-11T21:25:21Z

Codecov Report

❌ Patch coverage is 79.31034% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.34%. Comparing base (edc936f) to head (97bdefe).

Files with missing lines	Patch %	Lines
...c/logical_plan/producer/expr/aggregate_function.rs	79.31%	2 Missing and 4 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #25090   +/-   ##
=======================================
  Coverage   82.33%   82.34%           
=======================================
  Files        1137     1137           
  Lines      432498   432526   +28     
  Branches   432498   432526   +28     
=======================================
+ Hits       356116   356149   +33     
+ Misses      54843    54833   -10     
- Partials    21539    21544    +5

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

alamb · 2026-09-13T20:42:10Z

See comment here: #25100 (comment)

The Substrait producer exported every aggregate call with output_type: None, even though the logical plan already knows the result type. Derive the output field from the logical expression and write it to AggregateFunction.output_type, mirroring the handling already used for scalar functions. Closes apache#25049.

alamb · 2026-09-22T15:16:05Z

Maybe @gabotechs or @vbarua can help review this PR or suggest someone who is more focused on the substrait code at this time. I am sorry I don't think I am going to have time to drive this along

gabotechs · 2026-09-22T15:38:45Z

@namanjain24-sudo I see the PR fix is just three lines of code, but the quantity of text in the PR description and your following messages seems disproportionately high for just a 3 LOC fix.

If the text produced as description and messages is AI generated, please, try to review it first and summarize it as much as possible in order to be respectful with reviewers time.

namanjain24-sudo · 2026-09-22T17:17:27Z

Fair point, sorry for the noise — I'll keep it tight going forward. Trimmed the PR description down to the essentials.

gabotechs · 2026-09-23T11:59:59Z

I see some massive comments right after the PR description. As those relevant as well?

If they are relevant, can we summarize them to a reasonable length? or even merge them with just the PR description?
If they are not relevant, can we delete them?

namanjain24-sudo · 2026-09-23T12:04:18Z

Good call. Deleted the workflow-approval ping (stale, checks run now), the substrait-java repro, and the interop-testing tangent — folded the one relevant fact (substrait-java accepts the plan with this fix, rejects it without) into the PR description as one sentence. Left the short apology comment above yours since it's already brief.

gabotechs

Thanks @namanjain24-sudo! the fix is pretty straight forward, +1 on my side

…5367) ## Which issue does this PR close? - Closes apache#25366. ## Rationale for this change The producer left `output_type` unset on window function calls and on `LIKE` / `ILIKE`, including the `not` that wraps a negated one. Substrait documents both `Expression.WindowFunction.output_type` and `Expression.ScalarFunction.output_type` as: > Must be set to the return type of the function, exactly as derived using the declaration in the extension. A consumer that reads the field rejects such a call. substrait-java 0.103.0 refuses both plans, from `ProtoTypeConverter.from:125`: | plan produced for | error before this PR | | --- | --- | | `SELECT sum(i) OVER (ORDER BY i) FROM t` | `UnsupportedOperationException: Type is not set` at `ProtoExpressionConverter.fromWindowFunction:406` | | `SELECT i FROM t WHERE CAST(i AS VARCHAR) LIKE '1%'` | `UnsupportedOperationException: Type is not set` at `ProtoExpressionConverter:201`, via `ProtoRelConverter.newFilter` | A DataFusion round trip cannot catch this: the consumer reads `output_type` only in `consumer/expr/cast.rs`, so producer and consumer agree on the omission. apache#15831 and apache#20597 set this field for binary and unary expressions, `from_function` and higher order functions, and apache#25049 and apache#25090 cover `AggregateFunction`. These were the remaining expression kinds. ## What changes are included in this PR? Both types come from the expression itself, so they match what DataFusion derives rather than being restated in the producer: - `from_window_function` derives the type from `Expr::WindowFunction(..).to_field(schema)` and writes it on the call it builds. `make_substrait_window_function` had one caller and eight parameters once the type was added, so its body now sits in `from_window_function` and the helper is gone. - `from_like` derives the type from `Expr::Like(..).to_field(schema)` and sets it on the `like` call and on the `not` wrapper of a negated one, which yields that same type. `make_substrait_like_expr` had `from_like` as its only caller, so, as with the window helper, its body now sits in `from_like`. ## What is the testing strategy for this PR? - Two unit tests next to the existing `binary_expr_output_type`: `window_function_output_type` asserts `sum(i)` over a nullable `i64` carries a nullable `i64`, and `like_output_type` asserts a `LIKE` over a nullable input carries a nullable boolean, on the `like` call, on the `not` of a negated one, and on the `like` nested inside that `not`. - Each half was reverted on its own to check the tests pin it: without the window change only `window_function_output_type` fails, without the `LIKE` change only `like_output_type` fails. - `cargo test -p datafusion-substrait` passes (60 unit, 210 integration with the 6 that were already ignored, 3 doc tests), and `./ci/scripts/rust_clippy.sh`, the workspace clippy CI runs, is clean. - Emitted protobuf before and after, read directly rather than through a consumer: | plan | before | after | | --- | --- | --- | | window `sum` | `output_type` unset | set | | `like` | unset | set | | `not` around a negated `like` | unset | set | - With the same plans, substrait-java 0.103.0 no longer raises `Type is not set`. Both now get through `ProtoPlanConverter`, while the plans built from `main` still fail there. Each then stops further along for reasons that have nothing to do with this field: substrait-spark has no visitor for a window function that appears in a project expression, and Spark's `like` takes two arguments while the call we emit passes three, the third being the escape character. That second one is apache#25442, which emits `LIKE` in the two-argument form the extension defines. ## Are there any user-facing changes? No API changes. Plans produced for window functions and `LIKE` now carry the expression's type, which consumers that require it will accept.

## Which issue does this PR close? Closes apache#25049. ## Rationale for this change The Substrait producer exported aggregate calls with `output_type: None`. Per spec, `output_type` should carry the function's return type — this mirrors the fix already done for scalar functions in apache#20597. Verified against substrait-java 0.103.0: it rejects an aggregate measure with no `output_type` (`UnsupportedOperationException: Type is not set`) and accepts the same plan once this field is set, recovering the correct type. ## What changes are included in this PR? `from_aggregate_function` now derives the output field via `Expr::AggregateFunction(..).to_field(schema)` and sets it with `to_substrait_type_from_field`, the same path scalar functions already use. ## What is the testing strategy for this PR? Added `aggregate_function_output_type` unit test covering `count`, `sum`, `avg`, `min`. Fails on `main`, passes with this change. Existing `datafusion-substrait` suite passes unchanged. ## Are there any user-facing changes? Substrait plans now declare `output_type` on aggregate calls. No public API changes.

…5367) ## Which issue does this PR close? - Closes apache#25366. ## Rationale for this change The producer left `output_type` unset on window function calls and on `LIKE` / `ILIKE`, including the `not` that wraps a negated one. Substrait documents both `Expression.WindowFunction.output_type` and `Expression.ScalarFunction.output_type` as: > Must be set to the return type of the function, exactly as derived using the declaration in the extension. A consumer that reads the field rejects such a call. substrait-java 0.103.0 refuses both plans, from `ProtoTypeConverter.from:125`: | plan produced for | error before this PR | | --- | --- | | `SELECT sum(i) OVER (ORDER BY i) FROM t` | `UnsupportedOperationException: Type is not set` at `ProtoExpressionConverter.fromWindowFunction:406` | | `SELECT i FROM t WHERE CAST(i AS VARCHAR) LIKE '1%'` | `UnsupportedOperationException: Type is not set` at `ProtoExpressionConverter:201`, via `ProtoRelConverter.newFilter` | A DataFusion round trip cannot catch this: the consumer reads `output_type` only in `consumer/expr/cast.rs`, so producer and consumer agree on the omission. apache#15831 and apache#20597 set this field for binary and unary expressions, `from_function` and higher order functions, and apache#25049 and apache#25090 cover `AggregateFunction`. These were the remaining expression kinds. ## What changes are included in this PR? Both types come from the expression itself, so they match what DataFusion derives rather than being restated in the producer: - `from_window_function` derives the type from `Expr::WindowFunction(..).to_field(schema)` and writes it on the call it builds. `make_substrait_window_function` had one caller and eight parameters once the type was added, so its body now sits in `from_window_function` and the helper is gone. - `from_like` derives the type from `Expr::Like(..).to_field(schema)` and sets it on the `like` call and on the `not` wrapper of a negated one, which yields that same type. `make_substrait_like_expr` had `from_like` as its only caller, so, as with the window helper, its body now sits in `from_like`. ## What is the testing strategy for this PR? - Two unit tests next to the existing `binary_expr_output_type`: `window_function_output_type` asserts `sum(i)` over a nullable `i64` carries a nullable `i64`, and `like_output_type` asserts a `LIKE` over a nullable input carries a nullable boolean, on the `like` call, on the `not` of a negated one, and on the `like` nested inside that `not`. - Each half was reverted on its own to check the tests pin it: without the window change only `window_function_output_type` fails, without the `LIKE` change only `like_output_type` fails. - `cargo test -p datafusion-substrait` passes (60 unit, 210 integration with the 6 that were already ignored, 3 doc tests), and `./ci/scripts/rust_clippy.sh`, the workspace clippy CI runs, is clean. - Emitted protobuf before and after, read directly rather than through a consumer: | plan | before | after | | --- | --- | --- | | window `sum` | `output_type` unset | set | | `like` | unset | set | | `not` around a negated `like` | unset | set | - With the same plans, substrait-java 0.103.0 no longer raises `Type is not set`. Both now get through `ProtoPlanConverter`, while the plans built from `main` still fail there. Each then stops further along for reasons that have nothing to do with this field: substrait-spark has no visitor for a window function that appears in a project expression, and Spark's `like` takes two arguments while the call we emit passes three, the third being the escape character. That second one is apache#25442, which emits `LIKE` in the two-argument form the extension defines. ## Are there any user-facing changes? No API changes. Plans produced for window functions and `LIKE` now carry the expression's type, which consumers that require it will accept.

github-actions Bot added the substrait Changes to the substrait crate label Sep 8, 2026

namanjain24-sudo force-pushed the fix-substrait-aggregate-output-type branch 4 times, most recently from 3cc32f3 to 3616533 Compare September 11, 2026 17:22

alexandrefimov mentioned this pull request Sep 11, 2026

Substrait producer omits the required output_type on aggregate functions #25049

Closed

namanjain24-sudo mentioned this pull request Sep 11, 2026

Substrait producer emits AGGREGATION_PHASE_UNSPECIFIED for every aggregate and window function #25100

Open

namanjain24-sudo force-pushed the fix-substrait-aggregate-output-type branch from 3616533 to 8d5511a Compare September 14, 2026 11:43

This was referenced Sep 16, 2026

Substrait producer omits output_type on window functions and LIKE expressions #25366

Closed

fix: Set Substrait output_type on window functions and LIKE #25367

Merged

fix: Set Substrait aggregation phase to INITIAL_TO_RESULT #25146

Open

namanjain24-sudo force-pushed the fix-substrait-aggregate-output-type branch from 8d5511a to 97bdefe Compare September 17, 2026 23:07

gabotechs approved these changes Sep 23, 2026

View reviewed changes

gabotechs added this pull request to the merge queue Sep 23, 2026

Merged via the queue into apache:main with commit ed2cc50 Sep 23, 2026
44 checks passed

nuno-faria added the v56.0.0 label Oct 3, 2026

Navigation Menu

Repository navigation

fix: Set Substrait output_type on aggregate functions - #25090

fix: Set Substrait output_type on aggregate functions#25090
gabotechs merged 1 commit into
apache:mainfrom
namanjain24-sudo:fix-substrait-aggregate-output-type

namanjain24-sudo commented Sep 8, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Sep 11, 2026 •

edited

Loading

Uh oh!

alamb commented Sep 13, 2026

Uh oh!

alamb commented Sep 22, 2026

Uh oh!

gabotechs commented Sep 22, 2026 •

edited

Loading

Uh oh!

namanjain24-sudo commented Sep 22, 2026

Uh oh!

gabotechs commented Sep 23, 2026

Uh oh!

namanjain24-sudo commented Sep 23, 2026

Uh oh!

gabotechs left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

namanjain24-sudo commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alamb commented Sep 13, 2026

Uh oh!

alamb commented Sep 22, 2026

Uh oh!

gabotechs commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namanjain24-sudo commented Sep 22, 2026

Uh oh!

gabotechs commented Sep 23, 2026

Uh oh!

namanjain24-sudo commented Sep 23, 2026

Uh oh!

gabotechs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

namanjain24-sudo commented Sep 8, 2026 •

edited

Loading

codecov-commenter commented Sep 11, 2026 •

edited

Loading

gabotechs commented Sep 22, 2026 •

edited

Loading