askalt commented on code in PR #23903:
URL: https://github.com/apache/datafusion/pull/23903#discussion_r3743727412
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -264,14 +264,34 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send +
Sync {
/// joins).
fn children(&self) -> Vec<&Arc<dyn ExecutionPlan>>;
+ /// Returns a clone of the existing plan with the children replaced,
skipping
+ /// recomputation of plan properties if possible as indicated by the hint.
+ #[expect(deprecated)]
+ fn replace_children(
+ self: Arc<Self>,
+ children: Vec<Arc<dyn ExecutionPlan>>,
+ hint: ChildrenPropertiesHint,
Review Comment:
Would it make sense to pass a `ReplaceChildrenHints` structure here (that
includes `ChildrenPropertiesHint` as a member)? That could make the API easier
to extend in the future.
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -308,13 +333,13 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send +
Sync {
/// it will be called from within a walk of the execution plan tree so
that it will be called on each child later
/// or was already called on each child.
///
- /// Note to implementers: unlike [`ExecutionPlan::with_new_children`] this
method does not accept new children as an argument,
+ /// Note to implementers: unlike [`ExecutionPlan::replace_children`] this
method does not accept new children as an argument,
/// thus it is expected that any cached plan properties will remain valid
after the reset.
///
/// [`DynamicFilterPhysicalExpr`]:
datafusion_physical_expr::expressions::DynamicFilterPhysicalExpr
fn reset_state(self: Arc<Self>) -> Result<Arc<dyn ExecutionPlan>> {
let children = self.children().into_iter().cloned().collect();
- self.with_new_children(children)
+ self.replace_children(children, ChildrenPropertiesHint::Recompute)
Review Comment:
Why does `reset_state` require recomputing properties? It seems to me that
resetting runtime state should not affect the plan properties.
For performance, it would be nice to avoid recomputing properties in
`reset_plan_states`, especially for plans without an explicit `reset_state`
override. For example, a simple filter and projection should not need to
recompute anything when their state is reset.
##########
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:
A "hint" suggests that its absence should not logically break anything.
However, if children are not recomputed when required, it could lead to bugs.
Would `ChildrenPropertiesRequirement` be a better name?
--
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]
