cool9850311 commented on code in PR #26773:
URL: https://github.com/apache/datafusion/pull/26773#discussion_r4423688465
##########
datafusion/physical-optimizer/src/limit_pushdown_past_window.rs:
##########
@@ -188,17 +196,22 @@ fn apply_limit(
if !node.is::<SortExec>() && !node.is::<SortPreservingMergeExec>() {
return None;
}
- let latest = ctx.limit.take();
- let Some(fetch) = latest else {
- ctx.limit = None;
- ctx.lookahead = 0;
+ // Taking the state both reads the chain we were walking and forgets it.
+ let TraverseState {
+ limit, lookahead, ..
+ } = std::mem::take(ctx);
+ let Some(fetch) = limit else {
return Some(Transformed::no(Arc::clone(node)));
};
let fetch = match node.fetch() {
- None => fetch + ctx.lookahead,
- Some(existing) => cmp::min(existing, fetch + ctx.lookahead),
+ None => fetch + lookahead,
+ Some(existing) => cmp::min(existing, fetch + lookahead),
};
- Some(Transformed::complete(node.with_fetch(Some(fetch)).unwrap()))
+ // Carry on past this sort rather than stopping. `ctx` is back to its
default
+ // above, which is exactly what it held at the start of the traversal, so
+ // walking the subtree below is equivalent to running the rule on it afresh
+ // -- and that subtree may hold another independent limit over another
window.
+ Some(Transformed::yes(node.with_fetch(Some(fetch)).unwrap()))
Review Comment:
fixed in 4877863, your patch as-is, with unit tests as you suggested. The
nested shape you described gets its own test in 41d9edc.
--
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]
