fix: Set Substrait output_type on aggregate functions

[namanjain24-sudo]

Which issue does this PR close?

• Closes Substrait producer omits the required output_type on aggregate functions #25049.

Rationale for this change

The Substrait producer exported every aggregate call with output_type: None, even
though the logical plan already knows the result type. Per the Substrait spec,
AggregateFunction.output_type carries the return type derived from the referenced
function declaration. A consumer that validates required fields can reject such plans,
and a consumer that relies on the declaration for schema inference has no type to use.

This mirrors #15831 / #20597, which fixed the same missing output_type for
BinaryExpr and other scalar functions.

What changes are included in this PR?

from_aggregate_function now derives the output field from the logical expression
(Expr::AggregateFunction(..).to_field(schema)) and writes it to
AggregateFunction.output_type via to_substrait_type_from_field, which is the same
path already used for scalar functions.

Because the type is now converted rather than dropped, an aggregate whose return type
cannot be represented in Substrait produces an error instead of silently emitting a
call with no declared type. That is the behaviour the issue asks for ("write a
conforming type, or report that it cannot represent that function contract").

The change is limited to aggregate functions. The sites that still omit output_type are
window functions (window_function.rs:110), the LIKE and NOT LIKE paths
(scalar_function.rs:296 and :312) and the not() wrapper in utils.rs:106; they are left
for follow-up work, as in #20597. (from_in_list is not one of them: it emits
SingularOrList, which has no output_type field.)

What is the testing strategy for this PR?

A new unit test aggregate_function_output_type in
datafusion/substrait/src/logical_plan/producer/expr/aggregate_function.rs covers the
four aggregates from the issue report and asserts both the type and its nullability:

Query	Declared output_type
count(i)	Int64 (non-nullable)
sum(i)	Int64 (nullable)
avg(i)	Float64 (nullable)
min(i)	Int64 (nullable)

The test fails on main (left: None) and passes with this change.

I also ran the reproducer from the issue (the --aggregate-output-types probe from
substrait-conformance-cases), which now reports:

{"function":"count","logical_type":"Int64","declaration":"count -> i64","has_output_type":true}
{"function":"sum","logical_type":"Int64","declaration":"sum -> i64","has_output_type":true}
{"function":"avg","logical_type":"Float64","declaration":"avg -> fp64","has_output_type":true}
{"function":"min","logical_type":"Int64","declaration":"min -> i64","has_output_type":true}
{"cases":4,"missing_output_types":0}

down from {"cases":4,"missing_output_types":4}. The existing datafusion-substrait
suite (including the roundtrip tests) passes unchanged.

Are there any user-facing changes?

Substrait plans produced by DataFusion now declare output_type on aggregate calls.
There are no public API changes.

[namanjain24-sudo]

@alamb could a committer approve the workflows on this PR and on #25091? Both were opened on 2026-09-08 and neither has had a single check run since. The one green mark on each is the Process labeler — Rust, Dev, CodeQL, Dependencies, Detect breaking changes and Large files PR check are all sitting at action_required.

I asked on #25191 a few hours ago rather than pinging the same reviewers repeatedly, so I'm folding these two into one request here instead of opening a third thread.

Both are rebased onto current main, no conflicts, one commit each. On the rebased commits, cargo test -p datafusion-substrait passes on both — 272 tests, 0 failures.

They are small and independent:

• This PR (fix: Set Substrait output_type on aggregate functions #25090, closes Substrait producer omits the required output_type on aggregate functions #25049) — one file, +53/−3. Sets AggregateFunction.output_type from the logical expression's field, the same path scalar functions already use.
• fix: Derive Substrait intersection nullability from every input #25091 (closes Substrait intersection schemas retain primary-input nullability #25042) — narrows intersection output nullability to the AND of the inputs, per the Substrait set-operation rules.

Happy to rework either if you'd prefer a different approach. And if approving workflows on fork PRs is itself part of what #25148 is trying to reduce, I'm fine with these waiting until there's a decision there — just wanted to flag that they aren't stalled on anything I can fix from my side.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 79.31034% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.93%. Comparing base (f8cc678) to head (3616533).

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   81.93%   81.93%   -0.01%     
==========================================
  Files        1133     1133              
  Lines      423529   423557      +28     
  Branches   423529   423557      +28     
==========================================
+ Hits       347028   347049      +21     
- Misses      55910    55912       +2     
- Partials    20591    20596       +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.

[namanjain24-sudo]

Fair question, and my PR description was the reason for it — it argued from the spec and only said a
consumer "can reject such plans", without naming one. Here is the concrete failure, measured rather
than asserted.

The engine is substrait-java (io.substrait:core:0.103.0). It reads the field unconditionally:

// ProtoAggregateFunctionConverter.java:82
.outputType(protoTypeConverter.from(measure.getOutputType()))

// ProtoTypeConverter.java:125
case KIND_NOT_SET:
  throw new UnsupportedOperationException("Type is not set: " + type);

There is no hasOutputType() guard on that path, so every aggregate measure we export hits it.

Reproduction, with no DataFusion consumer in the loop. I built SELECT sum(i) FROM t over one
non-null i64 column, called to_substrait_plan, wrote the protobuf to a file, and handed that file
to substrait-java's ProtoPlanConverter.

One wrinkle that matters if you try this, and that I should have led with: a DataFusion plan as
emitted today does not even reach the output_type check. It fails earlier with

IllegalStateException: Function 'sum' references URN anchor -1, but no URN is registered at that anchor

because we still write extension_urn_reference: u32::MAX for every function declaration
(extensions.rs:120), which is the known gap tracked in #11545. To isolate this bug I registered
extension:io.substrait:functions_arithmetic on the plan and used the compound key sum:i64. I
changed nothing else in either plan.

plan	#11545 worked around	substrait-java 0.103.0
main	no	IllegalStateException: … URN anchor -1
main	yes	UnsupportedOperationException: Type is not set:
this PR	no	IllegalStateException: … URN anchor -1
this PR	yes	accepted — Struct{nullable=false, fields=[I64{nullable=true}]}

So output_type is a second, independent gate sitting behind #11545. Fixing this alone does not
make DataFusion plans readable by substrait-java. Leaving it unfixed means they still will not be
once #11545 is. The type substrait-java recovers, nullable i64, is exactly what the optimized
DataFusion plan's own schema says sum(t.i) is, so the value written is right and not merely
present.

Why nothing in our suite catches it. Our consumer never reads this field — the only output_type
read anywhere under logical_plan/consumer/ is in cast.rs. Producer and consumer therefore agree
on a shape the spec does not describe, and every DataFusion-to-DataFusion round trip passes.

Spec wording, identical in the pinned substrait 0.63.0 crate, in upstream v0.87.0 and on
main:

Must be set to the return type of the function, exactly as derived using the declaration in the
extension.

Precedent. This is the same defect and the same fix as #15831, fixed by #20597, which set
output_type from Expr::ScalarFunction(..).to_field(schema) for scalar functions. This PR uses
that identical path for aggregates.

One correction to my own description while I am here: it lists from_in_list as a remaining
follow-up site, which is wrong. from_in_list emits SingularOrList, which has no output_type
field at all. The sites that genuinely still omit it are window functions
(window_function.rs:110), the LIKE and NOT LIKE paths (scalar_function.rs:296 and :312), and
the not() wrapper in utils.rs:106. I have fixed that sentence in the description.

I will answer the same question on #25100 and #25190 on their own threads. One clarification about
#25100, since I had it half wrong when I first wrote this: substrait-java's core reader does not
throw on AGGREGATION_PHASE_UNSPECIFIED, but its Spark converter maps it to Spark's Final
(ToAggregateFunction.scala:77, their own comment: UNSPECIFIED implies INTERMEDIATE_TO_RESULT),
while INITIAL_TO_RESULT maps to Complete. So that one is not merely a spec argument either — it
is a silent misreading rather than an exception, which is arguably the worse failure of the two.