You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2026/09/15 22:03:53 UTC
[PR] fix: do not use the top-k aggregate stream for a DISTINCT limit with no ordering direction [datafusion]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: do not use the top-k aggregate stream for a DISTINCT limit with no ordering direction [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25349:
URL: https://github.com/apache/datafusion/pull/25349#issuecomment-5688957811
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25349?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 81.92%. Comparing base ([`22651d2`](https://app.codecov.io/gh/apache/datafusion/commit/22651d24cc8196f3206e09a36a437eda9e766b87?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6a8704d`](https://app.codecov.io/gh/apache/datafusion/commit/6a8704d97bf31f429e5792b4c90314a0700ff9e8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 6 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25349 +/- ##
==========================================
- Coverage 81.92% 81.92% -0.01%
==========================================
Files 1135 1135
Lines 427772 427773 +1
Branches 427772 427773 +1
==========================================
- Hits 350456 350448 -8
- Misses 56373 56379 +6
- Partials 20943 20946 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25349?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: do not use the top-k aggregate stream for a DISTINCT limit with no ordering direction [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #25349:
URL: https://github.com/apache/datafusion/pull/25349
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org