shivbhatia10 commented on code in PR #21057:
URL: https://github.com/apache/datafusion/pull/21057#discussion_r2969475128
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -796,6 +796,12 @@ impl OptimizerRule for PushDownFilter {
filter.predicate = new_predicate;
}
+ // If the child has a fetch (limit), pushing a filter below it would
+ // change semantics: the limit should apply before the filter, not
after.
+ if filter.input.fetch().is_some() {
Review Comment:
I think this makes a lot of sense, the reason I initially skipped `Limit`
was because we already handle it correctly in `PushDownFilter::rewrite`,
although very indirectly, because `Limit` is not covered in the match statement
and therefore it falls back to leaving the filter unchanged. However this is a
fragile assumption - I think the right thing to do here is to add another
method to `LogicalPlan` called `skip`, since `Limit` can have both fetch and
skip variants, and if either of them is non-empty we should block pushdown.
This is true in general for any logical plan node I believe. So I've added that
method, used the `get_skip_type` and `get_fetch_type` methods on `Limit` to
extract these `Optional<usize>` types, and now we check for the presence of
both skip and fetch in push down filter.
--
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]
