perf: reuse the projected equivalence group when only child orderings change - #24445
Open
zhuqi-lucas wants to merge 12 commits into
Open
perf: reuse the projected equivalence group when only child orderings change#24445zhuqi-lucas wants to merge 12 commits into
zhuqi-lucas wants to merge 12 commits into
Conversation
github-actions
Bot
added
physical-expr
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24445 +/- ##
========================================
Coverage 81.31% 81.31%
========================================
Files 1117 1117
Lines 395911 396253 +342
Branches 395911 396253 +342
========================================
+ Hits 321918 322201 +283
- Misses 55177 55186 +9
- Partials 18816 18866 +50 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… change
`replace_children_if_necessary` already short-circuits two cases: identical
child pointers, and identical child `PlanProperties` pointers. Inserting a
sort below a projection satisfies neither — the child is a new object, so its
properties pointer differs — yet the child's equivalence group is unchanged.
Sorting changes which orderings hold, not which expressions are equal.
`Recompute` therefore re-projects a group identical to the one the projection
already holds. `EquivalenceGroup::project` is a pure function of the group and
the mapping, so when both are unchanged the previous result can be handed back
instead.
`ProjectionExec::replace_children` now compares the old and new child
equivalence groups on the `Recompute` path and, when they match, reuses the
cached group and derives only the orderings. The check is local to
`ProjectionExec`: no new `ChildrenPropertiesMode` variant, and no change to the
shared path, so other operators are unaffected.
Supporting changes: `EquivalenceGroup` gains `PartialEq` (comparing `classes`,
since `map` is an index into them), and `EquivalenceProperties::project` splits
so `project_with_eq_group` can take an already-projected group.
Measured on a query over a 1191-line view with 38 SELECTs, 117 CASE
expressions and 7 joins across 11 tables. Ten warm samples per
configuration, same build flags, the only variable being this patch:
before (median) after (median)
EnforceSorting 197.8ms 90.1ms -54%
optimizer rules 338.1ms 214.6ms -37%
planning wall 420.6ms 299.0ms -29%
The two ranges do not overlap (196.9-199.2 against 88.8-91.0). Logical rules
are unchanged, which is the control: the saving lands in the physical phase and
nowhere else. `EnforceDistribution` improves as well, since it rebuilds the
same projections.
The saving scales with projection count times expression size times rule
passes, so plans with narrow projections will see little.
zhuqi-lucas
force-pushed
the
qizhu/reuse-eq-group-on-child-swap
branch
from
August 18, 2026 06:50
d1e682e to
82edbc3
Compare
Contributor
Author
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing qizhu/reuse-eq-group-on-child-swap (82edbc3) to 6eaca8b (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing qizhu/reuse-eq-group-on-child-swap (82edbc3) to 6eaca8b (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
Adds unit tests for the fast path and the invariant it rests on. `EquivalenceGroup`'s new `PartialEq`: equal classes compare equal however they were arrived at (a bridged `a = b`, `b = c` matches a directly stated one), member order within a class is immaterial, and differing or widened classes compare unequal. `EquivalenceProperties::project_with_eq_group`: handing back exactly the group `project` would have computed reproduces it in full -- group, orderings, constraints and schema all match. A second test passes an empty group to confirm the argument is actually consumed rather than ignored. `ProjectionExec::replace_children`: a sort below the projection changes the orderings but not the equivalence group, and the resulting properties match building the projection from scratch. A child that equates a different pair must not inherit the cached group, which is the case that would be unsound; inverting the guard to always reuse makes that test fail. `Keep` mode is covered too, and a separate test pins the premise that sorting leaves the equivalence group untouched, so a change in that behaviour fails there first rather than silently weakening the fast path.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes physical plan property recomputation for ProjectionExec by reusing an already-projected equivalence group when the only upstream change is to child orderings (e.g., inserting a SortExec below a projection). This reduces repeated EquivalenceGroup::project work during physical optimization passes while keeping plan semantics unchanged.
Changes:
- Add
PartialEqforEquivalenceGroup(comparing only equivalence classes). - Split
EquivalenceProperties::projectto allowproject_with_eq_group, reusing a previously projected equivalence group while still re-deriving orderings. - Add a
ProjectionExecfast path inreplace_children(Recompute)to reuse the cached projected equivalence group when the child’s equivalence group is unchanged, plus targeted regression tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| datafusion/physical-plan/src/projection.rs | Adds a replace_children fast path to reuse cached projected equivalence group when only orderings change; adds tests covering the new behavior. |
| datafusion/physical-expr/src/equivalence/properties/mod.rs | Introduces project_with_eq_group and refactors project to delegate, enabling reuse of an already-projected group while recomputing orderings/constraints. |
| datafusion/physical-expr/src/equivalence/class.rs | Implements PartialEq for EquivalenceGroup based on classes (treating map as derived/index-only), with tests validating intended equality behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`EquivalenceGroup`'s new `PartialEq` compared `classes` positionally. `classes` is a `Vec` but its order carries no meaning -- `remove_class_at_idx` uses `swap_remove` -- so two groups describing exactly the same equalities could compare unequal depending on how they were built. That is a trap for any caller treating this as a semantic check, and it also made the projection fast path needlessly conservative. It now compares the classes as the sets they are. `test_project_with_eq_group_derives_orderings_from_the_caller_group` was vacuous: it asserted on `eq_group()`, which stores the argument verbatim, so it would have passed even if the group were ignored everywhere else. Asserting on `oeq_class()` would not have helped either, since that is built from the projected orderings and never consults the group. It now asserts on behaviour, and picks the probe carefully: `projected_orderings` resolves `[a ASC]` to `[c1 ASC]` by itself, so asking whether `c1` is ordered proves nothing. Asking about `a1` does, because reaching that conclusion requires the supplied group to say `a1 = c1`. `assert_same_properties` compared partitioning through derived `Debug`, which changes with any field addition. It now compares the partition count and the explicit `Display` form. Both fixes are covered: reverting the comparison to positional makes `test_equivalence_group_eq_ignores_class_order` fail.
Contributor
Author
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing qizhu/reuse-eq-group-on-child-swap (09d9cd2) to c429919 (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
The guard compares the old and new child equivalence groups, but `EquivalenceGroup::project` is a pure function of *two* arguments. The second, the projection mapping, was only implicitly unchanged. Document why, and cover it. The mapping comes from `projector.projection()`, carried over untouched, and from the child's schema, which `ProjectionMapping::try_new` consults only for field names and indices -- never for types or nullability. A child differing only in nullability therefore keeps the same mapping. A child that renamed or reordered those fields would change the group as well, since its members are `Column`s carrying those names, so the comparison already rejects it; were one to slip through, `try_new`'s name assertion errors out rather than letting a stale group into the plan. The new test swaps in a child differing only in nullability and asserts the fast path agrees with `try_from_projector`, the path it replaces. It compares against that rather than a freshly built projection deliberately: `replace_children` carries the existing `Projector` over, so the output schema stays as it was, while `try_new` derives a new one from the new child. That difference belongs to `replace_children` and predates this change, so the meaningful contract is that its two paths agree. Handing the fast path an empty group makes the test fail.
The test swapped in a child that made fields nullable. That is the one direction `is_allowed_field_change` forbids a physical optimizer rule from taking: it permits a field to become non-nullable, never the reverse. Testing the fast path against an input the framework rules out proves less than it appears to. Swapping the other way exercises the same thing -- a child differing only in nullability keeps the projection mapping identical, since `ProjectionMapping::try_new` reads only field names and indices -- while staying inside what a rule is allowed to do. Worth recording why the cached output schema is not a hazard here. Both `replace_children` paths carry the existing `Projector` over, so the output schema does not track a child whose nullability changed. Since a rule may only tighten, that cached schema can only ever be more conservative than the child, never less, so it cannot claim non-null for data that carries nulls.
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing qizhu/reuse-eq-group-on-child-swap (09d9cd2) to c429919 (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
…anning
The equivalence-group comparison was changed to set semantics in review. That
turned an O(n) positional comparison into an O(n^2) scan on a hot path --
`ProjectionExec` consults it on every child replacement during physical
optimization -- and the benchmark bot caught it: `physical_plan_clickbench_all`
went from 129.7ms to 158.5ms against an unchanged 130.0ms baseline, with most
clickbench queries 1.22x to 1.31x slower. The run before that change showed no
regression at all.
Measured directly on the comparison, 20k iterations of two equal groups:
classes positional set-wise ratio
8 121.9ms 267.5ms 2.2x
16 204.2ms 899.3ms 4.4x
32 404.1ms 3.23s 8.0x
64 807.5ms 12.29s 15.2x
Positional is linear, set-wise quadratic, and the quadratic term costs far more
than the recomputation the fast path exists to avoid.
The review comment behind the change was about API shape rather than
correctness: a public `PartialEq` reads as semantic equality, and a positional
comparison of a `Vec` whose order is incidental is not that. Rather than pay
for set semantics, this drops `PartialEq` and exposes `has_same_classes`, whose
name claims only what it does. The doc records why it is positional and why the
one direction it can err in is harmless: a caller can be told the groups differ
when they match, never the reverse, so it only ever forfeits an optimization.
Contributor
Author
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing qizhu/reuse-eq-group-on-child-swap (95cc536) to c429919 (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing qizhu/reuse-eq-group-on-child-swap (95cc536) to c429919 (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
adriangb
requested changes
Aug 19, 2026
adriangb
left a comment
adriangb
left a comment
Contributor
There was a problem hiding this comment.
A query over a 1191-line view with 38 SELECTs, 117 CASE expressions and 7 joins across 11 tables
Is this query public or reproducible with an MRE? I don't see an obvious perf improvement in #24445 (comment)
Comment on lines
+1175
to
+1183
| /// Same as [`Self::project`], but takes an already-projected equivalence | ||
| /// group instead of computing one. | ||
| /// | ||
| /// [`EquivalenceGroup::project`] is a pure function of the group and the | ||
| /// mapping, so a caller that knows both are unchanged since the last | ||
| /// projection can hand back the previous result rather than recomputing an | ||
| /// identical one. Orderings are still derived here: they are precisely what | ||
| /// changes when a sort is introduced below this node. | ||
| pub fn project_with_eq_group( |
Contributor
There was a problem hiding this comment.
This is a public function that has an unchecked precondition: a caller passing a group that isn't self.eq_group.project(mapping) silently gets wrong equivalence properties. Should we make that an error or something?
Contributor
Author
There was a problem hiding this comment.
Good catch. It cannot be a hard error without performing the projection the caller is skipping, so it is a debug_assert! now. project routes around it through a private unchecked entry point, since it builds the group itself and satisfies the precondition by construction. Added a should_panic test so the assertion cannot quietly stop being wired up.
Contributor
There was a problem hiding this comment.
Could this be:
/// Projects `self`, reusing `cached`'s already-projected equivalence group when
/// `self`'s group is unchanged from `previous`'s. Falls back to a full
/// projection otherwise — there is no precondition to violate.
pub fn project_reusing(
&self,
mapping: &ProjectionMapping,
output_schema: SchemaRef,
previous: &EquivalenceProperties, // what `cached` was projected from
cached: &EquivalenceProperties, // that projection's result
) -> SelfInstead?
The PR description says:
Holding the equivalence group behind an Arc would reduce the comparison to Arc::ptr_eq and make EquivalenceProperties::clone cheaper, which happens on every SortExec construction. It touches every site that mutates the group, so it seemed better kept separate. The structural comparison's cost is already inside the numbers above.
But it's actually not that bad:
- eq_group is a private field; the only public accessor is pub fn eq_group(&self) -> &EquivalenceGroup (properties/mod.rs:280) — signature unchanged under Arc via deref, so zero downstream breakage
- exactly 5 mutation sites, all in that one file (lines 327, 388, 415, 429, 1424), each becoming Arc::make_mut(&mut self.eq_group)
- no crate outside physical-expr names EquivalenceGroup except ffi and because of this PR projection.rs
If we did add the Arc we would be able to write:
pub fn project_reusing(&self, mapping, output_schema, cached: &EquivalenceProperties) -> Self {
let eq_group = match &cached.projected_from {
Some(src) if Arc::ptr_eq(src, &self.eq_group) => Arc::clone(&cached.eq_group),
_ => Arc::new(self.eq_group.project(mapping)),
};
...
}| } | ||
|
|
||
| #[test] | ||
| fn test_replace_children_reuses_eq_group_when_only_orderings_change() -> Result<()> { |
Contributor
There was a problem hiding this comment.
I don't think this are hitting the fast path. I.e. if we deleted the fast path this test would still pass. Could we add a cfg(test) counter or something that asserts that we are actually hitting the fast path?
Contributor
Author
There was a problem hiding this comment.
You were right, they passed with the fast path deleted. There is a cfg(test) probe now and the tests assert the reuse count: one for the two cases that should reuse, zero for the case where the child equivalence group changed. Deleting the fast path fails the first two. The probe is thread local rather than a global counter, since tests run in parallel and a shared count would let one test see another's hits.
Comment on lines
+323
to
+327
| /// The comparison is positional because it runs on a hot path: | ||
| /// `ProjectionExec` consults it every time a rule replaces its child. Set | ||
| /// semantics would mean scanning the other group per class, and the quadratic | ||
| /// blowup costs far more than the recomputation it is trying to avoid -- | ||
| /// measured at 8x for 32 classes and 15x for 64. |
Contributor
There was a problem hiding this comment.
I worry these measured multipliers will bitrot
Contributor
Author
There was a problem hiding this comment.
Agreed, dropped them. The reason the comparison is positional does not depend on the exact numbers, so the doc just states the shape of the cost now and the measurements stay in the commit that introduced it.
Contributor
Author
|
Thank you @adriangb for review, addressed review comments. On the perf question: physical_sorted_union_order_by_50_uint64 1.10 410.3±7.29ms ? ?/sec 1.00 373.9±8.28ms ? ?/sec |
Three things from review. `project_with_eq_group` is public and its precondition -- that `eq_group` is `self.eq_group.project(mapping)` -- was unchecked, so a caller passing anything else silently got equivalence properties that disagree with the projection. It cannot be a hard error, since checking means performing the projection the caller is skipping, so it is now a `debug_assert!`. `project` itself routes around the check through a private unchecked entry point, since it builds the group and satisfies the precondition by construction. A `should_panic` test pins that the assertion is wired up. The `replace_children` tests asserted the resulting properties were correct but not that the fast path produced them, so deleting the fast path left them passing. They now count reuses through a `cfg(test)` probe and assert the count: one for the two cases that should reuse, zero for the case where the child's equivalence group changed. Deleting the fast path now fails both of the first two. The probe is thread local rather than a global counter, since the test binary runs tests in parallel and a shared count would let one test observe another's hits. Dropped the measured multipliers from the `has_same_classes` doc. The reason the comparison is positional does not depend on the exact numbers, and the numbers would go stale; they live in the commit that introduced it instead.
zhuqi-lucas
force-pushed
the
qizhu/reuse-eq-group-on-child-swap
branch
from
August 20, 2026 05:30
22e2104 to
b0f4ea4
Compare
Contributor
Author
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing qizhu/reuse-eq-group-on-child-swap (eea206c) to f1f0449 (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
adriangb
reviewed
Aug 20, 2026
Comment on lines
+238
to
+242
| fn try_from_projector_reusing_eq_group( | ||
| projector: Projector, | ||
| input: Arc<dyn ExecutionPlan>, | ||
| eq_group: EquivalenceGroup, | ||
| ) -> Result<Self> { |
Contributor
There was a problem hiding this comment.
This duplicates compute_properties and can drift if PlanProperties grows a new field.
The duplication exists only because reuse is expressed as a second constructor. But the two paths differ in exactly one expression — how eq_properties is obtained. Everything else (output_partitioning, pipeline_behavior, boundedness, PlanProperties::new, the Self { … } literal) is copied. So push the difference down into compute_properties as a parameter and delete the second constructor:
fn compute_properties(
input: &Arc<dyn ExecutionPlan>,
projection_mapping: &ProjectionMapping,
schema: SchemaRef,
reused_eq_group: Option<EquivalenceGroup>, // <- the only new knob
) -> Result<PlanProperties> {
let input_eq_properties = input.equivalence_properties();
// The only thing reuse changes. Everything below is common, so the two
// paths cannot drift.
let eq_properties = match reused_eq_group {
Some(eq_group) => input_eq_properties
.project_with_eq_group(projection_mapping, schema, eq_group),
None => input_eq_properties.project(projection_mapping, schema),
};
// ... unchanged: partitioning, PlanProperties::new
}
fn try_from_projector(projector, input) -> Result<Self> {
Self::try_from_projector_with_eq_group(projector, input, None)
}
fn try_from_projector_with_eq_group(
projector: Projector,
input: Arc<dyn ExecutionPlan>,
reused_eq_group: Option<EquivalenceGroup>,
) -> Result<Self> { /* the single existing body */ }replace_children(Recompute) then loses its early return and its duplicated call. The guard just produces the Option:
let child = children.swap_remove(0);
let reused_eq_group = if self.input.equivalence_properties().eq_group()
.has_same_classes(child.equivalence_properties().eq_group())
{
#[cfg(test)]
eq_group_reuse_probe::record_hit();
Some(self.cache.equivalence_properties().eq_group().clone())
} else {
None
};
ProjectionExec::try_from_projector_with_eq_group(self.projector.clone(), child, reused_eq_group)
.map(|p| Arc::new(p) as _)We could also move the guard into EquivalenceProperties instead of the group:
pub fn project_reusing(
&self,
mapping: &ProjectionMapping,
output_schema: SchemaRef,
// (the eq properties this was last projected from, the group that produced)
previous: Option<(&EquivalenceGroup, &EquivalenceGroup)>,
) -> Self
Contributor
Author
There was a problem hiding this comment.
Thanks @adriangb , very good suggestion!
Done, and it reads much better. compute_properties takes the optional group, try_from_projector delegates with None, and there is one body instead of two. replace_children loses the early return and the second call site as you sketched.
Left the guard in ProjectionExec rather than moving it into EquivalenceProperties. The soundness argument leans on facts the plan node owns -- that the projector was carried over untouched, so the mapping is unchanged -- and the (previous_group, produced_group) pair reads as a cache key that EquivalenceProperties would have no way to validate. Happy to move it if you would rather it live there.
No behaviour change; deleting the fast path still fails the two tests that assert it is taken.
The second constructor duplicated `compute_properties` -- partitioning, pipeline behaviour, boundedness, the `PlanProperties` and `Self` literals were all copied -- so a new field on `PlanProperties` would have had to be added twice, and the two paths could drift apart silently. They differed in exactly one expression: whether the equivalence group is reprojected or handed back. That is now a parameter on `compute_properties`, `try_from_projector` delegates with `None`, and there is a single body. `replace_children` loses its early return and its second call site: the guard just produces the `Option`. No behaviour change. Deleting the fast path still fails the two tests that assert it is taken.
Contributor
|
run benchmark sql_planner |
1 similar comment
Contributor
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing qizhu/reuse-eq-group-on-child-swap (321dae0) to dbdc627 (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing qizhu/reuse-eq-group-on-child-swap (321dae0) to dbdc627 (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing qizhu/reuse-eq-group-on-child-swap (321dae0) to dbdc627 (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing qizhu/reuse-eq-group-on-child-swap (321dae0) to dbdc627 (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
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 close?
Closes #24478.
Rationale for this change
replace_children_if_necessaryalready short-circuits two cases: identical child pointers, and identical childPlanPropertiespointers. A rule that inserts a sort below a projection satisfies neither, because the child is a new object and so its properties pointer differs. Its equivalence group, however, is unchanged: sorting changes which orderings hold, not which expressions are equal to one another.Recomputetherefore re-projects a group identical to the one the projection already holds.EquivalenceGroup::projectis a pure function of the group and the mapping, so when both are unchanged the previous result can be handed back instead of recomputed.This matters on plans with many wide projections, because the recompute is repeated for every projection above the inserted sort, on every pass of the rule.
What changes are included in this PR?
EquivalenceGroupgainsPartialEq, comparingclasses.mapis an index intoclassesand carries no additional information.EquivalenceProperties::projectsplits, soproject_with_eq_groupcan take an already-projected group. Orderings are still derived, since they are precisely what changes when a sort appears below.ProjectionExec::replace_childrencompares the old and new child equivalence groups on theRecomputepath and, when they match, reuses the cached group.The check is local to
ProjectionExec. No newChildrenPropertiesModevariant and no change to the shared path, so other operators are unaffected.Measurements
A query over a 1191-line view with 38 SELECTs, 117 CASE expressions and 7 joins across 11 tables. Ten warm samples per configuration, identical build flags, the only variable being this patch:
The ranges do not overlap: 196.9–199.2 against 88.8–91.0. Logical rules are unchanged across the two configurations, which acts as the control: the saving lands in the physical phase and nowhere else.
EnforceDistributionimproves as well, since it rebuilds the same projections.The saving scales with projection count times expression size times rule passes, so plans with narrow projections should see little. I expect
sql_plannerto show a small delta for that reason.Are there any user-facing changes?
No. Plans are unchanged.
Testing
datafusion-physical-expr1594,datafusion-physical-plan1716,datafusion-physical-optimizer33,datafusion-optimizer760,datafusion-common550,datafusioncore 442, and all 502 sqllogictest files. No test or expected plan was modified.Worth flagging for review: an earlier revision passed the projected equivalence properties to
Partitioning::projectwherecompute_propertiespasses the input's. Every unit test stayed green; onlyrange_partitioning.sltcaught it, via the case asserting that a join preserving Range partitioning lets the aggregate above it skip a Hash repartition. It produced a worse plan rather than a wrong answer, which is why nothing else noticed.Follow-up
Holding the equivalence group behind an
Arcwould reduce the comparison toArc::ptr_eqand makeEquivalenceProperties::clonecheaper, which happens on everySortExecconstruction. It touches every site that mutates the group, so it seemed better kept separate. The structural comparison's cost is already inside the numbers above.