Copilot commented on code in PR #23332:
URL: https://github.com/apache/datafusion/pull/23332#discussion_r3549268004
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -1279,16 +1318,22 @@ pub fn with_new_children_if_necessary(
old_children.len(),
"Wrong number of children"
);
- if children.is_empty()
- || children
+ if !children.is_empty() {
+ // Layer 1: same child pointers → return the plan unchanged.
+ if children
.iter()
.zip(old_children.iter())
- .any(|(c1, c2)| !Arc::ptr_eq(c1, c2))
- {
- plan.with_new_children(children)
- } else {
- Ok(plan)
+ .all(|(c1, c2)| Arc::ptr_eq(c1, c2))
+ {
+ return Ok(plan);
+ }
+ // 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);
+ }
}
+ // Layer 3: full recompute.
+ plan.with_new_children(children)
Review Comment:
`with_new_children_if_necessary` skips the layer-1 `Arc::ptr_eq`
short-circuit when `children` is empty. For leaf plans (0 children), this will
always fall through to `with_new_children(vec![])`, which can unnecessarily
reset metrics / state and defeats the helper’s “no work if unchanged” contract.
The layer-1 check should run even for empty child lists (the `all(...)` is
vacuously true).
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -1551,15 +1596,20 @@ pub fn has_same_children_properties(
/// Helper macro to avoid properties re-computation if passed children
properties
/// the same as plan already has. Could be used to implement fast-path for
method
/// [`ExecutionPlan::with_new_children`].
+///
+/// New call sites should route through [`with_new_children_if_necessary`],
+/// which applies this check together with the child-pointer short-circuit
+/// (see [`with_new_children_if_necessary`] for the layered policy). This
+/// macro remains for direct-caller sites that have not been migrated yet.
#[macro_export]
macro_rules! check_if_same_properties {
($plan: expr, $children: expr) => {
if $crate::execution_plan::has_same_children_properties(
$plan.as_ref(),
&$children,
)? {
- let plan = $plan.with_new_children_and_same_properties($children);
- return Ok(::std::sync::Arc::new(plan));
+ return ::std::sync::Arc::clone(&$plan)
+ .with_new_children_and_same_properties($children);
}
Review Comment:
`check_if_same_properties!` currently does an extra `Arc::clone(&$plan)`
before calling `with_new_children_and_same_properties`. Since the macro
immediately `return`s on this path, it can move `$plan` directly and avoid an
extra atomic refcount inc/dec in the fast-path.
--
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]
