perf(physical-plan): fold PlanProperties fast-path into with_new_children_if_necessary (PR 1 of #22555)#23332
Draft
zhuqi-lucas wants to merge 1 commit into
Draft
Conversation
…dren_if_necessary Route the "children are unchanged" short-circuit through a single helper instead of two independent layers (caller-side Arc::ptr_eq check plus a per-impl check_if_same_properties! fast-path from apache#19792). with_new_children_if_necessary now applies three layers, cheapest first: 1. same child pointers -> return the original plan unchanged 2. same child PlanProperties Arcs -> reuse the plan's PlanProperties cache via with_new_children_and_same_properties 3. otherwise -> full with_new_children recompute To make layer 2 dispatchable through &dyn ExecutionPlan, promote with_new_children_and_same_properties from an ad-hoc inherent method on each impl to a trait method on ExecutionPlan with a safe default that falls back to full recompute. All 22 existing impls migrate to overrides. The check_if_same_properties! macro is kept for direct callers of with_new_children until PR 2 audits them and adds a clippy lint (see issue apache#22555 for the plan). Part of apache#22555.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_childrencallers and add a clippy lint.Rationale for this change
Today the "skip work when children are unchanged" intent is split across two layers:
with_new_children_if_necessaryshort-circuits viaArc::ptr_eqon child pointers.check_if_same_properties!macro from CachePlanProperties, add fast-path forwith_new_children#19792, invoked inside each impl'swith_new_children, short-circuits when children'sPlanPropertiesArcs match (allowing the plan to reuse its cachedPlanPropertiesArc 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_necessarynow applies three layers, cheapest first:children[i]isArc::ptr_eqto the corresponding existing child → return the original plan unchanged, no allocation.PlanPropertiesArcs match → call the newExecutionPlan::with_new_children_and_same_propertiestrait method to reuse the plan'sPlanPropertiescache without recomputing.ExecutionPlan::with_new_children.To make layer 2 dispatchable via
&dyn ExecutionPlan,with_new_children_and_same_propertiesis promoted from an ad-hoc inherent method on each impl to a trait method with a safe default that falls back towith_new_children. All 22 existing impls migrate their inherent method to a trait override (mechanical change — signature&self → self: Arc<Self>, returnSelf → Result<Arc<dyn ExecutionPlan>>, body wrapped inOk(Arc::new(...))).The
check_if_same_properties!macro and its call sites inside impls are kept, so direct callers ofwith_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_layersinexecution_plan.rsthat constructs test-localWithChildrenTestLeaf+WithChildrenTestParentplans (the parent tracks recompute vs fast-path calls viaAtomicUsize) and asserts, for each of the three layers:Arc::ptr_eq(result, parent)returns true,recompute_calls == 0,fast_path_calls == 0Arc::ptr_eq(result.properties(), orig_props)returns true,recompute_calls == 0,fast_path_calls == 1Arc::ptr_eq(result.properties(), orig_props)returns false,recompute_calls == 1,fast_path_callsunchangedAll 1523
datafusion-physical-planunit tests pass. Full workspacecargo check+cargo clippy --all-targets --all-features -- -D warningspass.Are there any user-facing changes?
Yes —
ExecutionPlangains a new default-implemented trait methodwith_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 asapi change.Follow-up (PR 2, not in this PR)
plan.with_new_children(children)across the codebase and route them throughwith_new_children_if_necessary.disallowed_methodsclippy lint (or custom lint) that forbids directExecutionPlan::with_new_childrenoutside of a small allow-list.check_if_same_properties!macro and its impl-side invocations, making the helper the single source of truth as described in the issue.