adriangb opened a new pull request, #25349:
URL: https://github.com/apache/datafusion/pull/25349
## Rationale for this change
`SELECT DISTINCT ... LIMIT n` with no `ORDER BY` fails at execution with
`Internal error: Ordering direction required for DISTINCT with limit` when
one of the
projected columns is a window function:
```sql
CREATE TABLE t (env VARCHAR, region VARCHAR, deployment VARCHAR);
INSERT INTO t VALUES ('prod','a','x'), ('prod','a','y'), ('staging','b','x');
SELECT DISTINCT env, region, count(*) OVER (PARTITION BY env, deployment) AS
n FROM t LIMIT 30;
```
`LimitedDistinctAggregation` pushes `LimitOptions::new(limit)` (so
`descending: None`) into the
`AggregateExec` while `is_unordered_unfiltered_group_by_distinct()` holds.
That predicate is not
stable under later rewrites: `EnforceSorting` inserts the sort the window
function requires below
the aggregation, the aggregation is rebuilt through `replace_children` with
`ChildrenPropertiesMode::Recompute`, which re-derives the properties from
the now-ordered child
while carrying `limit_options` over, and the rebuilt node ends up with
`ordering_mode=PartiallySorted(..)` and an output ordering. The predicate is
now false, so
`execute_typed` selects `GroupedTopKAggregateStream`, whose constructor
needs an ordering
direction that was never supplied, and errors.
The top-k stream is not just missing a direction here: it also reads only
the first group key
(`aggr.group_expr().expr()[0]`), so it would be wrong for a multi-column
`DISTINCT` even with a
direction. The correct behaviour is to not take that path at all.
## What changes are included in this PR?
`AggregateExec::execute_typed` now takes the top-k path only when an
ordering direction is
actually available, matching what `GroupedTopKAggregateStream::new`
requires: either
`limit_options().descending` is set (which is what `TopKAggregation` pushes,
and it already
restricts itself to a single group key) or `get_minmax_desc()` supplies one.
Otherwise execution
falls back to the regular grouped streams, which is correct because they
treat the pushed limit
as a soft limit (or ignore it) and the `LIMIT` above the aggregation still
truncates the result.
The pushdown itself and the resulting plans are unchanged; this only changes
which stream is
selected at execution time.
## Are these changes tested?
Yes. A new case in `datafusion/sqllogictest/test_files/aggregate.slt` covers
the multi-column and
single-column forms of the query and asserts the returned rows (matching
PostgreSQL and DuckDB).
It fails on `main` with the reported internal error and passes with this
change.
`cargo test -p datafusion-physical-plan` (2239 tests) and the
`aggregate.slt`,
`aggregates_topk.slt`, `limit.slt`, `window.slt`, `group_by.slt`,
`explain.slt` and
`distinct_on.slt` sqllogictest files all pass.
## Are there any user-facing changes?
Queries that previously failed with `Internal error: Ordering direction
required for DISTINCT
with limit` now return the correct rows. There are no API changes and no
plan changes.
--
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]
