zhuqi-lucas opened a new pull request, #23332: URL: https://github.com/apache/datafusion/pull/23332
## 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`](https://github.com/apache/datafusion/blob/main/datafusion/physical-plan/src/execution_plan.rs) short-circuits via `Arc::ptr_eq` on child pointers. - **callee-side** — the `check_if_same_properties!` macro from #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. -- 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]
