fix: Set Substrait output_type on aggregate functions - #25090
Open
namanjain24-sudo wants to merge 1 commit into
Open
fix: Set Substrait output_type on aggregate functions#25090namanjain24-sudo wants to merge 1 commit into
namanjain24-sudo wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
The Substrait producer exported every aggregate call with
output_type: None, eventhough the logical plan already knows the result type. Per the Substrait spec,
AggregateFunction.output_typecarries the return type derived from the referencedfunction 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_typeforBinaryExprand other scalar functions.What changes are included in this PR?
from_aggregate_functionnow derives the output field from the logical expression(
Expr::AggregateFunction(..).to_field(schema)) and writes it toAggregateFunction.output_typeviato_substrait_type_from_field, which is the samepath 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_likeandfrom_in_liststill omitoutput_typeand are left for follow-up work, as in #20597.What is the testing strategy for this PR?
A new unit test
aggregate_function_output_typeindatafusion/substrait/src/logical_plan/producer/expr/aggregate_function.rscovers thefour aggregates from the issue report and asserts both the type and its nullability:
output_typecount(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-typesprobe fromsubstrait-conformance-cases), which now reports:down from
{"cases":4,"missing_output_types":4}. The existingdatafusion-substraitsuite (including the roundtrip tests) passes unchanged.
Are there any user-facing changes?
Substrait plans produced by DataFusion now declare
output_typeon aggregate calls.There are no public API changes.