Uh oh!
There was an error while loading. Please reload this page.
Cache PlanProperties, add fast-path for with_new_children - #19792
Merged
Conversation
with_new_childrenwith_new_childrenaskalt
marked this pull request as draft
January 13, 2026 14:26
askaltforce-pushed
the
askalt/with_new_children_fast_path
branch
from
January 13, 2026 15:20
72ff575 to
796f731Compareaskalt
commented
Jan 13, 2026
ContributorAuthor
Also added a typical analytical query plan re-usage benchmark. On the $ cargo bench --profile=release-nonlto --bench plan_reuse |
askaltforce-pushed
the
askalt/with_new_children_fast_path
branch
from
January 13, 2026 15:24
796f731 to
5601c4fComparealamb
commented
Jan 13, 2026
Contributor
I filed a ticket to track this idea |
alamb
commented
Jan 13, 2026
Contributor
run benchmark sql_planner |
alamb-ghbot
commented
Jan 13, 2026
🤖 |
44 tasks
alamb
commented
Jan 13, 2026
Contributor
Could you move the plan_reuse benchmark into its own PR (as I think it is valuable both for this PR and others, and it makes it easier to automatically compare performance) |
alamb-ghbot
commented
Jan 13, 2026
Benchmark script failed with exit code 101. Last 10 lines of output: Click to expand |
askalt
commented
Jan 14, 2026
ContributorAuthor
Done in #19806 |
askaltforce-pushed
the
askalt/with_new_children_fast_path
branch
from
January 14, 2026 10:32
5601c4f to
99cf634CompareContributorAuthor
askaltforce-pushed
the
askalt/with_new_children_fast_path
branch
from
January 14, 2026 14:49
99cf634 to
b81dd66Comparealamb
reviewed
Jan 14, 2026
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thanks @askalt -- this is quite clever and I think it looks very promising
I also think we may be able to potentially make with_new_children even faster by checking the children as well -- and if they are the same there is no reason to recompute everything either.
However, this likely won't help your usecase as the children will likely change (their states need to be reset) 🤔
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
askaltforce-pushed
the
askalt/with_new_children_fast_path
branch
from
January 15, 2026 11:48
b81dd66 to
741b085Compareaskalt
marked this pull request as ready for review
January 15, 2026 11:49
alamb
commented
Feb 24, 2026
Contributor
I plan to merge this later today unless anyone else would like more time to review |
ContributorAuthor
Thank you! Sorry for the delay, I checked the patches, they look good to me. |
alamb
commented
Feb 24, 2026
Contributor
It was my bad -- I was out last week I just merged up again to resolve a conflict |
alamb
commented
Feb 24, 2026
Contributor
run benchmark sql_planner |
alamb
commented
Feb 24, 2026
Contributor
Thanks again @askalt |
Uh oh!
There was an error while loading. Please reload this page.
alamb-ghbot
commented
Feb 24, 2026
🤖 |
alamb-ghbot
commented
Feb 24, 2026
🤖: Benchmark completed Details |
alamb
commented
Feb 25, 2026
Contributor
The benchmark results make this look like an across the board won I would say for planning speed |
alamb-ghbot
commented
Mar 21, 2026
🤖 |
alamb-ghbot
commented
Mar 21, 2026
🤖: Benchmark completed Details |
alamb-ghbot
commented
Mar 21, 2026
🤖 |
alamb
commented
Mar 21, 2026
Contributor
(sorry for the benchmark noise -- I had some script issues) |
de-bgunter pushed a commit
to de-bgunter/datafusion
that referenced
this pull request
Mar 24, 2026
…#19792) - closesapache#19796 This patch aims to implement a fast-path for the ExecutionPlan::with_new_children function for some plans, moving closer to a physical plan re-use implementation and improving planning performance. If the passed children properties are the same as in self, we do not actually recompute self's properties (which could be costly if projection mapping is required). Instead, we just replace the children and re-use self's properties as-is. To be able to compare two different properties -- ExecutionPlan::properties(...) signature is modified and now returns `&Arc<PlanProperties>`. If `children` properties are the same in `with_new_children` -- we clone our properties arc and then a parent plan will consider our properties as unchanged, doing the same. - Return `&Arc<PlanProperties>` from `ExecutionPlan::properties(...)` instead of a reference. - Implement `with_new_children` fast-path if there is no children properties changes for all major plans. Note: currently, `reset_plan_states` does not allow to re-use plan in general: it is not supported for dynamic filters and recursive queries features, as in this case state reset should update pointers in the children plans. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
pullBot
pushed a commit
to Stars1233/datafusion
that referenced
this pull request
Jul 10, 2026
…dren_if_necessary (PR 1 of apache#22555) (apache#23332) ## Which issue does this PR close? Part of apache#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 apache#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.
epgif pushed a commit
to influxdata/arrow-datafusion
that referenced
this pull request
Aug 14, 2026
UnionExec::try_new recomputes the union schema via union_schema() on every construction, including with_new_children rebuilds performed by optimizer passes. For wide unions whose children share one schema (generated UNION ALL, unions of per-partition scans) this makes physical planning O(n^2) in child count. Two fast paths: - union_schema(): if every input schema is pointer- or content-equal to the first, return the first schema (merging N identical schemas is the identity operation). - UnionExec::with_new_children(): when child count and per-position child schemas are unchanged, reuse the existing schema. Plan properties are always recomputed, since they can legitimately change when schemas do not. (DataFusion 53 has an analogous properties optimization upstream, apache#19792; this variant targets the 51 line.) Adds a union_schema benchmark covering shared-Arc, content-equal, and adversarial (last child differs) shapes. All existing union unit tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alamb pushed a commit
to influxdata/arrow-datafusion
that referenced
this pull request
Aug 14, 2026
UnionExec::try_new recomputes the union schema via union_schema() on every construction, including with_new_children rebuilds performed by optimizer passes. For wide unions whose children share one schema (generated UNION ALL, unions of per-partition scans) this makes physical planning O(n^2) in child count. Two fast paths: - union_schema(): if every input schema is pointer- or content-equal to the first, return the first schema (merging N identical schemas is the identity operation). - UnionExec::with_new_children(): when child count and per-position child schemas are unchanged, reuse the existing schema. Plan properties are always recomputed, since they can legitimately change when schemas do not. (DataFusion 53 has an analogous properties optimization upstream, apache#19792; this variant targets the 51 line.) Adds a union_schema benchmark covering shared-Arc, content-equal, and adversarial (last child differs) shapes. All existing union unit tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ryux1 pushed a commit
to ryux1/datafusion
that referenced
this pull request
Aug 15, 2026
…n share a schema (apache#24389) ## Which issue does this close? Complements apache#19792. Fits with the wide-`UnionExec` planning-cost work, but originates from an InfluxDB issue. ## Rationale for this change `union_schema` builds the output schema for `UnionExec` and `InterleaveExec` by coercing field metadata and nullability across **every** child. That merge is quadratic in the number of children: for each output field it walks all inputs, and for each input it walks *every other* input to union field-level metadata. For a union of `n` children with `f` fields the construction cost is `O(n^2 * f)` (worse when fields carry metadata). For narrow unions this is insignificant. It matters when a plan fans a single source out into many identical-schema children and unions them back together -- e.g. a union assembled from repartitioned copies of the same input. An instance like this occurred with InfluxDB: every child schema was the same, so the merge, guaranteed to reproduce the first child's schema, unnecessarily incurred the planning latency penalty from `O(n^2 * f)` complexity. ### Relationship to apache#19792 `UnionExec` construction has two quadratic halves: - **`with_new_children` / `PlanProperties`** -- addressed by apache#19792 (`with_new_children_and_same_properties`, `Arc<PlanProperties>`, the properties fast path). Already on `main`. - **`union_schema`** -- *not* covered by apache#19792 and still quadratic on `main`. This PR complements it by making `union_schema` skip the merge when it can't change the result. It deliberately doesn't touch `with_new_children`; that path is already handled. ## What changes are included? A fast path at the top of `union_schema`: after taking `inputs[0].schema()`, if every remaining child's schema is either the **same allocation** (`Arc::ptr_eq`) or **structurally equal** (`==`) to the first, return the first schema immediately. Otherwise we fall through to the existing full merge, so behavior for genuinely heterogeneous unions is byte-for-byte unchanged. ```rust let first_schema = inputs[0].schema(); if inputs[1..].iter().all(|input| { let schema = input.schema(); Arc::ptr_eq(&schema, &first_schema) || schema == first_schema }) { return Ok(first_schema); } ``` `InterleaveExec` shares `union_schema`, so it gets the same speedup for free. ## On the cost of the deep compare... The natural objection (which came up before this PR): **doesn't the deep `==` make the *unequal* case slower?** I'll paraphrase the prior conclusions, risking verbosity to avoid rehashing the discussion. Spoiler: it's not an issue. - The equal case avoids the merge, and its check is cheap. The shared-`Arc` case is settled by pointer comparison. The distinct-but-equal case runs `Schema::eq`, which is allocation-free and short-circuits on the first difference. Benchmarks show a small loss versus a pointer-equality-only control (the theoretical floor) but it still beats the full merge by a wide margin, and that advantage grows with schema complexity. - The adversarial worst case is bounded. The one shape where the scan is pure overhead is `last_differs`: children `0..n-1` are equal and the last diverges, so we scan `n` schemas, fail on the last, then merge anyway. That's a single linear `==` pass bounded by the merge that follows -- a constant fraction, not another factor of `n` -- and it takes *thousands* of near-identical children differing only in the last to hit. - Ordinary unequal unions fail fast. `SELECT a ... UNION ALL SELECT b ...` differs at field 0, so `==` rejects on the first field (see `names_differ`). And `UnionExec::try_new` already rejects misaligned children, so the only divergence `union_schema` ever sees is top-level (caught in the first pass). ## Benchmark results New bench `datafusion/physical-plan/benches/union_schema.rs` measures `UnionExec::try_new` construction over a flat schema and a nested/struct schema, for the four child shapes above. Run interleaved (baseline / patched alternated per cell) on a fixed-clock T2D VM. ### `union_schema` construction (lower is better) | scenario | n | baseline (ms) | patched (ms) | change | |---|---|---|---|---| | union_exec_try_new/shared_arc | 100 | 0.263 | 0.042 | 6.2× | | union_exec_try_new/shared_arc | 1000 | 2.60 | 0.429 | 6.1× | | union_exec_try_new/shared_arc | 4000 | 10.5 | 1.74 | 6.0× | | union_exec_try_new/content_equal | 100 | 0.262 | 0.042 | 6.2× | | union_exec_try_new/content_equal | 1000 | 2.61 | 0.433 | 6.0× | | union_exec_try_new/content_equal | 4000 | 10.5 | 1.74 | 6.0× | | union_exec_try_new/last_differs | 4000 | ~98 | ~97 | flat (±1.5%) | | union_exec_try_new/names_differ | 4000 | ~87 | ~87 | flat (±1%) | | union_exec_try_new_nested/content_equal | 1000 | 2.68 | 0.431 | 6.2× | | union_exec_try_new_nested/content_equal | 4000 | 10.8 | 1.73 | 6.2× | The `last_differs` (adversarial: N-1 children equal, deep compare then full merge) and `names_differ` (typical unequal: fails on the first field) cells were re-measured with tight per-cell interleaving (baseline/patched adjacent, 4 rounds) to control for variance: both are within ±1.5%, straddling zero. Interpretation: the **deep compare cost isn't observable end to end**. ### End-to-end planning: no regression (`sql_planner`) `cargo bench --bench sql_planner` (TPC-H + ClickBench) run baseline vs patched on the fixed-clock T2D VM. Every case lands within ±1% -- run-to-run noise -- with no case regressing beyond that noise. Notable rows, including the union-heavy `sorted_union` cases the fast path is meant to help: | case | baseline (ms) | patched (ms) | |---|---|---| | physical_plan_tpcds_all | 995.9 ± 1.4 | 991.9 ± 1.9 | | physical_plan_tpch_all | 60.4 ± 0.2 | 60.3 ± 0.1 | | physical_sorted_union_order_by_50 | 349.9 ± 2.4 | 346.3 ± 2.7 | | physical_sorted_union_order_by_10 | 12.3 ± 0.03 | 12.2 ± 0.07 | | physical_select_all_from_1000 | 30.8 ± 0.25 | 30.7 ± 0.08 | The full TPC-H q1-q22 and ClickBench sets are all flat (ratio is 1.00-1.01 in both directions). Separately, interleaving tests per-cell (baseline and patched back-to-back, so run-to-run variance -- e.g. thermal -- cancels rather than favoring one) for `physical_join_distinct` + eight ClickBench queries (4 rounds) confirmed the same thing: patched and baseline straddle zero; no systematic regression from the deep compare. ## Testing - `cargo test -p datafusion-physical-plan --lib union` -- all pass, including a new `test_union_schema_fast_path_content_equal` that exercises the `==` branch with pointer-distinct-but-equal schemas and asserts the result matches the shared schema (i.e. identical to the slow-path merge). - `cargo clippy -p datafusion-physical-plan --lib -- -D warnings` -- clean. - `cargo bench --bench union_schema` -- compiles and runs. ## Are there any user-facing changes? No: planning-time performance change only, results and schema are identical. --------- Signed-off-by: Reid Kaufmann <reid.kaufmann@gmail.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
PlanPropertiesredundently #19796This patch aims to implement a fast-path for the ExecutionPlan::with_new_children function for some plans, moving closer to a physical plan re-use implementation and improving planning performance. If the passed children properties are the same as in self, we do not actually recompute self's properties (which could be costly if projection mapping is required). Instead, we just replace the children and re-use self's properties as-is.
To be able to compare two different properties -- ExecutionPlan::properties(...) signature is modified and now returns
&Arc<PlanProperties>. Ifchildrenproperties are the same inwith_new_children-- we clone our properties arc and then a parent plan will consider our properties as unchanged, doing the same.&Arc<PlanProperties>fromExecutionPlan::properties(...)instead of a reference.with_new_childrenfast-path if there is no children properties changes for allmajor plans.
Note: currently,
reset_plan_statesdoes not allow to re-use plan in general: it is notsupported for dynamic filters and recursive queries features, as in this case state reset
should update pointers in the children plans.