alamb commented on code in PR #23903:
URL: https://github.com/apache/datafusion/pull/23903#discussion_r3761127788
##########
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:
looks good to me
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -1560,11 +1607,29 @@ pub fn with_new_children_if_necessary(
}
// Layer 2: same child properties → reuse `PlanProperties` cache.
if has_same_children_properties(plan.as_ref(), &children)? {
- return plan.with_new_children_and_same_properties(children);
+ return plan.replace_children(
+ children,
+ ReplaceChildrenOptions {
+ children_properties:
ChildrenPropertiesMode::SameProperties,
+ },
+ );
}
}
// Layer 3: full recompute.
- plan.with_new_children(children)
+ plan.replace_children(
+ children,
+ ReplaceChildrenOptions {
+ children_properties: ChildrenPropertiesMode::Recompute,
+ },
+ )
+}
+
+#[deprecated(since = "55.0.0", note = "Use `replace_children_if_necessary`")]
+pub fn with_new_children_if_necessary(
Review Comment:
💯 for keeping the old stub
##########
docs/source/library-user-guide/upgrading/55.0.0.md:
##########
@@ -576,6 +576,78 @@ See [PR
#22733](https://github.com/apache/datafusion/pull/22733) for
details, including the per-variant size breakdown and benchmark
results.
+### `ExecutionPlan::with_new_children` and
`ExecutionPlan::with_new_children_and_same_properties` deprecated
+
+`with_new_children` and `with_new_children_and_same_properties` have been
+deprecated. These methods are used to replace the child plans of an
+`ExecutionPlan` while leaving the plan otherwise identical.
+
+`with_new_children_if_necessary` has also been deprecated in favor of
+`replace_children_if_necessary` for consistency in naming.
+
+As noted
[here](https://github.com/apache/datafusion/pull/23332#discussion_r3554897693),
+while the addition of `with_new_children_and_same_properties` has the benefit
+of skipping potentially expensive computation in the case that replacement
children
+have the same properties as the original children, it widens the API surface
area
+of `ExecutionPlan` in a way that could be confusing for users.
+
+Thus, to rectify this, we unify these methods by introducing
`replace_children`.
+`replace_children` solves this problem by taking `ReplaceChildrenOptions`,
+which includes a `ChildrenPropertiesMode`. The mode has two variants,
+`SameProperties` and `Recompute`, which tell `replace_children` whether plan
+properties can be reused or need to be recomputed.
+
+This method is called from `replace_children_if_necessary`, which is the
+standard entry point that should be used for replacing the children of a node.
+
+**Migration guide:**
+
+To migrate from `with_new_children` and `with_new_children_and_same_properties`
+to `replace_children`, it is recommended to implement `replace_children` with
+a `match` statement matching on the `ChildrenPropertiesMode`. In the case that
+the properties match the children, `ChildrenPropertiesMode::SameProperties`,
+follow the body of `with_new_children_and_same_properties`. In the case that
+the properties do not match the children, `ChildrenPropertiesMode::Recompute`,
+follow the body of `with_new_children`.
+
+For example, take a look at the implementation for `FilterExec`:
Review Comment:
👍
--
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]
