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:
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? On main the inner `SortExec` here stays a full sort,
and with this PR it becomes `TopK(fetch=5)`. Fine to do in a follow-up if you'd
rather keep this PR as is.
```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;
```
##########
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:
Optional cleanup: since `reset()` returns the state to its `Default`,
`std::mem::take` can do the copy and the reset in one step, and `reset()`
itself can become `*self = Self::default();`.
```diff
- let latest = ctx.limit;
- let lookahead = ctx.lookahead;
- ctx.reset();
- let Some(fetch) = latest else {
+ let TraverseState {
+ limit, lookahead, ..
+ } = std::mem::take(ctx);
+ let Some(fetch) = limit else {
```
--
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]
