fix: ignore repeated sort keys in an ordered aggregate's ORDER BY

[hassaanch23]

Which issue does this PR close?

• Closes Duplicate expression in an ordered aggregate's ORDER BY panics (first_last.rs:602 assertion) or fails with an internal Arrow error #25398.

Rationale for this change

If an ordered aggregate's ORDER BY names the same expression more than once, the query panics or fails with an internal Arrow error:

CREATE TABLE t AS SELECT i % 3 AS b, i AS id FROM (SELECT unnest(range(0,10)) AS i);
SELECT b, first_value(id ORDER BY b, b) FROM t GROUP BY b;
-- panicked at datafusion/functions-aggregate/src/first_last.rs:602:13
-- assertion `left == right` failed  left: 2  right: 1

Repeating a key is legal SQL. DISTINCT ON is the easy way to hit this by accident, because its key has to lead the ORDER BY, and writing the key out again produces the duplicate: SELECT DISTINCT ON (b) b FROM t ORDER BY b, b.

The two sides of the ordered aggregate count the sort keys differently:

• AggregateExprBuilder::build derives the ordering state fields (ordering_fields) from the ORDER BY list as written, so b, b becomes two fields.
• Every accumulator (first_value, last_value, nth_value, array_agg, string_agg) builds its ordering with LexOrdering::new, which drops a sort key whose expression already appeared, so b, b becomes one key.

Depending on which consumer compares them first, the mismatch shows up as the first_last.rs assertion, Incorrect number of arrays provided to RowConverter, or a column-count mismatch in the state schema.

What changes are included in this PR?

AggregateExprBuilder::build now passes order_bys through LexOrdering before deriving the ordering types and fields. The state and the accumulators then agree on the number of sort keys.

A repeated key can never break a tie the earlier one left, so dropping it doesn't change results. As with LexOrdering elsewhere, the first occurrence wins, so ORDER BY b ASC, b DESC orders like ORDER BY b ASC.

What is the testing strategy for this PR?

A new block in aggregate.slt next to the existing ordered string_agg tests. Its table has a unique sort key, so each expected value is decided by the ordering, not by a tie. It covers:

• first_value, last_value and nth_value, with nth_value using an expression key (k + 0, k + 0)
• array_agg, plus string_agg with ORDER BY k DESC, k ASC
• DISTINCT ON (g) ... ORDER BY g, g, k

Without the change, three of the new queries fail: the two panics and the RowConverter error from the issue. With it, they pass. The other .slt files that exercise ordered aggregates (aggregate, array_agg, group_by, first_last_*, distinct_on, window, order, subquery_sort) still pass.

I also ran every query in the issue's matrix and compared it with the same query with the repeated key removed. All twelve return the same rows.

Are there any user-facing changes?

Queries that failed now return results. No API changes.

Notes for reviewers

• Not addressed here: min/max with an ORDER BY. The issue's min(id ORDER BY b, b) row turns out to be a separate bug. Grouped min(v ORDER BY k) and max(v ORDER BY k) fail on main with number of columns(2) must match number of fields(3) even without any repeated key, under the default target_partitions. Min/Max use the default state_fields, which appends ordering_fields, but their accumulators only emit the value. The new tests leave out min for that reason. I can open a separate issue for it.
• Unknown: with_new_expressions. It rebuilds order_bys by zipping the existing (now deduplicated) keys with new expressions, and keeps ordering_fields as-is. I haven't found a rewrite that maps two distinct sort keys onto the same expression. If one exists, it could reintroduce a repeat on that path, and it may deserve a look from someone who knows those rewrites better.

[github-actions]

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

     Cloning apache/main
    Building datafusion-physical-expr v55.1.0 (current)
       Built [  33.691s] (current)
     Parsing datafusion-physical-expr v55.1.0 (current)
      Parsed [   0.052s] (current)
    Building datafusion-physical-expr v55.1.0 (baseline)
       Built [  33.277s] (baseline)
     Parsing datafusion-physical-expr v55.1.0 (baseline)
      Parsed [   0.052s] (baseline)
    Checking datafusion-physical-expr v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.353s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/inherent_method_missing.ron

Failed in:
  AggregateFunctionExpr::create_accumulator_with_metrics, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/91f3ff28275443f3be4d4ecdd95a603fa2bc7f1d/datafusion/physical-expr/src/aggregate.rs:751
  AggregateFunctionExpr::create_groups_accumulator_with_metrics, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/91f3ff28275443f3be4d4ecdd95a603fa2bc7f1d/datafusion/physical-expr/src/aggregate.rs:942

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  68.462s] datafusion-physical-expr
    Building datafusion-sqllogictest v55.1.0 (current)
       Built [ 107.397s] (current)
     Parsing datafusion-sqllogictest v55.1.0 (current)
      Parsed [   0.023s] (current)
    Building datafusion-sqllogictest v55.1.0 (baseline)
       Built [ 105.514s] (baseline)
     Parsing datafusion-sqllogictest v55.1.0 (baseline)
      Parsed [   0.024s] (baseline)
    Checking datafusion-sqllogictest v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.098s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [ 216.133s] datafusion-sqllogictest

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.28%. Comparing base (b300cea) to head (237ee34).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25400      +/-   ##
==========================================
- Coverage   82.28%   82.28%   -0.01%     
==========================================
  Files        1137     1137              
  Lines      430211   430214       +3     
  Branches   430211   430214       +3     
==========================================
- Hits       354018   354001      -17     
- Misses      54771    54786      +15     
- Partials    21422    21427       +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.