perf(physical-plan): fold PlanProperties fast-path into with_new_children_if_necessary (PR 1 of #22555)

[zhuqi-lucas]

Which issue does this PR close?

Part of #22555. This is PR 1 of 2 — see the issue body for the full plan. PR 2 will audit direct with_new_children callers and add a clippy lint.

Rationale for this change

Today the "skip work when children are unchanged" intent is split across two layers:

• caller-side — with_new_children_if_necessary short-circuits via Arc::ptr_eq on child pointers.
• callee-side — the check_if_same_properties! macro from Cache PlanProperties, add fast-path for with_new_children #19792, invoked inside each impl's with_new_children, short-circuits when children's PlanProperties Arcs match (allowing the plan to reuse its cached PlanProperties Arc instead of recomputing).

Having two independent layers means two places to maintain and two places for future changes to drift apart. This PR consolidates the fast-path into the single free-function helper so callers get both short-circuits uniformly.

What changes are included in this PR?

with_new_children_if_necessary now applies three layers, cheapest first:

1. Same child pointers — every children[i] is Arc::ptr_eq to the corresponding existing child → return the original plan unchanged, no allocation.
2. Same child properties — children's PlanProperties Arcs match → call the new ExecutionPlan::with_new_children_and_same_properties trait method to reuse the plan's PlanProperties cache without recomputing.
3. Full recompute — otherwise, delegate to ExecutionPlan::with_new_children.

To make layer 2 dispatchable via &dyn ExecutionPlan, with_new_children_and_same_properties is promoted from an ad-hoc inherent method on each impl to a trait method with a safe default that falls back to with_new_children. All 22 existing impls migrate their inherent method to a trait override (mechanical change — signature &self → self: Arc<Self>, return Self → Result<Arc<dyn ExecutionPlan>>, body wrapped in Ok(Arc::new(...))).

The check_if_same_properties! macro and its call sites inside impls are kept, so direct callers of with_new_children (which PR 2 will audit + migrate) do not regress on this PR.

Are these changes tested?

Yes — added test_with_new_children_if_necessary_layers in execution_plan.rs that constructs test-local WithChildrenTestLeaf + WithChildrenTestParent plans (the parent tracks recompute vs fast-path calls via AtomicUsize) and asserts, for each of the three layers:

• Layer 1: Arc::ptr_eq(result, parent) returns true, recompute_calls == 0, fast_path_calls == 0
• Layer 2: Arc::ptr_eq(result.properties(), orig_props) returns true, recompute_calls == 0, fast_path_calls == 1
• Layer 3: Arc::ptr_eq(result.properties(), orig_props) returns false, recompute_calls == 1, fast_path_calls unchanged

All 1523 datafusion-physical-plan unit tests pass. Full workspace cargo check + cargo clippy --all-targets --all-features -- -D warnings pass.

Are there any user-facing changes?

Yes — ExecutionPlan gains a new default-implemented trait method with_new_children_and_same_properties. Downstream impls that used to override the ad-hoc inherent method with the same name will need to re-implement as a trait override (mechanical signature change). Marking as api change.

Follow-up (PR 2, not in this PR)

• Audit the ~47 remaining direct callers of plan.with_new_children(children) across the codebase and route them through with_new_children_if_necessary.
• Add a disallowed_methods clippy lint (or custom lint) that forbids direct ExecutionPlan::with_new_children outside of a small allow-list.
• Once all callers migrate, remove the check_if_same_properties! macro and its impl-side invocations, making the helper the single source of truth as described in the issue.

[Copilot]

Pull request overview

This PR consolidates the “skip work when rebuilding plans with unchanged children” logic into with_new_children_if_necessary by adding a new ExecutionPlan::with_new_children_and_same_properties trait method (defaulting to with_new_children) and migrating existing plan implementations to override it. It also updates the existing check_if_same_properties! macro to route through the new trait method and adds a unit test covering the intended 3-layer behavior.

Changes:

• Add ExecutionPlan::with_new_children_and_same_properties as a trait method (default implementation falls back to with_new_children).
• Update with_new_children_if_necessary to apply layered short-circuits (pointer-eq, properties-eq, then full recompute) and adjust has_same_children_properties to accept &dyn ExecutionPlan.
• Mechanically migrate various ExecutionPlan impls to implement with_new_children_and_same_properties, plus add a new unit test covering the layered behavior.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
datafusion/physical-plan/src/execution_plan.rs	Adds trait method + layered helper behavior + macro update + new unit test
datafusion/physical-plan/src/windows/window_agg_exec.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/unnest.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/union.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl (Union + Interleave)
datafusion/physical-plan/src/sorts/sort_preserving_merge.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/sorts/partial_sort.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/repartition/mod.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/projection.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/limit.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl (Global + Local)
datafusion/physical-plan/src/joins/symmetric_hash_join.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/joins/sort_merge_join/exec.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/joins/piecewise_merge_join/exec.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl and updates reset_state callsite
datafusion/physical-plan/src/joins/nested_loop_join.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/joins/cross_join.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/filter.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/coop.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/coalesce_partitions.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/coalesce_batches.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/buffer.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/async_func.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl
datafusion/physical-plan/src/aggregates/mod.rs	Moves with_new_children_and_same_properties into ExecutionPlan trait impl

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kosiew]

@zhuqi-lucas

Looks good to me.
I just have one small, non-blocking suggestion that could help strengthen the test coverage.

[kosiew]

Nice change! One small suggestion: it might be worth adding a test for a plan that does not override with_new_children_and_same_properties, where the replacement child has the same PlanProperties Arc. That would help show that the default fallback still goes through with_new_children and keeps external or custom ExecutionPlan implementations on the semantics-preserving path. Totally non-blocking though. The existing override-path coverage and cargo check already look good.

[zhuqi-lucas]

Good suggestion, thanks — done in e4328d9. Added WithChildrenTestParentDefault (a parent that intentionally does not override with_new_children_and_same_properties) plus test_with_new_children_if_necessary_default_fallback, which verifies that when the helper hits the same-properties branch on such a plan, the trait default forwards to with_new_children and the returned plan carries a freshly-recomputed PlanProperties Arc.

[zhuqi-lucas]

@zhuqi-lucas

Looks good to me. I just have one small, non-blocking suggestion that could help strengthen the test coverage.

Thanks @kosiew for review!

[alamb]

Thanks @zhuqi-lucas and @kosiew -- this does make sense to me though I have some thougths on the API design

[alamb]

do we really have to add a new method to the trait? I am thinking of someone who goes to implement an ExecutionPlan and is bewildered by all the possible things that need to be implemented

For example, what if we deprecated with_new_children and renamed with_new_children_and_same_properties to soemthing like replace_children (e.g. that is clear it is different than with_new_children)

That way, in a few releases from now, ExecutionPlan can be trimmed down (people will have to implement replace_children and remove their implementation of with_new_children but at least ExecutionPlan is simpler in the long term

[zhuqi-lucas]

Thanks @alamb for review, good point — filed as follow-up in #23441 (canonical replace_children + deprecate both existing methods + migrate ~30 impls). Prefer to keep this PR focused on the semantic change so the API rename gets its own review thread.