Repository navigation
fix: cast outer projection expr to declared field type in SingleDistinctToGroupBy - #26211
jdymitarai wants to merge 1 commit into
Conversation
|
Hi @jdymitarai, thank you for your contribution. The code looks good to me. I traced the changes by hand, and ran a review with Claude. The test you added explicitly tests for this regression, and the test passes with your changes and fails without them. One thing needs to change before I approve. The PR description needs to follow the DataFusion PR template and answer all the questions in it. Once the description is sorted, I'll hit approve! |
|
Thanks @HairstonE. I updated the pull request description to follow the DataFusion PR template format across all required sections. |
|
@jdymitarai Two small nits before I approve. The testing section references |
|
Good catch @HairstonE. I updated the PR description to fix the filename to single_distinct_to_groupby.rs and explicitly documented the user-facing type change for Decimal expressions rewritten by this rule. |
Which issue does this PR close?
Closes #26193.
Rationale for this change
When the SingleDistinctToGroupBy optimizer rule rewrites an aggregate combining DISTINCT and non-distinct expressions such as sum over Decimal128, the non-distinct expression is split into inner and outer phases. The outer phase evaluates sum over the already-widened Decimal output of the inner aggregation, which widens precision a second time and causes the resulting schema to diverge from the original aggregate schema.
What changes are included in this PR?
The outer projection construction in SingleDistinctToGroupBy now checks whether the data type produced by outer_aggr differs from the field type declared in the original schema. When a mismatch occurs, the projected expression is wrapped with a cast back to the declared field type, ensuring schema consistency across optimizer passes.
Are these changes tested?
Covered by a new regression unit test in single_distinct_to_groupby.rs verifying that two-phase decimal precision expansion preserves the original schema.
Are there any user-facing changes?
Yes, aggregate queries rewritten by SingleDistinctToGroupBy containing Decimal expressions (such as sum over Decimal128) now retain their original declared Decimal precision and scale rather than producing an unintentionally expanded Decimal type.