yashaggarwal85d opened a new pull request, #26584:
URL: https://github.com/apache/datafusion/pull/26584
## Which issue does this PR close?
- Closes #26488.
## Rationale for this change
When a subquery uses `ORDER BY ... OFFSET n` and an aggregate reads it, the
physical plan drops the `SortExec` below `GlobalLimitExec`. As a result,
`OFFSET` skips arbitrary rows instead of the first `n` rows in sort order, and
the query returns wrong results:
```sql
SELECT g, SUM(v) FROM (SELECT * FROM t ORDER BY s, id OFFSET 1) q GROUP BY g;
```
The plan going into `EnsureRequirements` is:
```text
AggregateExec
GlobalLimitExec: skip=1
ProjectionExec: [g, v] <- drops the sort columns s, id
SortExec: [s, id]
```
In `ensure_sorting`, a child sort that the parent doesn't require is removed
when the child has no output ordering, on the assumption that the sort is
"neutralized". Here the child of `GlobalLimitExec` is the projection, which
drops `s` and `id`, so it reports no ordering, and the sort gets removed. But a
limit decides which rows to keep from its input order, so that sort still
determines the result.
Without the aggregate (for example under a `FilterExec`), the sort survives
only because `OutputRequirements` wraps it in an `OutputRequirementExec`. The
aggregate stops that search, which is why the bug needs an aggregate to show up.
## What changes are included in this PR?
In `ensure_sorting`, don't remove a sort through the "neutralized sort" path
when the current node is a limit (`GlobalLimitExec` / `LocalLimitExec`).
A `SortExec` with a fetch (TopK) is intentionally not covered: it re-sorts
its input, so a sort below it really is redundant
(`test_handles_multiple_orthogonal_sorts` covers that).
## What is the testing strategy for this PR?
Added `limit.slt` cases from the issue: grouped and ungrouped aggregates
over an `ORDER BY ... OFFSET` subquery, plus an `EXPLAIN` showing that the
`SortExec` is kept. The grouped query returned `0 6` / `1 -2` on `main`; it now
returns `0 9` / `1 -2`, matching the expected result in the issue.
The full sqllogictest suite and the `physical_optimizer` integration tests
pass.
## Are there any user-facing changes?
Only the bug fix: queries like the one above now return correct results.
---
AI-assisted: I used an AI coding assistant while preparing this PR. I have
reviewed and understand all 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]
