perf: Avoid cloning EquivalenceProperties in ordering satisfaction checks - #24800
Open
jayzhan211 wants to merge 2 commits into
Open
perf: Avoid cloning EquivalenceProperties in ordering satisfaction checks#24800jayzhan211 wants to merge 2 commits into
jayzhan211 wants to merge 2 commits into
Conversation
…ed keys in requirements
…EquivalenceProperties
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24800 +/- ##
=======================================
Coverage 81.53% 81.53%
=======================================
Files 1123 1123
Lines 406041 406115 +74
Branches 406041 406115 +74
=======================================
+ Hits 331049 331117 +68
- Misses 55631 55632 +1
- Partials 19361 19366 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 PR close?
Rationale for this change
Physical planning asks "is this ordering already satisfied?" constantly — sort
removal,
EnforceSorting,EnforceDistribution, and the requirement checks forwindows, joins and aggregates all call into
EquivalenceProperties::ordering_satisfy,ordering_satisfy_requirementandextract_common_sort_prefix.Each of those calls deep-clones the entire
EquivalenceProperties— everyequivalence 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:
for, so the whole clone is wasted. This is the most common shape of these calls.
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, toordering_satisfy_requirementandcommon_sort_prefix_length(the latter backsordering_satisfy,extract_common_sort_prefixandreorder):selfand clonesonly when it actually needs to register a constant. Single-key checks never
clone at all.
Plus a new criterion benchmark,
equivalence_properties, covering these entrypoints.
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:
ordering_satisfy— 1 keyordering_satisfy— 1 key, unsatisfiedordering_satisfy_requirement— 1 keyordering_satisfy_requirement— 4 keysordering_satisfy— 4 keysextract_common_sort_prefix— 4 keysHow it scales (
ordering_satisfy, 1 key):Reading the tables: for an N-key check the work goes from
1 clone + N registrationsto(N > 1 ? 1 : 0) clones + (N − 1) registrations.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%.
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_keysat 32classes.
Reproducing
The benchmark is included in this PR, so reverting just the one source file gives
you the baseline:
The second run prints criterion's own
change: [...] (p = ...)line perbenchmark.
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
equivalenceunit tests in
datafusion/physical-exprand, for plan-shape regressions, bysqllogictest — these functions decide whether a
SortExeccan be removed, so abehavior change would surface as a diff in an
EXPLAINplan.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.shis clean.
Are there any user-facing changes?
No. No public API or behavior changes — planning is just faster.