From 237ee34223a117d96a82858520c3a141be048d29 Mon Sep 17 00:00:00 2001
From: hassaanch23 <imhassaan04@gmail.com>
Date: Thu, 17 Sep 2026 13:45:57 +0500
Subject: [PATCH] fix: ignore repeated sort keys in an ordered aggregate's
 ORDER BY

An ordered aggregate whose ORDER BY named the same expression twice, for
example `first_value(id ORDER BY b, b)` or `DISTINCT ON (b) b ... ORDER BY
b, b`, panicked in first_last.rs or failed with an internal Arrow error.

AggregateExprBuilder::build derived the ordering state fields from the
ORDER BY list as written, while every accumulator builds its ordering with
LexOrdering::new, which drops a sort key whose expression already appeared.
The two sides disagreed on how many sort keys there are.

Drop repeated keys in the builder the same way. A repeated key can never
break a tie the earlier one left, so results do not change.

Closes #25398
---
 datafusion/physical-expr/src/aggregate.rs     | 11 +++++-
 .../sqllogictest/test_files/aggregate.slt     | 35 +++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/datafusion/physical-expr/src/aggregate.rs b/datafusion/physical-expr/src/aggregate.rs
index df22bc69d8706..1d58cb7051b40 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 {
@@ -264,6 +264,15 @@ impl AggregateExprBuilder {
         } = self;
         assert_or_internal_err!(!args.is_empty(), "args should not be empty");
 
+        // A sort key that repeats an earlier one can never break a tie the earlier
+        // one left, so it is redundant (`ORDER BY b ASC, b DESC` orders like
+        // `ORDER BY b ASC`). Accumulators build their ordering with `LexOrdering`,
+        // which drops such repeats, so drop them here as well: the ordering state
+        // must have exactly one field per sort key the accumulators compare.
+        let order_bys = LexOrdering::new(order_bys)
+            .map(Vec::from)
+            .unwrap_or_default();
+
         let ordering_types = order_bys
             .iter()
             .map(|e| e.expr.data_type(&schema))
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