Skip to content

perf: Avoid cloning EquivalenceProperties in ordering satisfaction checks - #24800

Queued
jayzhan211 wants to merge 4 commits into
apache:mainfrom
jayzhan211:lazy-clone-ordering-satisfy
Queued

perf: Avoid cloning EquivalenceProperties in ordering satisfaction checks#24800
jayzhan211 wants to merge 4 commits into
apache:mainfrom
jayzhan211:lazy-clone-ordering-satisfy

Conversation

@jayzhan211

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • Closes #.

Rationale for this change

Physical planning asks "is this ordering already satisfied?" constantly — sort
removal, EnforceSorting, EnforceDistribution, and the requirement checks for
windows, joins and aggregates all call into
EquivalenceProperties::ordering_satisfy, ordering_satisfy_requirement and
extract_common_sort_prefix.

Each of those calls deep-clones the entire EquivalenceProperties — every
equivalence class, every equivalent ordering, and the normalized ordering cache —
before doing anything else, even when it never modifies the copy.

The clone exists for a real reason: as the check walks a multi-key ordering left
to right, it registers each satisfied key as a constant so the next key is
evaluated within that key's tie group. That mutates state, so it needs its own
copy. But two cases pay for it and get nothing back:

  1. A single-key check never mutates anything. There is no "next key" to set up
    for, so the whole clone is wasted. This is the most common shape of these calls.
  2. The last key of any check registers constants nobody reads. After the
    final key is verified, the code still calls add_satisfied_key_constants,
    which rebuilds the ordering cache and re-runs ordering discovery — and then the
    object is dropped.

What changes are included in this PR?

Two changes in EquivalenceProperties, to ordering_satisfy_requirement and
common_sort_prefix_length (the latter backs ordering_satisfy,
extract_common_sort_prefix and reorder):

  • Clone on first write instead of up front. The loop borrows self and clones
    only when it actually needs to register a constant. Single-key checks never
    clone at all.
  • Skip the registration after the last key. Nothing reads it.

Plus a new criterion benchmark, equivalence_properties, covering these entry
points.

This only changes when the copy is made — the results of these functions are
unchanged.

Metrics

Apple M4 Pro, rustc 1.97.0, criterion. All changes significant at p = 0.00.

Properties under test: 3 equivalent orderings ([c0,c1,c2,c3], [c4,c5], [c6])
and a varying number of equivalence classes.

At 8 equivalence classes:

benchmark before after change
ordering_satisfy — 1 key 2.72 µs 0.41 µs −84.9%
ordering_satisfy — 1 key, unsatisfied 1.47 µs 0.41 µs −72.5%
ordering_satisfy_requirement — 1 key 2.70 µs 0.36 µs −86.3%
ordering_satisfy_requirement — 4 keys 7.15 µs 6.07 µs −13.8%
ordering_satisfy — 4 keys 6.99 µs 6.20 µs −11.6%
extract_common_sort_prefix — 4 keys 7.22 µs 6.38 µs −9.2%

How it scales (ordering_satisfy, 1 key):

equivalence classes before after change
2 2.44 µs 0.43 µs −82.5%
8 2.72 µs 0.41 µs −84.9%
32 4.63 µs 0.41 µs −90.8%

Reading the tables: for an N-key check the work goes from
1 clone + N registrations to (N > 1 ? 1 : 0) clones + (N − 1) registrations.

  • 1-key checks drop both the clone and the registration. Note the "after"
    column is flat at ~0.41 µs regardless of how many equivalence classes exist —
    with the clone gone, the check no longer scales with the size of the
    equivalence group at all. The "before" column does, which is why the win grows
    from −82% to −91%.
  • Multi-key checks still clone once and save one of N registrations. Since a
    registration rebuilds the ordering cache and re-runs ordering discovery, that
    single saved call is worth 9–14% here, rising to −37.8% for 4_keys at 32
    classes.

Reproducing

The benchmark is included in this PR, so reverting just the one source file gives
you the baseline:

# baseline: this PR's parent version of the file, with the new benchmark kept
git checkout HEAD^ -- datafusion/physical-expr/src/equivalence/properties/mod.rs
cargo bench -p datafusion-physical-expr --bench equivalence_properties -- --save-baseline before

# with the change
git checkout HEAD -- datafusion/physical-expr/src/equivalence/properties/mod.rs
cargo bench -p datafusion-physical-expr --bench equivalence_properties -- --baseline before

The second run prints criterion's own change: [...] (p = ...) line per
benchmark.

Are these changes tested?

No new correctness tests: this does not change what any of these functions
return, so existing coverage is the right check. Covered by the equivalence
unit tests in datafusion/physical-expr and, for plan-shape regressions, by
sqllogictest — these functions decide whether a SortExec can be removed, so a
behavior change would surface as a diff in an EXPLAIN plan.

Full workspace suite
(--features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption):
10,981 passed, 0 failed, and all 505 sqllogictest files pass. ./dev/rust_lint.sh
is clean.

Are there any user-facing changes?

No. No public API or behavior changes — planning is just faster.

@jayzhan211
jayzhan211 requested a review from adriangb August 30, 2026 14:17
@github-actions github-actions Bot added the physical-expr Changes to the physical-expr crates label Aug 30, 2026
@jayzhan211
jayzhan211 requested a review from rluvaton August 30, 2026 14:17
@codecov-commenter

codecov-commenter commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.62%. Comparing base (3b63300) to head (d57cf89).

Files with missing lines Patch % Lines
...on/physical-expr/src/equivalence/properties/mod.rs 88.88% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24800      +/-   ##
==========================================
- Coverage   81.63%   81.62%   -0.01%     
==========================================
  Files        1123     1123              
  Lines      409673   409686      +13     
  Branches   409673   409686      +13     
==========================================
+ Hits       334417   334422       +5     
- Misses      55625    55631       +6     
- Partials    19631    19633       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jayzhan211,

Thanks for working on this. The lazy clone approach looks good to me, and I like that it also avoids updating the temporary state after the final key. I don't see any blocking issues.

I left one small suggestion about the Criterion benchmarks and what work we want them to measure.

let props = properties(n_classes);

// A single sort key: the most common shape by far.
group.bench_with_input(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to have benchmark coverage for this. One thought: if the goal is to isolate the cost of the satisfaction checks themselves, could we pre-build the ordering/requirement inputs and use iter_batched so the per-iteration setup stays outside the timed routine? Right now the closures also create Columns and PhysicalSortExprs, and the prefix benchmark builds the LexOrdering, so those costs are included in the measurements. If the intention is to measure the end-to-end caller cost instead, keeping the setup here makes sense, but it would be helpful to document that scope. We would still want any cloning that is part of the operation under test to remain inside the timed iteration.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — the setup was measurable, not incidental. asc(i) does a format! plus an Arc allocation per key per iteration, which was a real fraction of a ~400 ns measurement.

Switched to iter_batched: the sort exprs, requirements and the LexOrdering are now built once up front, and since each check takes its input by value, the untimed setup step hands each iteration a fresh clone. Only the call is timed. Also swapped bench_with_input for bench_function, as the parameter was only being used as a label.

Cloning done inside the check stays inside the timed iteration — that is the thing this PR is about, so it has to be measured.

It moved the numbers in the direction you'd expect. At 8 equivalence classes, 1 key:

before after change
ordering_satisfy 2.73 µs 0.36 µs −86.6% (was −84.9%)
ordering_satisfy_requirement 2.90 µs 0.30 µs −89.2% (was −86.3%)

The old numbers were understating the improvement, since the constant setup cost sat in both columns.

I also documented the scope in the module header rather than leaving it implicit:

//! # Scope
//!
//! These measure the satisfaction check itself, not the cost of assembling its
//! arguments. The sort expressions, requirements and orderings are built once,
//! up front. Because the checks take their input by value, each iteration gets a
//! fresh copy from the untimed setup step of `iter_batched`; only the call is
//! timed. Any copying the check does internally is part of what is measured.

Metrics in the PR description updated to match.

@jayzhan211
jayzhan211 added this pull request to the merge queue Sep 2, 2026
@jayzhan211

Copy link
Copy Markdown
Contributor Author

Thanks @kosiew !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants