ninglinLiu commented on code in PR #26674:
URL: https://github.com/apache/datafusion/pull/26674#discussion_r4369609998
##########
datafusion/physical-optimizer/src/limit_pushdown_past_window.rs:
##########
@@ -194,16 +194,22 @@ fn apply_limit(
ctx.lookahead = 0;
return Some(Transformed::no(Arc::clone(node)));
};
+ // Saturate like EnsureRequirements sort pushdown: a huge window
+ // FOLLOWING bound must not wrap the TopK fetch in release builds.
let fetch = match node.fetch() {
- None => fetch + ctx.lookahead,
- Some(existing) => cmp::min(existing, fetch + ctx.lookahead),
+ None => fetch.saturating_add(ctx.lookahead),
Review Comment:
Thanks, good catch on the unspillable TopK. Done in 80ec0e7: when `fetch +
lookahead` overflows, `apply_limit` now resets the lookahead and leaves the
sort as is, so the plan keeps a plain `SortExec`.
##########
datafusion/sqllogictest/test_files/window_limits.slt:
##########
@@ -862,3 +862,17 @@ physical_plan
03)----BoundedWindowAggExec: wdw=[lead(employees.salary,Int64(2)) ORDER BY
[employees.empno ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT
ROW: Field { "lead(employees.salary,Int64(2)) ORDER BY [employees.empno ASC
NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW": nullable UInt64
}, frame: ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW], mode=[Sorted]
04)------SortExec: TopK(fetch=5), expr=[empno@0 ASC NULLS LAST],
preserve_partitioning=[false]
05)--------DataSourceExec: file_groups={1 group:
[[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100_with_dates.csv]]},
projection=[empno, salary], file_type=csv, has_header=true
+
+# Huge FOLLOWING bound must not overflow LimitPushPastWindows (issue #26658)
Review Comment:
Added both cases (the `lead` offset on a huge frame and the huge `FOLLOWING`
start bound) in 80ec0e7. `cargo test -p datafusion-sqllogictest --test
sqllogictests -- window_limits` passes locally.
--
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]
