You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by gi...@apache.org on 2026/09/29 15:47:45 UTC
(datafusion) branch gh-readonly-queue/main/pr-25803-bae36eb7fab7d9c97338478845b9f2d2e8a35ac9 created (now 2cac713274)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-25803-bae36eb7fab7d9c97338478845b9f2d2e8a35ac9
in repository https://gitbox.apache.org/repos/asf/datafusion.git
at 2cac713274 fix: unparser drops ORDER BY of ordered aggregates (#25803)
This branch includes the following new commits:
new 2cac713274 fix: unparser drops ORDER BY of ordered aggregates (#25803)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org
(datafusion) 01/01: fix: unparser drops ORDER BY of ordered aggregates (#25803)
Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-25803-bae36eb7fab7d9c97338478845b9f2d2e8a35ac9
in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit 2cac713274496ba8ee08e58655e80f0d464f6d48
Author: Zeren Wang <53...@users.noreply.github.com>
AuthorDate: Tue Sep 29 15:47:35 2026 +0000
fix: unparser drops ORDER BY of ordered aggregates (#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.
---------
Co-authored-by: Neil Conway <ne...@gmail.com>
---
datafusion/sql/src/unparser/expr.rs | 35 ++++++++++-----
datafusion/sql/tests/cases/plan_to_sql.rs | 75 ++++++++++++++++++++++++++++++-
2 files changed, 97 insertions(+), 13 deletions(-)
diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs
index 0f2cef167c..3fb349f5ed 100644
--- a/datafusion/sql/src/unparser/expr.rs
+++ b/datafusion/sql/src/unparser/expr.rs
@@ -408,18 +408,29 @@ impl Unparser<'_> {
..
} = &agg.params;
- // if this is a WITHIN GROUP aggregate, skip the prepended arg
- let (args_to_use, within_group) =
- if agg.func.supports_within_group_clause() && !order_by.is_empty() {
- let args_to_use = self.function_args_to_sql(&args[1..])?;
- let within_group = order_by
- .iter()
- .map(|sort_expr| self.sort_to_sql(sort_expr))
- .collect::<Result<Vec<ast::OrderByExpr>>>()?;
- (args_to_use, within_group)
+ // An aggregate's `order_by` is spelled one of two ways in SQL:
+ // `WITHIN GROUP (ORDER BY ..)` for ordered-set aggregates, and an
+ // `ORDER BY` clause inside the argument list for every other
+ // aggregate. Both map onto `params.order_by`.
+ let (args_to_use, within_group, clauses) = if order_by.is_empty() {
+ (self.function_args_to_sql(args)?, vec![], vec![])
+ } else {
+ let order_by_sql = order_by
+ .iter()
+ .map(|sort_expr| self.sort_to_sql(sort_expr))
+ .collect::<Result<Vec<ast::OrderByExpr>>>()?;
+ if agg.func.supports_within_group_clause() {
+ // Ordered-set aggregates carry the ordered value as their
+ // first argument, so skip it.
+ (self.function_args_to_sql(&args[1..])?, order_by_sql, vec![])
} else {
- (self.function_args_to_sql(args)?, Vec::new())
- };
+ (
+ self.function_args_to_sql(args)?,
+ vec![],
+ vec![ast::FunctionArgumentClause::OrderBy(order_by_sql)],
+ )
+ }
+ };
let filter = match filter {
Some(filter) => Some(Box::new(self.expr_to_sql_inner(filter)?)),
@@ -435,7 +446,7 @@ impl Unparser<'_> {
duplicate_treatment: distinct
.then_some(DuplicateTreatment::Distinct),
args: args_to_use,
- clauses: vec![],
+ clauses,
}),
filter,
null_treatment: None,
diff --git a/datafusion/sql/tests/cases/plan_to_sql.rs b/datafusion/sql/tests/cases/plan_to_sql.rs
index de22321327..75a536d3ec 100644
--- a/datafusion/sql/tests/cases/plan_to_sql.rs
+++ b/datafusion/sql/tests/cases/plan_to_sql.rs
@@ -241,7 +241,14 @@ fn roundtrip_statement() -> Result<()> {
"SELECT left[1] FROM array",
"SELECT {a:1, b:2}",
"SELECT s.a FROM (SELECT {a:1, b:2} AS s)",
- "SELECT MAP {'a': 1, 'b': 2}"
+ "SELECT MAP {'a': 1, 'b': 2}",
+ // Ordered aggregates, both spellings of the ordering.
+ "SELECT first_name, last_value(age ORDER BY salary) FROM person GROUP BY first_name",
+ "SELECT first_name, first_value(age ORDER BY salary DESC) FROM person GROUP BY first_name",
+ "SELECT first_name, array_agg(age ORDER BY salary) FROM person GROUP BY first_name",
+ "SELECT first_name, array_agg(DISTINCT age ORDER BY salary, id) FROM person GROUP BY first_name",
+ "SELECT first_name, string_agg(CAST(age AS VARCHAR), ',' ORDER BY salary) FROM person GROUP BY first_name",
+ "SELECT first_name, percentile_cont(0.5) WITHIN GROUP (ORDER BY age) FROM person GROUP BY first_name",
];
// For each test sql string, we transform as follows:
@@ -261,6 +268,21 @@ fn roundtrip_statement() -> Result<()> {
.with_aggregate_function(sum_udaf())
.with_aggregate_function(count_udaf())
.with_aggregate_function(max_udaf())
+ .with_aggregate_function(
+ datafusion_functions_aggregate::array_agg::array_agg_udaf(),
+ )
+ .with_aggregate_function(
+ datafusion_functions_aggregate::first_last::first_value_udaf(),
+ )
+ .with_aggregate_function(
+ datafusion_functions_aggregate::first_last::last_value_udaf(),
+ )
+ .with_aggregate_function(
+ datafusion_functions_aggregate::string_agg::string_agg_udaf(),
+ )
+ .with_aggregate_function(
+ datafusion_functions_aggregate::percentile_cont::percentile_cont_udaf(),
+ )
.with_expr_planner(Arc::new(CoreFunctionPlanner::default()))
.with_expr_planner(Arc::new(NestedFunctionPlanner))
.with_expr_planner(Arc::new(FieldAccessPlanner));
@@ -343,6 +365,18 @@ macro_rules! roundtrip_statement_with_dialect_helper {
.with_aggregate_function(
datafusion_functions_aggregate::percentile_cont::percentile_cont_udaf(),
)
+ .with_aggregate_function(
+ datafusion_functions_aggregate::array_agg::array_agg_udaf(),
+ )
+ .with_aggregate_function(
+ datafusion_functions_aggregate::first_last::first_value_udaf(),
+ )
+ .with_aggregate_function(
+ datafusion_functions_aggregate::first_last::last_value_udaf(),
+ )
+ .with_aggregate_function(
+ datafusion_functions_aggregate::string_agg::string_agg_udaf(),
+ )
.with_expr_planner(Arc::new(CoreFunctionPlanner::default()))
.with_expr_planner(Arc::new(NestedFunctionPlanner))
.with_expr_planner(Arc::new(FieldAccessPlanner));
@@ -4611,6 +4645,45 @@ fn roundtrip_approx_percentile_cont_within_group_with_centroids()
Ok(())
}
+/// Ordered aggregates spell their ordering in one of two ways: an argument-list
+/// clause (`array_agg(x ORDER BY y)`) or a `WITHIN GROUP` clause. The
+/// `roundtrip_*_within_group` tests above cover the `WITHIN GROUP` variant;
+/// this covers the argument-list variant.
+#[test]
+fn roundtrip_ordered_aggregate_order_by() -> Result<(), DataFusionError> {
+ roundtrip_statement_with_dialect_helper!(
+ sql: "SELECT first_name, last_value(age ORDER BY salary) FROM person GROUP BY first_name",
+ parser_dialect: GenericDialect {},
+ unparser_dialect: UnparserDefaultDialect {},
+ expected: @"SELECT person.first_name, last_value(person.age ORDER BY person.salary ASC NULLS LAST) FROM person GROUP BY person.first_name",
+ );
+ roundtrip_statement_with_dialect_helper!(
+ sql: "SELECT first_name, first_value(age ORDER BY salary DESC) FROM person GROUP BY first_name",
+ parser_dialect: GenericDialect {},
+ unparser_dialect: UnparserDefaultDialect {},
+ expected: @"SELECT person.first_name, first_value(person.age ORDER BY person.salary DESC NULLS FIRST) FROM person GROUP BY person.first_name",
+ );
+ roundtrip_statement_with_dialect_helper!(
+ sql: "SELECT first_name, array_agg(age ORDER BY salary) FROM person GROUP BY first_name",
+ parser_dialect: GenericDialect {},
+ unparser_dialect: UnparserDefaultDialect {},
+ expected: @"SELECT person.first_name, array_agg(person.age ORDER BY person.salary ASC NULLS LAST) FROM person GROUP BY person.first_name",
+ );
+ roundtrip_statement_with_dialect_helper!(
+ sql: "SELECT first_name, string_agg(CAST(age AS VARCHAR), ',' ORDER BY salary) FROM person GROUP BY first_name",
+ parser_dialect: GenericDialect {},
+ unparser_dialect: UnparserDefaultDialect {},
+ expected: @"SELECT person.first_name, string_agg(CAST(person.age AS VARCHAR), ',' ORDER BY person.salary ASC NULLS LAST) FROM person GROUP BY person.first_name",
+ );
+ roundtrip_statement_with_dialect_helper!(
+ sql: "SELECT first_name, array_agg(DISTINCT age ORDER BY salary, id) FROM person GROUP BY first_name",
+ parser_dialect: GenericDialect {},
+ unparser_dialect: UnparserDefaultDialect {},
+ expected: @"SELECT person.first_name, array_agg(DISTINCT person.age ORDER BY person.salary ASC NULLS LAST, person.id ASC NULLS LAST) FROM person GROUP BY person.first_name",
+ );
+ Ok(())
+}
+
#[test]
fn snowflake_flatten_multiple_unnest_cross_join() -> Result<(), DataFusionError> {
// Realistic Snowflake pattern:
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org