You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "haohuaijin (via GitHub)" <gi...@apache.org> on 2026/02/04 07:07:20 UTC
[PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
haohuaijin opened a new pull request, #20145:
URL: https://github.com/apache/datafusion/pull/20145
## Which issue does this PR close?
Closes #20144
## Rationale for this change
see #20144
## What changes are included in this PR?
This PR impl `handle_child_pushdown_result` for `UnionExec`, for any case, the filter will always pushdown to UnionExec
## Are these changes tested?
yes, add two test cases
## Are there any user-facing changes?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2767281673
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -370,6 +376,66 @@ impl ExecutionPlan for UnionExec {
) -> Result<FilterDescription> {
FilterDescription::from_children(parent_filters, &self.children())
}
+
+ fn handle_child_pushdown_result(
+ &self,
+ phase: FilterPushdownPhase,
+ child_pushdown_result: ChildPushdownResult,
+ _config: &ConfigOptions,
+ ) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
+ // For non-Pre phase, use default behavior
+ if !matches!(phase, FilterPushdownPhase::Pre) {
Review Comment:
update comment in [8afe471](https://github.com/apache/datafusion/pull/20145/commits/8afe4710a2a901f6d2f032dfeac129237594c68d)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2762557905
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -1808,6 +1808,67 @@ fn test_filter_pushdown_through_union() {
);
}
+#[test]
+fn test_filter_pushdown_through_union_mixed_support() {
+ // Test case where one child supports filter pushdown and one doesn't
+ let scan1 = TestScanBuilder::new(schema()).with_support(true).build();
+ let scan2 = TestScanBuilder::new(schema()).with_support(false).build();
+
+ let union = UnionExec::try_new(vec![scan1, scan2]).unwrap();
+
+ let predicate = col_lit_predicate("a", "foo", &schema());
+ let plan = Arc::new(FilterExec::try_new(predicate, union).unwrap());
+
+ insta::assert_snapshot!(
+ OptimizationTest::new(plan, FilterPushdown::new(), true),
+ @r"
+ OptimizationTest:
+ input:
+ - FilterExec: a@0 = foo
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
+ output:
+ Ok:
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true, predicate=a@0 = foo
+ - FilterExec: a@0 = foo
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
Review Comment:
this is main purpose for this pr
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2764194379
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -1808,6 +1808,67 @@ fn test_filter_pushdown_through_union() {
);
}
+#[test]
+fn test_filter_pushdown_through_union_mixed_support() {
+ // Test case where one child supports filter pushdown and one doesn't
+ let scan1 = TestScanBuilder::new(schema()).with_support(true).build();
+ let scan2 = TestScanBuilder::new(schema()).with_support(false).build();
+
+ let union = UnionExec::try_new(vec![scan1, scan2]).unwrap();
+
+ let predicate = col_lit_predicate("a", "foo", &schema());
+ let plan = Arc::new(FilterExec::try_new(predicate, union).unwrap());
+
+ insta::assert_snapshot!(
+ OptimizationTest::new(plan, FilterPushdown::new(), true),
+ @r"
+ OptimizationTest:
+ input:
+ - FilterExec: a@0 = foo
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
+ output:
+ Ok:
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true, predicate=a@0 = foo
+ - FilterExec: a@0 = foo
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
Review Comment:
This one really seems like it should be reproducible from an SLT test
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Pushdown filters through `UnionExec` nodes [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #20145:
URL: https://github.com/apache/datafusion/pull/20145
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2762539490
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -1808,6 +1808,67 @@ fn test_filter_pushdown_through_union() {
);
}
+#[test]
+fn test_filter_pushdown_through_union_mixed_support() {
+ // Test case where one child supports filter pushdown and one doesn't
+ let scan1 = TestScanBuilder::new(schema()).with_support(true).build();
+ let scan2 = TestScanBuilder::new(schema()).with_support(false).build();
+
+ let union = UnionExec::try_new(vec![scan1, scan2]).unwrap();
+
+ let predicate = col_lit_predicate("a", "foo", &schema());
+ let plan = Arc::new(FilterExec::try_new(predicate, union).unwrap());
+
+ insta::assert_snapshot!(
+ OptimizationTest::new(plan, FilterPushdown::new(), true),
+ @r"
+ OptimizationTest:
+ input:
+ - FilterExec: a@0 = foo
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
+ output:
+ Ok:
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true, predicate=a@0 = foo
+ - FilterExec: a@0 = foo
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
+ "
+ );
+}
+
+#[test]
+fn test_filter_pushdown_through_union_does_not_support() {
+ // Test case where one child supports filter pushdown and one doesn't
+ let scan1 = TestScanBuilder::new(schema()).with_support(false).build();
+ let scan2 = TestScanBuilder::new(schema()).with_support(false).build();
+
+ let union = UnionExec::try_new(vec![scan1, scan2]).unwrap();
+
+ let predicate = col_lit_predicate("a", "foo", &schema());
+ let plan = Arc::new(FilterExec::try_new(predicate, union).unwrap());
+
+ insta::assert_snapshot!(
+ OptimizationTest::new(plan, FilterPushdown::new(), true),
+ @"
+ OptimizationTest:
+ input:
+ - FilterExec: a@0 = foo
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
+ output:
+ Ok:
+ - UnionExec
+ - FilterExec: a@0 = foo
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
+ - FilterExec: a@0 = foo
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
Review Comment:
i think even for this case, pushdown filter do not have some bad effect.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2764663514
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -1808,6 +1808,67 @@ fn test_filter_pushdown_through_union() {
);
}
+#[test]
+fn test_filter_pushdown_through_union_mixed_support() {
+ // Test case where one child supports filter pushdown and one doesn't
+ let scan1 = TestScanBuilder::new(schema()).with_support(true).build();
+ let scan2 = TestScanBuilder::new(schema()).with_support(false).build();
+
+ let union = UnionExec::try_new(vec![scan1, scan2]).unwrap();
+
+ let predicate = col_lit_predicate("a", "foo", &schema());
+ let plan = Arc::new(FilterExec::try_new(predicate, union).unwrap());
+
+ insta::assert_snapshot!(
+ OptimizationTest::new(plan, FilterPushdown::new(), true),
+ @r"
+ OptimizationTest:
+ input:
+ - FilterExec: a@0 = foo
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
+ output:
+ Ok:
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true, predicate=a@0 = foo
+ - FilterExec: a@0 = foo
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
Review Comment:
added in https://github.com/apache/datafusion/pull/20145/changes#diff-cc932d43efc8c2d251d018ecaa22d7e898b4da64d5f216c08a089e13cecfb67f
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2764363053
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -1808,6 +1808,67 @@ fn test_filter_pushdown_through_union() {
);
}
+#[test]
+fn test_filter_pushdown_through_union_mixed_support() {
+ // Test case where one child supports filter pushdown and one doesn't
+ let scan1 = TestScanBuilder::new(schema()).with_support(true).build();
+ let scan2 = TestScanBuilder::new(schema()).with_support(false).build();
+
+ let union = UnionExec::try_new(vec![scan1, scan2]).unwrap();
+
+ let predicate = col_lit_predicate("a", "foo", &schema());
+ let plan = Arc::new(FilterExec::try_new(predicate, union).unwrap());
+
+ insta::assert_snapshot!(
+ OptimizationTest::new(plan, FilterPushdown::new(), true),
+ @r"
+ OptimizationTest:
+ input:
+ - FilterExec: a@0 = foo
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
+ output:
+ Ok:
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true, predicate=a@0 = foo
+ - FilterExec: a@0 = foo
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
Review Comment:
let me try, maybe one csv and one parquet file can reproduce
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2764363053
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -1808,6 +1808,67 @@ fn test_filter_pushdown_through_union() {
);
}
+#[test]
+fn test_filter_pushdown_through_union_mixed_support() {
+ // Test case where one child supports filter pushdown and one doesn't
+ let scan1 = TestScanBuilder::new(schema()).with_support(true).build();
+ let scan2 = TestScanBuilder::new(schema()).with_support(false).build();
+
+ let union = UnionExec::try_new(vec![scan1, scan2]).unwrap();
+
+ let predicate = col_lit_predicate("a", "foo", &schema());
+ let plan = Arc::new(FilterExec::try_new(predicate, union).unwrap());
+
+ insta::assert_snapshot!(
+ OptimizationTest::new(plan, FilterPushdown::new(), true),
+ @r"
+ OptimizationTest:
+ input:
+ - FilterExec: a@0 = foo
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
+ output:
+ Ok:
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true, predicate=a@0 = foo
+ - FilterExec: a@0 = foo
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
Review Comment:
let me try, maybe one memory and one parquet file can reproduce
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2764368872
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -370,6 +376,66 @@ impl ExecutionPlan for UnionExec {
) -> Result<FilterDescription> {
FilterDescription::from_children(parent_filters, &self.children())
}
+
+ fn handle_child_pushdown_result(
+ &self,
+ phase: FilterPushdownPhase,
+ child_pushdown_result: ChildPushdownResult,
+ _config: &ConfigOptions,
+ ) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
+ // For non-Pre phase, use default behavior
+ if !matches!(phase, FilterPushdownPhase::Pre) {
+ return Ok(FilterPushdownPropagation::if_all(child_pushdown_result));
+ }
+
+ // Collect unsupported filters for each child
Review Comment:
Correct, thank you for such good explainđź‘Ť
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#issuecomment-3848255245
Thanks for you reviews @adriangb , i add the slt test case for reproduce.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2765311293
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -370,6 +376,66 @@ impl ExecutionPlan for UnionExec {
) -> Result<FilterDescription> {
FilterDescription::from_children(parent_filters, &self.children())
}
+
+ fn handle_child_pushdown_result(
+ &self,
+ phase: FilterPushdownPhase,
+ child_pushdown_result: ChildPushdownResult,
+ _config: &ConfigOptions,
+ ) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
+ // For non-Pre phase, use default behavior
+ if !matches!(phase, FilterPushdownPhase::Pre) {
+ return Ok(FilterPushdownPropagation::if_all(child_pushdown_result));
+ }
+
+ // Collect unsupported filters for each child
Review Comment:
Can you add something along these lines as a comment justifying the added complexity?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2764705202
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -370,6 +376,66 @@ impl ExecutionPlan for UnionExec {
) -> Result<FilterDescription> {
FilterDescription::from_children(parent_filters, &self.children())
}
+
+ fn handle_child_pushdown_result(
+ &self,
+ phase: FilterPushdownPhase,
+ child_pushdown_result: ChildPushdownResult,
+ _config: &ConfigOptions,
+ ) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
+ // For non-Pre phase, use default behavior
+ if !matches!(phase, FilterPushdownPhase::Pre) {
Review Comment:
i'm thinking the purpose for this pr can only happen in the pre phase, post phase is for dynamic filter, seem like not related.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2762557905
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -1808,6 +1808,67 @@ fn test_filter_pushdown_through_union() {
);
}
+#[test]
+fn test_filter_pushdown_through_union_mixed_support() {
+ // Test case where one child supports filter pushdown and one doesn't
+ let scan1 = TestScanBuilder::new(schema()).with_support(true).build();
+ let scan2 = TestScanBuilder::new(schema()).with_support(false).build();
+
+ let union = UnionExec::try_new(vec![scan1, scan2]).unwrap();
+
+ let predicate = col_lit_predicate("a", "foo", &schema());
+ let plan = Arc::new(FilterExec::try_new(predicate, union).unwrap());
+
+ insta::assert_snapshot!(
+ OptimizationTest::new(plan, FilterPushdown::new(), true),
+ @r"
+ OptimizationTest:
+ input:
+ - FilterExec: a@0 = foo
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
+ output:
+ Ok:
+ - UnionExec
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=true, predicate=a@0 = foo
+ - FilterExec: a@0 = foo
+ - DataSourceExec: file_groups={1 group: [[test.parquet]]}, projection=[a, b, c], file_type=test, pushdown_supported=false
Review Comment:
this is main propose for this pr
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2764240477
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -370,6 +376,66 @@ impl ExecutionPlan for UnionExec {
) -> Result<FilterDescription> {
FilterDescription::from_children(parent_filters, &self.children())
}
+
+ fn handle_child_pushdown_result(
+ &self,
+ phase: FilterPushdownPhase,
+ child_pushdown_result: ChildPushdownResult,
+ _config: &ConfigOptions,
+ ) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
+ // For non-Pre phase, use default behavior
+ if !matches!(phase, FilterPushdownPhase::Pre) {
+ return Ok(FilterPushdownPropagation::if_all(child_pushdown_result));
+ }
+
+ // Collect unsupported filters for each child
Review Comment:
In principle we could make `UnionExec` transparent similar to `CoalesceBatchesExec`.
Help me understand why we are adding the FilterExec here.
My guess is that you are trying to address the case of `Child2` supporting pushdown but `Child1` not supporting it.
Without this specialized implementation we would get:
```
FilterExec
UnionExec
Child1
Child2
```
i.e. no changes to the plan, not incorrect but we are applying filters to the output of Child2 that are unnecessary (it already applied these filters)
With this logic we get:
```
UnionExec
FilterExec
Child1
Child2
```
Which skips re-applying filters to the output of Child2.
Is this interpretation correct?
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -370,6 +376,66 @@ impl ExecutionPlan for UnionExec {
) -> Result<FilterDescription> {
FilterDescription::from_children(parent_filters, &self.children())
}
+
+ fn handle_child_pushdown_result(
+ &self,
+ phase: FilterPushdownPhase,
+ child_pushdown_result: ChildPushdownResult,
+ _config: &ConfigOptions,
+ ) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
+ // For non-Pre phase, use default behavior
+ if !matches!(phase, FilterPushdownPhase::Pre) {
Review Comment:
Why does the Pre / Post phase need different behavior?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2765310438
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -370,6 +376,66 @@ impl ExecutionPlan for UnionExec {
) -> Result<FilterDescription> {
FilterDescription::from_children(parent_filters, &self.children())
}
+
+ fn handle_child_pushdown_result(
+ &self,
+ phase: FilterPushdownPhase,
+ child_pushdown_result: ChildPushdownResult,
+ _config: &ConfigOptions,
+ ) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
+ // For non-Pre phase, use default behavior
+ if !matches!(phase, FilterPushdownPhase::Pre) {
Review Comment:
Agreed. The intuition here is to let the creator of the filter decide what to do with it.
I don't like that it makes assumptions about the implementation / what the creators of the filter want to do, but I don't see a better way to handle this.
I don't think forcing creation of the FilterExec would be good at least as things currently stand.
But we should add a comment explaining this.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2764705202
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -370,6 +376,66 @@ impl ExecutionPlan for UnionExec {
) -> Result<FilterDescription> {
FilterDescription::from_children(parent_filters, &self.children())
}
+
+ fn handle_child_pushdown_result(
+ &self,
+ phase: FilterPushdownPhase,
+ child_pushdown_result: ChildPushdownResult,
+ _config: &ConfigOptions,
+ ) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
+ // For non-Pre phase, use default behavior
+ if !matches!(phase, FilterPushdownPhase::Pre) {
Review Comment:
i'm thinking the purpose for this pr can only happen in the pre phase, post phase is for dynamic filter, seem like not related, so i keep the default behavior
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: impl `handle_child_pushdown_result` for UnionExec [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#discussion_r2766727646
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -370,6 +376,66 @@ impl ExecutionPlan for UnionExec {
) -> Result<FilterDescription> {
FilterDescription::from_children(parent_filters, &self.children())
}
+
+ fn handle_child_pushdown_result(
+ &self,
+ phase: FilterPushdownPhase,
+ child_pushdown_result: ChildPushdownResult,
+ _config: &ConfigOptions,
+ ) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
+ // For non-Pre phase, use default behavior
+ if !matches!(phase, FilterPushdownPhase::Pre) {
+ return Ok(FilterPushdownPropagation::if_all(child_pushdown_result));
+ }
+
+ // Collect unsupported filters for each child
Review Comment:
add in [ded7d9b](https://github.com/apache/datafusion/pull/20145/commits/ded7d9bf1beaafc7f1e1e419b10490bc48659d28)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Pushdown filters through `UnionExec` nodes [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20145:
URL: https://github.com/apache/datafusion/pull/20145#issuecomment-3872722275
Thanks @adriangb and @haohuaijin
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org