diff --git a/datafusion/physical-expr/src/aggregate.rs b/datafusion/physical-expr/src/aggregate.rs index 6d95d8ea12bd8..7b72a2622aea6 100644 --- a/datafusion/physical-expr/src/aggregate.rs +++ b/datafusion/physical-expr/src/aggregate.rs @@ -61,7 +61,7 @@ use datafusion_functions_aggregate_common::accumulator::{ }; use datafusion_functions_aggregate_common::order::AggregateOrderSensitivity; use datafusion_physical_expr_common::physical_expr::PhysicalExpr; -use datafusion_physical_expr_common::sort_expr::PhysicalSortExpr; +use datafusion_physical_expr_common::sort_expr::{LexOrdering, PhysicalSortExpr}; #[derive(Debug, Clone)] struct AggregateHumanDisplay { @@ -265,12 +265,18 @@ impl AggregateExprBuilder { assert_or_internal_err!(!args.is_empty(), "args should not be empty"); // An order-insensitive aggregate ignores its ORDER BY, so drop it here. - // Everything derived from `order_bys` below, such as the ordering fields - // in the aggregate's state, then agrees that there is no ordering. + // Otherwise drop sort keys that repeat an earlier one: a repeated key can + // never break a tie the earlier one left (`ORDER BY b ASC, b DESC` orders + // like `ORDER BY b ASC`), and accumulators build their ordering with + // `LexOrdering`, which drops such repeats. Everything derived from + // `order_bys` below, such as the ordering fields in the aggregate's + // state, then matches the sort keys the accumulators compare. let order_bys = if fun.order_sensitivity().is_insensitive() { vec![] } else { - order_bys + LexOrdering::new(order_bys) + .map(Vec::from) + .unwrap_or_default() }; let ordering_types = order_bys diff --git a/datafusion/sqllogictest/test_files/aggregate.slt b/datafusion/sqllogictest/test_files/aggregate.slt index 2be4e173fc701..e204df30e8dcc 100644 --- a/datafusion/sqllogictest/test_files/aggregate.slt +++ b/datafusion/sqllogictest/test_files/aggregate.slt @@ -8106,6 +8106,41 @@ c,a,b,d d,b,a,c statement ok drop table t; +# A sort key repeated in an ordered aggregate's ORDER BY is redundant, and the +# query must behave as if the key were listed once (issue #25398). Each `k` is +# unique so every expected value is determined by the ordering. +statement ok +CREATE TABLE dup_sort_keys (g INT, k INT, v INT) AS VALUES + (1, 2, 20), (1, 1, 10), (2, 4, 40), (2, 3, 30); + +query IIII +SELECT g, + first_value(v ORDER BY k, k), + last_value(v ORDER BY k, k), + nth_value(v, 2 ORDER BY k + 0, k + 0) +FROM dup_sort_keys GROUP BY g ORDER BY g; +---- +1 10 20 20 +2 30 40 40 + +query I?T +SELECT g, + array_agg(v ORDER BY k, k), + string_agg(CAST(v AS VARCHAR), ',' ORDER BY k DESC, k ASC) +FROM dup_sort_keys GROUP BY g ORDER BY g; +---- +1 [10, 20] 20,10 +2 [30, 40] 40,30 + +query II +SELECT DISTINCT ON (g) g, v FROM dup_sort_keys ORDER BY g, g, k; +---- +1 10 +2 30 + +statement ok +drop table dup_sort_keys; + # Tests for aggregating with NaN values statement ok