perf: reuse the projected equivalence group when only child orderings change - #24445
Open
zhuqi-lucas wants to merge 8 commits into
Open
perf: reuse the projected equivalence group when only child orderings change#24445zhuqi-lucas wants to merge 8 commits into
zhuqi-lucas wants to merge 8 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.24% 81.24% -0.01%
==========================================
Files 1113 1113
Lines 392744 393086 +342
Branches 392744 393086 +342
==========================================
+ Hits 319090 319367 +277
- Misses 54900 54915 +15
- Partials 18754 18804 +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 |
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.