askalt commented on code in PR #23903:
URL: https://github.com/apache/datafusion/pull/23903#discussion_r3764097119
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -375,13 +468,18 @@ 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,
+ ReplaceChildrenOptions {
+ children_properties: ChildrenPropertiesMode::SameProperties,
+ },
Review Comment:
nit: we can add a constructor to reduce this a bit, e.g.
```
ReplaceChildrenOptions::new(ChildrenPropertiesMode::SameProperties)
```
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -936,6 +1034,27 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send +
Sync {
}
}
+/// Options for [`ExecutionPlan::replace_children`]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct ReplaceChildrenOptions {
+ /// Describes how plan properties should be handled for the replacement
+ /// children.
+ pub children_properties: ChildrenPropertiesMode,
+}
+
+/// Indicates whether the plan properties of the new children must be
recomputed.
+///
+/// Part of [`ReplaceChildrenOptions`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum ChildrenPropertiesMode {
+ /// The plan properties of the new children are identical to the properties
+ /// of the existing children, so we can skip recomputation.
+ SameProperties,
Review Comment:
nit: to be consistent with the second variant:
```suggestion
Keep,
```
--
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]
