Commit 24d8957
authored
## 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.
1 parent 7ac784f commit 24d8957
22 files changed
Lines changed: 682 additions & 305 deletions
File tree
- datafusion/physical-plan/src
- aggregates
- joins
- piecewise_merge_join
- sort_merge_join
- repartition
- sorts
- windows
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1610 | 1610 | | |
1611 | 1611 | | |
1612 | 1612 | | |
1613 | | - | |
1614 | | - | |
1615 | | - | |
1616 | | - | |
1617 | | - | |
1618 | | - | |
1619 | | - | |
1620 | | - | |
1621 | | - | |
1622 | | - | |
1623 | | - | |
1624 | 1613 | | |
1625 | 1614 | | |
1626 | 1615 | | |
| |||
1838 | 1827 | | |
1839 | 1828 | | |
1840 | 1829 | | |
| 1830 | + | |
| 1831 | + | |
| 1832 | + | |
| 1833 | + | |
| 1834 | + | |
| 1835 | + | |
| 1836 | + | |
| 1837 | + | |
| 1838 | + | |
| 1839 | + | |
| 1840 | + | |
1841 | 1841 | | |
1842 | 1842 | | |
1843 | 1843 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
113 | 113 | | |
114 | 114 | | |
115 | 115 | | |
116 | | - | |
117 | | - | |
118 | | - | |
119 | | - | |
120 | | - | |
121 | | - | |
122 | | - | |
123 | | - | |
124 | | - | |
125 | | - | |
126 | | - | |
127 | 116 | | |
128 | 117 | | |
129 | 118 | | |
| |||
180 | 169 | | |
181 | 170 | | |
182 | 171 | | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
183 | 183 | | |
184 | 184 | | |
185 | 185 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
123 | 123 | | |
124 | 124 | | |
125 | 125 | | |
126 | | - | |
127 | | - | |
128 | | - | |
129 | | - | |
130 | | - | |
131 | | - | |
132 | | - | |
133 | | - | |
134 | | - | |
135 | | - | |
136 | | - | |
137 | 126 | | |
138 | 127 | | |
139 | 128 | | |
| |||
181 | 170 | | |
182 | 171 | | |
183 | 172 | | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
184 | 184 | | |
185 | 185 | | |
186 | 186 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
117 | 117 | | |
118 | 118 | | |
119 | 119 | | |
120 | | - | |
121 | | - | |
122 | | - | |
123 | | - | |
124 | | - | |
125 | | - | |
126 | | - | |
127 | | - | |
128 | | - | |
129 | | - | |
130 | | - | |
131 | 120 | | |
132 | 121 | | |
133 | 122 | | |
| |||
195 | 184 | | |
196 | 185 | | |
197 | 186 | | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
198 | 198 | | |
199 | 199 | | |
200 | 200 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
100 | 100 | | |
101 | 101 | | |
102 | 102 | | |
103 | | - | |
104 | | - | |
105 | | - | |
106 | | - | |
107 | | - | |
108 | | - | |
109 | | - | |
110 | | - | |
111 | | - | |
112 | | - | |
113 | | - | |
114 | 103 | | |
115 | 104 | | |
116 | 105 | | |
| |||
164 | 153 | | |
165 | 154 | | |
166 | 155 | | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
167 | 167 | | |
168 | 168 | | |
169 | 169 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
234 | 234 | | |
235 | 235 | | |
236 | 236 | | |
237 | | - | |
238 | | - | |
239 | | - | |
240 | | - | |
241 | | - | |
242 | | - | |
243 | | - | |
244 | | - | |
245 | | - | |
246 | | - | |
247 | 237 | | |
248 | 238 | | |
249 | 239 | | |
| |||
290 | 280 | | |
291 | 281 | | |
292 | 282 | | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
293 | 293 | | |
294 | 294 | | |
295 | 295 | | |
| |||
0 commit comments