fix: omit ordering fields from the state of order-insensitive aggregates

[hassaanch23]

Which issue does this PR close?

• Part of Order-insensitive aggregates with an ORDER BY fail in grouped queries (min/max schema mismatch, avg/bit_*/stddev/var panic) #25401. This fixes the min/max schema mismatch. The avg/bit_*/stddev/var_* panics have a different cause and are fixed in fix: mark avg, bit_and/or/xor, stddev and variance as order-insensitive #25403; the two PRs are independent.

Rationale for this change

An order-insensitive aggregate called with an ORDER BY fails whenever the aggregation runs in two phases (Partial → Final/FinalPartitioned), grouped or not:

CREATE TABLE d (g INT, k INT, v INT) AS VALUES (1, 2, 20), (1, 1, 10), (2, 4, 40), (2, 3, 30);
SELECT g, min(v ORDER BY k) FROM d GROUP BY g;
-- Arrow error: Invalid argument error: number of columns(2) must match number of fields(3) in schema

AggregateExprBuilder::build() derived the aggregate's ordering_fields from its ORDER BY even when the function is order-insensitive. AggregateFunctionExpr::order_bys() hides those expressions from the accumulator, but state_fields() still passed ordering_fields. Min and Max use the default AggregateUDFImpl::state_fields, which appends them, while their accumulators only emit the value. The partial state schema therefore declared fields that no column filled. sum, count, bool_and and bool_or override state_fields without ordering fields, so they already worked.

What changes are included in this PR?

AggregateExprBuilder::build() now drops order_bys when the function's order_sensitivity() is insensitive. ordering_fields is then empty from the start, so the aggregate carries no ordering state that later code has to ignore. Order-sensitive aggregates are unchanged.

None of the built-in order-insensitive aggregates reads order_bys from AccumulatorArgs, and plans display aggregates by name(), so EXPLAIN output does not change.

What is the testing strategy for this PR?

Two queries in group_by.slt, in the section that sets target_partitions = 8, each selecting MIN, MAX and SUM with ORDER BY ts DESC over sales_global, with SUM as a control:

• Grouped by country. The plan is Partial → FinalPartitioned. On main it fails with number of columns(4) must match number of fields(6).
• Ungrouped, with WHERE amount > 0 so the input is multi-partition and the plan is Partial → Final. On main it fails with number of columns(3) must match number of fields(5). Without the filter, the single-partition input runs in Single mode and never builds a partial state.

Both queries pass with this change. The other .slt files covering aggregates still pass (group_by, aggregate, array_agg, first_last_*, distinct_on, agg_func_substitute, window, order).

Are there any user-facing changes?

Queries that failed now return results. No API changes.

Notes for reviewers

The state field names for ordering fields are being reworked in #25196. This PR only changes whether order-insensitive aggregates have ordering fields at all, so the two shouldn't interact beyond a possible textual rebase.

[neilconway]

Thanks @hassaanch23 ! This is a good analysis of the root cause of the bug. The approach we're taking in this PR is okay, but it leaves the state of the agg a little inconsistent (ordering_fields is defined but most code should probably check if the agg is order-insensitive before using it).

I wonder if it would be cleaner to clear order_bys in AggregateExprBuilder::build() for order-insensitive aggregates. That way ordering_fields is empty, so we avoid the inconsistent state in the first place. What do you think?

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.30769% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.32%. Comparing base (a2b8093) to head (2a95a74).

Files with missing lines	Patch %	Lines
datafusion/physical-expr/src/aggregate.rs	92.30%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #25402   +/-   ##
=======================================
  Coverage   82.32%   82.32%           
=======================================
  Files        1137     1137           
  Lines      431824   431883   +59     
  Branches   431824   431883   +59     
=======================================
+ Hits       355500   355563   +63     
+ Misses      54838    54824   -14     
- Partials    21486    21496   +10     

☔ 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.

[hassaanch23]

@neilconway Thanks, that's cleaner. I pushed cc17378. AggregateExprBuilder::build() now drops order_bys when the function is order-insensitive, so ordering_fields is empty from the start, and the state_fields() change is gone.

Before switching, I checked two things. None of the built-in order-insensitive aggregates (sum, count, min, max, any_value, bool_and, bool_or) reads order_bys from AccumulatorArgs. And plans display aggregates by name(), so EXPLAIN output doesn't change.

I also merged main. The semver-checks note was comparing against create_accumulator_with_metrics and create_groups_accumulator_with_metrics, which landed in #25051 after this branch was created. This PR doesn't touch either.

[neilconway]

Awesome, @hassaanch23 ! Nice work, I like this approach.

This is optional, but the SLTs are a bit fragile because they depend on the planner choosing a two-phase plan. We could also add a unit test that builds an order-insensitive UDAF with .order_by(), and then assert that its order_bys are empty. I got Claude to whip up something quickly: https://gist.github.com/neilconway/551457d97cea924b879be0dbc256deb7

[hassaanch23]

@neilconway Thanks! Pushed 2a95a74. It has your unit tests unchanged, with you credited as co-author, and both simplifications.

With the builder now clearing order_bys, order_insensitive_aggregate_discards_order_by exercises the builder itself rather than the accessor. With the clearing disabled, it fails at expr.order_bys().is_empty(), while order_sensitive_aggregate_keeps_order_by still passes. The full sqllogictest suite and clippy on all targets pass.

[neilconway]

@hassaanch23 Excellent! Thank you for the great PR and the quick turnaround time.