hassaanch23 opened a new pull request, #25400: URL: https://github.com/apache/datafusion/pull/25400
## Which issue does this PR close? - Closes #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: ```sql 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
