adriangb opened a new pull request, #25672:
URL: https://github.com/apache/datafusion/pull/25672
## Which issue does this PR close?
- Closes #25671.
## Rationale for this change
For `Limit(skip=0, fetch=n)` over a `Sort` without a fetch, `PushDownLimit`
sets `Sort.fetch = n` but keeps the `Limit`. It removes the `Limit` only on its
next visit, in the next optimizer pass. Thus every `ORDER BY ... LIMIT n` query
runs 3 logical optimizer passes where 2 are enough. The final plan does not
change. Only planning time does.
A query engine that wraps every query in a row-cap `LIMIT` over an `ORDER
BY` hits this on nearly every query.
Reproduction, `datafusion-cli` on `main` (95bb0a0dfa):
```sql
CREATE TABLE t (a INT, ts TIMESTAMP) AS VALUES (1, TIMESTAMP
'2026-01-01T00:00:00');
EXPLAIN VERBOSE SELECT a FROM t ORDER BY ts DESC LIMIT 10;
```
Rules that change the plan, per pass (the `logical_plan after <rule>` rows
that are not `SAME TEXT AS ABOVE`):
```text
main:
pass 0: push_down_limit, optimize_projections
pass 1: push_down_limit <- removes the Limit it kept in pass 0
pass 2: (no change)
this PR:
pass 0: push_down_limit, optimize_projections
pass 1: (no change)
```
The final `logical_plan` is identical on both:
```text
Projection: t.a
Sort: t.ts DESC NULLS FIRST, fetch=10
TableScan: t projection=[a, ts]
```
## What changes are included in this PR?
In `rewrite_limit`, `LogicalPlan::Sort` branch, when the rule gives the
`Sort` a new fetch and `skip == 0`, it now returns the `Sort` and drops the
`Limit` in the same visit. With `skip > 0` the `Limit` stays, as before.
One detail needs attention. The optimizer applies `PushDownLimit` top-down
and then recurses into the children of the node that the rule returns. Before
this change, the `Sort` was a child of the returned `Limit`, so the same pass
also visited it and applied the TopK-through-join pushdown
(`push_topk_through_join`). Now the `Sort` is the returned node, so the
traversal does not visit it again in this pass. The new branch therefore calls
`push_topk_through_join` on the `Sort` directly. Without that call, `Limit ->
Sort -> Left Join` would move the extra pass from the `Limit` removal to the
TopK pushdown instead of removing it.
## What is the testing strategy for this PR?
- `push_down_limit::test::limit_push_down_sort`: snapshot updated. After one
pass the plan is `Sort: ..., fetch=10` without the `Limit` above it.
- New `push_down_limit::test::limit_push_down_sort_settles_in_first_pass`:
runs `PushDownLimit` with `max_passes = 3` and asserts that the optimizer stops
after 2 passes (pass 1 leaves the plan unchanged). On `main` it runs 3.
-
`topk_through_join::test::topk_pushed_through_limit_then_sort_with_two_passes`
is renamed to `topk_pushed_through_limit_then_sort_in_one_pass` and now uses
`max_passes = 1`, with the same expected plan. It fails if the new branch does
not call `push_topk_through_join`.
I checked that the three tests fail without the fix. Results:
- `cargo test --profile ci -p datafusion-optimizer`: 898 + 26 + 5 passed, 0
failed.
- `cargo test --profile ci -p datafusion-sqllogictest --test sqllogictests`:
522 of 522 files pass. No expected plan changes, because the final plans are
the same.
- `cargo fmt --all -- --check` and `cargo clippy --profile ci -p
datafusion-optimizer --all-targets -- -D warnings`: clean.
## Are there any user-facing changes?
No. Final plans are the same. Planning an `ORDER BY ... LIMIT` query without
an `OFFSET` takes one less optimizer pass. A plan inspected after a single pass
(`max_passes = 1`) no longer has the redundant `Limit`.
🤖 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]
