JSOD11 commented on code in PR #23903:
URL: https://github.com/apache/datafusion/pull/23903#discussion_r3744812786
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -869,6 +894,18 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send +
Sync {
}
}
+/// A hint from `replace_children_if_necessary` to `replace_children`
indicating
+/// whether the properties of the new children must be recomputed.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum ChildrenPropertiesHint {
Review Comment:
This is a good point, how about a design like this to tie in with your
comment above?
```
pub struct ReplaceChildrenOptions {
pub children_properties: ChildrenPropertiesMode,
}
pub enum ChildrenPropertiesMode {
SameProperties,
Recompute,
}
```
Which gives us something like this:
```
fn replace_children(
self: Arc<Self>,
children: Vec<Arc<dyn ExecutionPlan>>,
options: ReplaceChildrenOptions,
) -> Result<Arc<dyn ExecutionPlan>> {
match options.children_properties {
ChildrenPropertiesMode::SameProperties => {
self.with_new_children_and_same_properties(children)
}
ChildrenPropertiesMode::Recompute =>
self.with_new_children(children),
}
}
```
Interested in hearing what everyone thinks. If we agree on this design, I'll
go ahead and swap all the implementations and call sites cc @askalt
@zhuqi-lucas @alamb
--
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]
