cool9850311 opened a new pull request, #26773: URL: https://github.com/apache/datafusion/pull/26773
## Which issue does this PR close? - Closes #26659. ## Rationale for this change `LimitPushPastWindows` turns the `SortExec` under a window into a TopK when a `LIMIT` sits above it, but only for the first window in a plan. Every later one keeps a full sort — more sorting and more rows held in memory than necessary. Results were always correct. ## What changes are included in this PR? Two commits, one per cause named in the issue. **`refactor: keep LimitPushPastWindows's traversal phase in TraverseState`** — `phase` was a local in `optimize()`, so neither the `reset` closure nor `apply_limit` could reach it, and those are the two places that drop the chain being walked. It moves into `TraverseState`, which gets a single `reset` that clears all three fields at once. **`fix: push a limit past every window in a plan, not just the first`** — `apply_limit` returned `Transformed::complete`, whose `TreeNodeRecursion` is `Stop`, ending the whole `transform_down`. It now returns `Transformed::yes`. ### Why not `Jump`, as the issue suggests The issue's diagnosis is exactly right, and `Jump` does fix the union case. It is not enough for a second window **nested further down the same chain**, because there the second limit and window sit *inside* the subtree `Jump` prunes: | | union, two branches | nested, same chain | | --- | --- | --- | | before | ❌ first branch only | ❌ outer only | | `Jump` | ✅ | ❌ still outer only | | `Transformed::yes` | ✅ | ✅ | This does deliberately reverse the early stop added in #17347. That stop existed to avoid walking below a sort that already had its fetch, which was necessary while the traversal state survived that point. It no longer does: the first commit makes `apply_limit` reset the state, so `ctx` there holds exactly what it held at the start of the traversal, and descending is equivalent to running the rule on that subtree afresh. With nothing stale to protect against, there is nothing left to prune for. ### Fixed along the way - **A second window nested in the same chain**, not just a second union branch. The issue says "The same happens for any later window in the plan, not only under a union"; `Jump` would have covered only the union half of that. - **`apply_limit` leaked `lookahead`.** Its successful path took `limit` but left `lookahead` set. Harmless while the traversal stopped right after, a live bug once it continues. ## What is the testing strategy for this PR? Two sqllogictest cases in `window_limits.slt`, one per plan shape. Both shapes are expressible in SQL, so they are `.slt` rather than unit tests, per the [testing guide](https://datafusion.apache.org/contributor-guide/testing.html#choosing-what-kind-of-test-to-write). Each asserts results with `enable_window_limits` both off and on, then asserts the plan. The nested case is what separates `Transformed::yes` from `Jump`: switching to `Jump` turns that case red and leaves the union case green. Two notes for anyone editing these later. The union case needs `rowsort` — `UnionExec` yields its branches in a nondeterministic order, and without it the case fails intermittently. The nested case's query has three load-bearing properties, called out in a comment: the two levels must order by different columns (or `EnforceSorting` drops the inner `SortExec`), the outer `SELECT` must reference `s1` (or projection pushdown removes the inner window), and both levels need their own `LIMIT`. Full sqllogictest suite passes with no existing expectation updated — every existing case is a single-window chain, where `Stop` and `yes` behave identically. `cargo clippy --all-targets --all-features -- -D warnings`, `./dev/rust_lint.sh` and the extended test suite are clean. No local benchmark reaches this rule (`benchmarks/queries/h2o/window.sql` has no `LIMIT`; the benchmark queries that do use `LIMIT` have no windows), so there is nothing to compare against `main`. ## Are there any user-facing changes? No API or configuration changes. Affected plans get a TopK where they had a full sort; results are unchanged. --- One thing I found but left alone, happy to file separately: `grow_limit`'s `LimitEffect::Absolute` arm uses `ctx.limit.unwrap_or(0)`, so with no limit in hand it invents one and would truncate a sort that has no `LIMIT` above it. It is reachable today (a union branch with a window and a sort but no limit), but no in-tree window function returns `Absolute`, so only a third-party UDWF can trigger it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
