cool9850311 commented on code in PR #26773:
URL: https://github.com/apache/datafusion/pull/26773#discussion_r4384409106
##########
datafusion/physical-optimizer/src/limit_pushdown_past_window.rs:
##########
@@ -80,16 +92,14 @@ impl PhysicalOptimizerRule for LimitPushPastWindows {
return Ok(original);
}
let mut ctx = TraverseState::default();
- let mut phase = Phase::FindOrGrow;
let result = original.transform_down(|node| {
// helper closure to DRY out most the early return cases
let reset = |node,
ctx: &mut TraverseState|
-> datafusion_common::Result<
Transformed<Arc<dyn ExecutionPlan>>,
> {
- ctx.limit = None;
- ctx.lookahead = 0;
+ ctx.reset();
Review Comment:
You're right that this is a fix and not a refactor — relabelled the commit
and reworked the PR description around it, and added the coverage. Thanks, I
would have shipped this with the reset untested.
One correction on the attribution, because it changed what I ended up
adding. Your query orders the two levels by different columns, so an outer
`SortExec` survives between the `RANGE` window and the inner limit.
`apply_limit` runs at that sort, and it clears the state *before* it checks
whether it has a limit in hand — so the phase gets restored there even if the
early-return path stops doing it. I built exactly that variant (early-return
path clears `limit`/`lookahead` only, `apply_limit` untouched) and your query
still comes out `TopK(fetch=5)`.
The shape that needs the clear in the early-return path is the same query
with both levels ordering by the same column. `EnforceSorting` then elides the
outer `SortExec`, so the node below the window is the inner `GlobalLimitExec`
itself and there is no sort on the way to clear anything:
```sql
SELECT empno, s1, sum(empno) OVER (ORDER BY empno) AS s2
FROM (SELECT empno, salary, sum(salary) OVER (ORDER BY empno ROWS UNBOUNDED
PRECEDING) AS s1 FROM employees ORDER BY empno LIMIT 5)
ORDER BY empno LIMIT 2;
```
```
02)--GlobalLimitExec: skip=0, fetch=2
03)----BoundedWindowAggExec: ... frame: RANGE BETWEEN UNBOUNDED PRECEDING
AND CURRENT ROW, mode=[Sorted]
04)------ProjectionExec: ...
05)--------GlobalLimitExec: skip=0, fetch=5
06)----------BoundedWindowAggExec: ... frame: ROWS BETWEEN UNBOUNDED
PRECEDING AND CURRENT ROW, mode=[Sorted]
07)------------SortExec: TopK(fetch=5), expr=[empno@0 ASC NULLS LAST],
preserve_partitioning=[false]
```
Dropping the phase reset from the early-return path turns that one red and
leaves yours green. Both are full sorts on `main`, so I added both and the
comment above them says which is which — yours as the shape a reader is more
likely to write, this one as the strict test.
These two cases live in the first commit now, so each commit carries the
tests for its own fix.
##########
datafusion/physical-optimizer/src/limit_pushdown_past_window.rs:
##########
@@ -188,17 +198,21 @@ fn apply_limit(
if !node.is::<SortExec>() && !node.is::<SortPreservingMergeExec>() {
return None;
}
- let latest = ctx.limit.take();
+ let latest = ctx.limit;
Review Comment:
Done, thanks — `reset` is `*self = Self::default()` and `apply_limit`
destructures `std::mem::take(ctx)`. It also removes the duplicate clearing
`main` had on the no-limit path, which was the other half of the field-by-field
handling.
--
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]
