Vanzeren opened a new pull request, #25803:
URL: https://github.com/apache/datafusion/pull/25803
## Which issue does this PR close?
- Closes #25796
## Rationale for this change
`array_agg(x ORDER BY y)`, `string_agg(x, d ORDER BY y)`, `first_value`,
`last_value`, and any other aggregate whose ordering is written inside the
argument list, lost that ordering when unparsed. Only aggregates using
`WITHIN GROUP` consumed `params.order_by`; every other aggregate emitted an
empty clause list.
The unparsed SQL stays valid, so it runs without error and quietly returns
different results. Any tool that forwards unparsed SQL to another engine
(for example `datafusion-federation`) gets wrong answers with no warning.
## What changes are included in this PR?
`Expr::AggregateFunction` unparsing now decides the spelling of the ordering
once and fills the matching AST slot:
- ordered-set aggregates (`supports_within_group_clause()`: `percentile_cont`
and `approx_percentile_cont*`) keep emitting `WITHIN GROUP (ORDER BY ..)`;
- every other aggregate with a non-empty `order_by` now emits
`ast::FunctionArgumentClause::OrderBy` inside the argument list;
- aggregates without ordering are unchanged.
The `clauses` list is keyed off whether a `WITHIN GROUP` clause is actually
emitted, so the two slots can never both end up empty.
## What is the testing strategy for this PR?
- `roundtrip_ordered_aggregate_order_by` round-trips `last_value`,
`first_value`, `array_agg` and `string_agg` from the issue, plus
`DISTINCT` and a multi-key ordering.
- `ordered_aggregate_order_by_survives_replanning` re-parses and re-plans the
emitted SQL and asserts the plan is unchanged, so a frozen-but-wrong
snapshot cannot hide a dropped ordering. It covers both spellings.
Both tests fail before the change and pass after it.
## Are there any user-facing changes?
Unparsed SQL now preserves the ordering of ordered aggregates instead of
silently dropping it. No API change.
## AI assistance
This change was prepared with AI assistance. Per the project's
AI-assisted contributions policy, the following assumptions are called out
explicitly so reviewers can check them rather than take them on trust:
- The `clauses` slot is keyed off whether a `WITHIN GROUP` clause is
actually emitted, rather than off `supports_within_group_clause()` alone.
A function that reports within-group support but reaches the
argument-list path therefore cannot end up with both slots empty.
- `f(x ORDER BY y)` is emitted unconditionally, with no dialect hook.
MySQL cannot express these functions anyway (`ARRAY_AGG` is
`JSON_ARRAYAGG` there), so nothing that previously worked regresses, but
a dialect guard is deliberately not part of this PR.
- `null_treatment` at the same site remains hardcoded to `None`, so
`IGNORE NULLS` is still dropped (see #25462). Kept out of scope rather
than mixing two fixes into one PR.
- sqlparser parses argument-list clauses in a fixed order (`WHERE` ->
`IGNORE|RESPECT NULLS` -> `ORDER BY` -> `LIMIT`). Emitting only
`ORDER BY` is safe today, but that order must be respected if
`null_treatment` is emitted later.
- The re-planning test compares `display_indent()` output rather than
structural plan equality. It catches a dropped or relocated ordering but
is not a general plan-equivalence check.
--
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]
