[X-3555] perf: reuse the projected equivalence group when only child orderings change - #72
Merged
Merged
Conversation
…orderings change Port of apache#24445, adapted to this branch's `with_new_children` API (upstream has since split that into `replace_children` with an explicit `ChildrenPropertiesMode`; the guard sits in the same spot either way). `check_if_same_properties!` requires the child's properties to be unchanged outright. A rule that inserts a sort below a projection does not qualify, since the child is a new object with new properties. Its equivalence group, however, is unchanged: sorting changes which orderings hold, not which expressions are equal to one another. The projection therefore re-projects a group identical to the one it already holds. `EquivalenceGroup::project` is a pure function of the group and the mapping, so when both are unchanged the previous result can be reused and only the orderings derived. The cost is paid once per projection above the insertion point on every pass of the rule, so it grows with projection count times expression size times passes. Measured on the stocks last-trade snapshot endpoint, ten warm samples per configuration: EnforceSorting 197.8ms -> 90.1ms -54% optimizer rules 338.1ms -> 214.6ms -37% planning wall 420.6ms -> 299.0ms -29% Logical rules are unchanged across the two configurations, which acts as the control: the saving lands in the physical phase and nowhere else.
Ports the upstream tests from apache#24445, adapted to this branch's `with_new_children` API. `EquivalenceGroup`'s new `PartialEq`: equal classes compare equal however they were arrived at, 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. A second test passes an empty group to confirm the argument is consumed rather than ignored. `ProjectionExec::with_new_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; inverting the guard to always reuse makes that test fail. The `check_if_same_properties!` fast path is covered by handing back the same child, since it compares properties by pointer. A separate test pins the premise that sorting leaves the equivalence group untouched, so a change there fails first rather than silently weakening the reuse.
There was a problem hiding this comment.
Pull request overview
This PR ports an upstream performance optimization to branch-54 that avoids recomputing projected equivalence groups for ProjectionExec::with_new_children when only the child’s ordering changes (e.g., a SortExec is inserted below), reducing physical planning overhead while keeping generated plans identical.
Changes:
- Add a
ProjectionExecfast path that reuses the cached projectedEquivalenceGroupwhen the new child’s equivalence group matches the previous child’s group. - Split
EquivalenceProperties::projectto delegate to a newproject_with_eq_group, allowing callers to reuse a previously projected equivalence group while still deriving new orderings. - Add
EquivalenceGroup: PartialEqand accompanying unit tests for the new behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| datafusion/physical-plan/src/projection.rs | Adds a with_new_children fast path to reuse projected equivalence groups and includes unit tests exercising the new behavior. |
| datafusion/physical-expr/src/equivalence/properties/mod.rs | Introduces project_with_eq_group and tests intended to validate equivalence with project and behavior under different groups. |
| datafusion/physical-expr/src/equivalence/class.rs | Implements PartialEq for EquivalenceGroup and adds tests validating equality expectations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`EquivalenceGroup`'s `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.
… test it 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.
… test 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 `with_new_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.
…kbench planning
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.
…ercise it 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.
MassivePizza
approved these changes
Aug 20, 2026
Ports three upstream review rounds from apache#24445 in one go, since the second reworked what the first had just changed. `compute_properties` takes the reuse as a parameter and there is one constructor instead of two. The second duplicated partitioning, pipeline behaviour, boundedness and both literals, so a new field on `PlanProperties` would have had to be added twice. `project_with_eq_group` took a group on trust and could only assert its precondition in debug builds. `project_reusing` takes the properties the cached projection was built from alongside the projection itself, compares the groups and falls back to a full projection when they differ, so there is no longer a precondition to violate. The reuse probe moves to `physical-expr` with the decision: `cfg(test)` does not reach across crates, so the tests asserting the fast path is taken have to live where the branch is. `physical-plan` keeps the tests asserting the resulting properties are right. Disabling the reuse still fails the former. Also drops an intra-doc link to a type no longer in scope, which `ci/scripts/rust_docs.sh` rejects under `-D warnings`.
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.
Port of apache/datafusion#24445
onto
branch-54, so atlas can pin it and measure against the real endpointswithout waiting for the upstream review cycle. Upstream issue:
apache/datafusion#24478.
Part of X-3555.
What this fixes
check_if_same_properties!requires the child'sPlanPropertiesto beunchanged by pointer. A rule that inserts a sort below a projection never
qualifies: the child is a new object with new properties. So the projection
falls into the full recompute path.
Its equivalence group, though, is unchanged.
SortExec::compute_propertiesclones the input's equivalence properties and only calls
reorder, which clearsand re-adds orderings and never touches the group. The projection therefore
re-derives a group bit-identical to the one it already holds.
EquivalenceGroup::projectis a pure function of the group and the mapping, sowhen both are unchanged the previous result can be reused and only the orderings
derived. The cost is paid once per projection above the insertion point, on
every pass of the rule, so it grows with projection count times expression size
times passes.
EnforceSortingruns six times on the snapshot plan.Difference from upstream
Upstream has since split
with_new_childrenintoreplace_childrenwith anexplicit
ChildrenPropertiesMode.branch-54predates that, so the guard sitsin
with_new_childrenimmediately aftercheck_if_same_properties!rather thanin the
Recomputearm. Same position in the control flow, same condition.The two
physical-exprfiles are byte-identical between the two bases, soEquivalenceGroup: PartialEqand theproject/project_with_eq_groupsplitare unchanged from the upstream patch.
Measurements
/stocks/last-trade/vX/snapshot?limit=1, local, warm, ten samples perconfiguration, identical build flags, the only variable being this patch:
EnforceSortingRanges do not overlap: 196.9–199.2 against 88.8–91.0. Logical rules are
unchanged across the two configurations, which is the control: the saving lands
in the physical phase and nowhere else.
EnforceDistributionimproves too, sinceit rebuilds the same projections.
Plans are unchanged; this is purely a planning-time saving.
Testing
datafusion-physical-planlib: 1440 passdatafusion-physical-exprlib: 1535 passcargo fmt --all -- --checkclean,cargo clippycleanEight new unit tests covering
EquivalenceGroup'sPartialEq,project_with_eq_groupagainstproject, and all threewith_new_childrenpaths. Inverting the guard to always reuse makes
test_with_new_children_recomputes_when_eq_group_changesfail, so the unsounddirection is genuinely covered rather than merely asserted.
Pre-existing failure, not from this change
sqllogictestreports one failure onencrypted_parquet.slt:88, "query isexpected to fail, but actually succeed". Reverting all three files to a pristine
74b5d951areproduces it, so it predates this branch. Clearing the scratchdirectory does not help, so it is not a stale-artifact issue either.
Affects
Nothing in production until atlas bumps its pinned DataFusion rev. Once pinned,
every query's physical planning takes the cheaper path; plan output is
unchanged, so the blast radius is planning latency only.