jayzhan211 commented on code in PR #26773:
URL: https://github.com/apache/datafusion/pull/26773#discussion_r4378683104
##########
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:
@cool9850311 thanks for tracking down both causes! Resetting `phase` here is
a behavior fix of its own, not just a refactor. On main, a window whose limit
can't grow (e.g. a RANGE frame) left `phase` at `Apply`, so no later limit was
ever picked up. Both new slt cases go through `apply_limit`, so nothing would
fail if this reset were dropped. Could you add an EXPLAIN case for it in this
PR? On main the inner `SortExec` here stays a full sort, and with this PR it
becomes `TopK(fetch=5)`.
```sql
EXPLAIN
SELECT empno, salary, s1, sum(empno) OVER (ORDER BY salary) 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 salary LIMIT 2;
```
--
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]
