adriangb commented on code in PR #25760:
URL: https://github.com/apache/datafusion/pull/25760#discussion_r4137276342
##########
datafusion/physical-optimizer/src/filter_pushdown.rs:
##########
@@ -605,6 +640,39 @@ fn push_down_filters(
Ok(res)
}
+/// Debug check for the order rule that the [`FilterConjunct`] properties rely
+/// on: result `i` of a node must describe input filter `i`.
+///
+/// A node can rewrite a filter (for example, remap its columns), thus this
+/// check cannot compare expressions. It finds results that are the same
+/// `Arc` as a different input filter, which is what a node that reorders its
+/// parent filters returns.
+fn check_parent_filter_order(
+ node: &Arc<dyn ExecutionPlan>,
+ child_idx: usize,
+ results: &[PushedDownPredicate],
Review Comment:
Yes, good call. It turns out we *have* to migrate the whole API in this PR
or we'd end up with wonkiness. Luckily getting it wrong, or a plan that forgets
to migrate, can only result in loosing the optionality metadata, so performance
not incorrect results.
Now the change goes all the way through pushdown:
- `ExecutionPlan::gather_filters_for_pushdown` now receives
`Vec<FilterConjunct>`.
- `PushedDownPredicate::predicate` and `ChildFilterPushdownResult::filter`
are `FilterConjunct`s.
- A node that rewrites a parent filter (projection, hash join key transfer,
column remap) keeps its properties with `FilterConjunct::with_expr`.
--
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]
