cool9850311 commented on code in PR #26773:
URL: https://github.com/apache/datafusion/pull/26773#discussion_r4431751442
##########
datafusion/physical-optimizer/src/limit_pushdown_past_window.rs:
##########
@@ -178,23 +198,27 @@ 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)));
};
// A fetch that overflows can't drop any rows, so skip the pushdown rather
// than turning the sort into a TopK that holds every row and can't spill.
- let Some(fetch) = fetch.checked_add(ctx.lookahead) else {
- ctx.lookahead = 0;
+ let Some(fetch) = fetch.checked_add(lookahead) else {
return Some(Transformed::no(Arc::clone(node)));
};
let fetch = match node.fetch() {
None => fetch,
Some(existing) => cmp::min(existing, fetch),
};
- 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:
Confirmed and fixed in eebba16a, your patch as-is. Two slt cases: the shape
you
gave, and the same stack with nothing in between -- that one is wrong on
`main`
too, since the old stop only kept the walk out of the subtree below a sort it
had already fetched, never out of a second window on the same chain.
The five `window.slt` fetches go up as you measured. Adding over-estimates
only
when the outer window doesn't read the inner's output, which is all five of
them.
--
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]
