Follow-up to #23332
Per @alamb's review, landing PR #23332 leaves the ExecutionPlan trait with two methods that each build a new plan from new children:
fn with_new_children(self: Arc<Self>, children) -> Result<...> — always recomputes PlanProperties.
fn with_new_children_and_same_properties(self: Arc<Self>, children) -> Result<...> — reuses PlanProperties; default falls back to with_new_children.
For someone implementing ExecutionPlan, seeing both is confusing ("which one do I override, and when?"). Long-term the trait should expose a single canonical method.
Proposal
- Introduce a canonical entry point, e.g.
replace_children(self: Arc<Self>, children, hint: ChildrenPropertiesHint) -> Result<Arc<dyn ExecutionPlan>>, where the hint encodes "same properties, reuse cache" vs "recompute".
- Deprecate both
with_new_children and with_new_children_and_same_properties with clear migration notes pointing at replace_children.
- Migrate the ~30 in-tree
ExecutionPlan impls and all direct callers to the new method.
- After a couple of releases, drop the deprecated methods, leaving the trait with a single canonical children-replacement primitive.
The with_new_children_if_necessary helper stays the caller-facing API and internally routes to replace_children with the appropriate hint.
Why not fold this into #23332
- The mechanical migration touches ~30 files; keeping it as a separate PR lets the semantic change (fast-path skip) land first and be reviewed on its own.
- The API rename is a naming discussion worth its own review thread.
Related
- PR #23332 — semantic change this issue builds on.
- Umbrella #22555 —
with_new_children_if_necessary consolidation.
cc @alamb
Follow-up to #23332
Per @alamb's review, landing PR #23332 leaves the
ExecutionPlantrait with two methods that each build a new plan from new children:fn with_new_children(self: Arc<Self>, children) -> Result<...>— always recomputesPlanProperties.fn with_new_children_and_same_properties(self: Arc<Self>, children) -> Result<...>— reusesPlanProperties; default falls back towith_new_children.For someone implementing
ExecutionPlan, seeing both is confusing ("which one do I override, and when?"). Long-term the trait should expose a single canonical method.Proposal
replace_children(self: Arc<Self>, children, hint: ChildrenPropertiesHint) -> Result<Arc<dyn ExecutionPlan>>, where the hint encodes "same properties, reuse cache" vs "recompute".with_new_childrenandwith_new_children_and_same_propertieswith clear migration notes pointing atreplace_children.ExecutionPlanimpls and all direct callers to the new method.The
with_new_children_if_necessaryhelper stays the caller-facing API and internally routes toreplace_childrenwith the appropriate hint.Why not fold this into #23332
Related
with_new_children_if_necessaryconsolidation.cc @alamb