Repository navigation
fix: keep sort below OFFSET when the sort columns are projected away - #26584
yashaggarwal85d wants to merge 4 commits into
Conversation
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.
|
@AdamGS thanks for updating the branch with main. Whenever you or another committer has time, this is ready for review. It keeps ensure_sorting from removing a sort below GlobalLimitExec/LocalLimitExec, which is what dropped the SortExec under the OFFSET in #26488. Tests are in limit.slt with an EXPLAIN check. |
…er-aggregate # Conflicts: # datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/mod.rs # datafusion/sqllogictest/test_files/limit.slt
|
I think something got messed up here, seems like the only file left in the PR is the SLT file? If I look by commits there were some code changes here that got lost. |
|
Yeah I think so too! Lemme check what's wrong here |
|
Either way, I think If there's a different case here you can reproduce or a gap in tests I'm glad to review either. |
|
Found it. #26763 landed on main a few days ago and fixes the same thing in ensure_sorting, so when I merged main the conflict resolved to main's version and only my slt test was left. Ran the test on current main and it passes, so yeah, already fixed |
Which issue does this PR close?
Rationale for this change
When a subquery uses
ORDER BY ... OFFSET nand an aggregate reads it, the physical plan drops theSortExecbelowGlobalLimitExec. As a result,OFFSETskips arbitrary rows instead of the firstnrows in sort order, and the query returns wrong results:The plan going into
EnsureRequirementsis: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 ofGlobalLimitExecis the projection, which dropssandid, 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 becauseOutputRequirementswraps it in anOutputRequirementExec. 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
SortExecwith a fetch (TopK) is intentionally not covered: it re-sorts its input, so a sort below it really is redundant (test_handles_multiple_orthogonal_sortscovers that).What is the testing strategy for this PR?
Added
limit.sltcases from the issue: grouped and ungrouped aggregates over anORDER BY ... OFFSETsubquery, plus anEXPLAINshowing that theSortExecis kept. The grouped query returned0 6/1 -2onmain; it now returns0 9/1 -2, matching the expected result in the issue.The full sqllogictest suite and the
physical_optimizerintegration 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.