namanjain24-sudo opened a new pull request, #25090:
URL: https://github.com/apache/datafusion/pull/25090
## Which issue does this PR close?
- Closes #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. Window functions, `from_like`
and
`from_in_list` still omit `output_type` and are left for follow-up work, as
in #20597.
## 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
