adriangb commented on code in PR #21267:
URL: https://github.com/apache/datafusion/pull/21267#discussion_r3281635155
##########
datafusion/physical-plan/src/coalesce_batches.rs:
##########
@@ -277,7 +277,13 @@ impl ExecutionPlan for CoalesceBatchesExec {
child_pushdown_result: ChildPushdownResult,
_config: &ConfigOptions,
) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
- Ok(FilterPushdownPropagation::if_all(child_pushdown_result))
+ let mut result =
FilterPushdownPropagation::if_all(child_pushdown_result);
+ if let Some(updated_child) = result.updated_node {
+ let mut new_self = self.clone();
+ new_self.input = updated_child;
+ result.updated_node = Some(Arc::new(new_self) as _);
+ }
+ Ok(result)
Review Comment:
@stuhood looking at this again I'm not sure this is needed / there is a real
bug. Do you have a regression test that fails?
`FilterPushdownPropagation::if_all` always sets `updated_node: None`:
https://github.com/apache/datafusion/blob/ad6a507beb0d9bcdd7c791524ba3af32be60c143/datafusion/physical-plan/src/filter_pushdown.rs#L238-L248
And then in the optimizer rule:
https://github.com/apache/datafusion/blob/ad6a507beb0d9bcdd7c791524ba3af32be60c143/datafusion/physical-optimizer/src/filter_pushdown.rs#L567-L568
https://github.com/apache/datafusion/blob/ad6a507beb0d9bcdd7c791524ba3af32be60c143/datafusion/physical-optimizer/src/filter_pushdown.rs#L591-L595
##########
datafusion/physical-plan/src/coalesce_batches.rs:
##########
@@ -277,7 +277,13 @@ impl ExecutionPlan for CoalesceBatchesExec {
child_pushdown_result: ChildPushdownResult,
_config: &ConfigOptions,
) -> Result<FilterPushdownPropagation<Arc<dyn ExecutionPlan>>> {
- Ok(FilterPushdownPropagation::if_all(child_pushdown_result))
+ let mut result =
FilterPushdownPropagation::if_all(child_pushdown_result);
+ if let Some(updated_child) = result.updated_node {
+ let mut new_self = self.clone();
+ new_self.input = updated_child;
+ result.updated_node = Some(Arc::new(new_self) as _);
+ }
+ Ok(result)
Review Comment:
So basically if any children have been updated we update the parent. Let me
know if I'm missing something.
--
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]
