alamb commented on code in PR #22525: URL: https://github.com/apache/datafusion/pull/22525#discussion_r3306660564
########## datafusion/sqllogictest/test_files/limit.slt: ########## @@ -989,3 +989,34 @@ c-4 statement ok DROP TABLE t21176; + +# Regression test for https://github.com/apache/datafusion/issues/22489 Review Comment: I don't think this guards against the issue: I reverted the code change locally ```diff diff --git a/datafusion/physical-optimizer/src/limit_pushdown.rs b/datafusion/physical-optimizer/src/limit_pushdown.rs index 63c4f21bd9..6164d86e53 100644 --- a/datafusion/physical-optimizer/src/limit_pushdown.rs +++ b/datafusion/physical-optimizer/src/limit_pushdown.rs @@ -375,14 +375,6 @@ pub(crate) fn pushdown_limits( (new_node, global_state) = pushdown_limit_helper(new_node.data, global_state)?; } - // Once a limit has been materialized above the current node, child - // subtrees should not inherit its `skip`. Keep `fetch`, but clear - // `skip` before recursing so child-local limits are not merged with - // an `OFFSET` that has already been applied. - if global_state.satisfied { - global_state.skip = 0; - } - // Apply pushdown limits in children let children = new_node.data.children(); let mut changed = false; ``` And then I ran the tests: ```shell cargo test --profile=ci --test sqllogictests ... Running with 16 test threads (available parallelism: 16) Completed 472 test files in 9 seconds ``` -- 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]
