adriangb commented on code in PR #20110:
URL: https://github.com/apache/datafusion/pull/20110#discussion_r2759202862
##########
datafusion/physical-plan/src/filter.rs:
##########
@@ -623,10 +623,26 @@ impl ExecutionPlan for FilterExec {
return
Ok(FilterPushdownPropagation::if_all(child_pushdown_result));
}
// We absorb any parent filters that were not handled by our children
- let unsupported_parent_filters =
- child_pushdown_result.parent_filters.iter().filter_map(|f| {
- matches!(f.all(),
PushedDown::No).then_some(Arc::clone(&f.filter))
- });
+ let mut unsupported_parent_filters: Vec<Arc<dyn PhysicalExpr>> =
+ child_pushdown_result
+ .parent_filters
+ .iter()
+ .filter_map(|f| {
+ matches!(f.all(),
PushedDown::No).then_some(Arc::clone(&f.filter))
+ })
+ .collect();
Review Comment:
Note: this is the same code, just added the `mut` which caused a reformat.
##########
datafusion/physical-plan/src/filter.rs:
##########
@@ -623,10 +623,26 @@ impl ExecutionPlan for FilterExec {
return
Ok(FilterPushdownPropagation::if_all(child_pushdown_result));
}
// We absorb any parent filters that were not handled by our children
- let unsupported_parent_filters =
- child_pushdown_result.parent_filters.iter().filter_map(|f| {
- matches!(f.all(),
PushedDown::No).then_some(Arc::clone(&f.filter))
- });
+ let mut unsupported_parent_filters: Vec<Arc<dyn PhysicalExpr>> =
+ child_pushdown_result
+ .parent_filters
+ .iter()
+ .filter_map(|f| {
+ matches!(f.all(),
PushedDown::No).then_some(Arc::clone(&f.filter))
+ })
+ .collect();
+
+ // If this FilterExec has a projection, the unsupported parent filters
+ // are in the output schema (after projection) coordinates. We need to
+ // remap them to the input schema coordinates before combining with
self filters.
+ if self.projection.is_some() {
Review Comment:
👍🏻 note that this is because filters get evaluated before the projection
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -3720,3 +3720,90 @@ async fn test_hashjoin_dynamic_filter_pushdown_is_used()
{
);
}
}
+
+/// related to https://github.com/apache/datafusion/issues/20109
+#[tokio::test]
+async fn test_filter_with_projection_pushdown() {
Review Comment:
Any chance you could add an SLT test that reproduces this? It could go in
datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -3720,3 +3720,90 @@ async fn test_hashjoin_dynamic_filter_pushdown_is_used()
{
);
}
}
+
+/// related to https://github.com/apache/datafusion/issues/20109
Review Comment:
```suggestion
/// Regression test for https://github.com/apache/datafusion/issues/20109
```
--
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]
