alamb commented on code in PR #14245: URL: https://github.com/apache/datafusion/pull/14245#discussion_r1932087589
########## datafusion/sqllogictest/test_files/joins.slt: ########## @@ -4264,8 +4266,10 @@ logical_plan physical_plan 01)GlobalLimitExec: skip=0, fetch=2 02)--NestedLoopJoinExec: join_type=Full, filter=c2@0 >= c2@1 -03)----MemoryExec: partitions=1, partition_sizes=[1] -04)----MemoryExec: partitions=1, partition_sizes=[1] +03)----GlobalLimitExec: skip=0, fetch=2 Review Comment: This plans is incorrect due to - https://github.com/apache/datafusion/issues/14335 (not this PR) ########## datafusion/physical-optimizer/src/limit_pushdown.rs: ########## @@ -247,7 +246,14 @@ pub fn pushdown_limit_helper( } } else { // Add fetch or a `LimitExec`: - global_state.satisfied = true; + // If the plan's children have limit, we shouldn't change the global state to true, + // because the children limit will be overridden if the global state is changed. + if pushdown_plan.children().iter().any(|child| { Review Comment: It looks to me like most of the rest of this rule is implemented in terms of `LimitExec` (defined in this file) to abstract away directly looking for `GlobalLimitExec` and `LocalLimitExec` I think you could do something like this instead to be more consistent with the rest of the codebase ```rust if pushdown_plan.children().iter().any(|&child| { extract_limit(child).is_some() }) { global_state.satisfied = false; } ``` However, this works too and fixes the bug so I think it is fine as is. ########## datafusion/sqllogictest/test_files/limit.slt: ########## @@ -711,3 +711,37 @@ OFFSET 3 LIMIT 2; statement ok drop table ordered_table; + +# Test limit pushdown with subquery +statement ok +create table testSubQueryLimit (a int, b int) as values (1,2), (2,3), (3,4); + +query IIII +select * from testSubQueryLimit as t1 join (select * from testSubQueryLimit limit 1) limit 10; +---- +1 2 1 2 +2 3 1 2 +3 4 1 2 + +query TT +explain select * from testSubQueryLimit as t1 join (select * from testSubQueryLimit limit 1) limit 10; +---- +logical_plan +01)Limit: skip=0, fetch=10 +02)--Cross Join: +03)----SubqueryAlias: t1 +04)------Limit: skip=0, fetch=10 +05)--------TableScan: testsubquerylimit projection=[a, b], fetch=10 +06)----Limit: skip=0, fetch=1 Review Comment: this plan looks good to me -- the Limit 1 is still here -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
