Skip to content

fix: keep sort below OFFSET when the sort columns are projected away - #26584

Open
yashaggarwal85d wants to merge 1 commit into
apache:mainfrom
yashaggarwal85d:fix/offset-sort-under-aggregate
Open

yashaggarwal85d wants to merge 1 commit into
apache:mainfrom
yashaggarwal85d:fix/offset-sort-under-aggregate

Conversation

@yashaggarwal85d

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

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:

SELECT g, SUM(v) FROM (SELECT * FROM t ORDER BY s, id OFFSET 1) q GROUP BY g;

The plan going into EnsureRequirements is:

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.

ensure_sorting removed a sort that its parent did not require when the
child reported no output ordering, assuming the sort was neutralized.
Below a limit this is wrong: a projection that drops the sort columns
hides the ordering, but the limit still keeps or skips rows based on it.
This made ORDER BY ... OFFSET subqueries consumed by an aggregate return
wrong results.

Closes apache#26488.
@github-actions github-actions Bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Physical optimizer drops SortExec below OFFSET when an aggregate consumes the subquery

1 participant