feat(physical-optimizer): skip a named rule handed a plan it has been seen to leave alone - #25356
Draft
zhuqi-lucas wants to merge 11 commits into
Draft
zhuqi-lucas wants to merge 11 commits into
zhuqi-lucas wants to merge 11 commits into
Conversation
…e-run The logical optimizer iterates and stops on convergence via LogicalPlanSignature. The physical optimizer runs its list once, which suits the default chain -- everything able to invalidate distribution or ordering requirements is deliberately ordered before the single EnsureRequirements. Custom rule lists do not have that luxury: a rewrite inserted after that point invalidates requirements again and needs its own enforcement pass, and some of those passes run on a plan no preceding rule touched. Rules already return their input Arc untouched when they have nothing to do, so pointer identity is an exact, allocation-free 'nothing happened' signal. A rule can now declare skip_if_unchanged(); when the config flag datafusion.optimizer.skip_unchanged_physical_rules is on, the optimizer remembers the plan each opted-in rule returned and skips the call when handed back that same object. The memo lives in the optimization run, keyed by rule name, so nothing leaks across queries (rule instances are shared) and a rule listed twice as two instances still matches. Debug builds run a skipped rule anyway and assert it changed nothing, so a rule that declares purity without having it fails a test rather than a query -- Spark is the only surveyed engine that checks this, and the engines that rely on counters instead have public incidents from non-idempotent rules. Both flags default to off, so nothing changes until a rule and the session agree.
github-actions
Bot
added
documentation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
The three existing tests exercise the mechanism; this one exercises the shape it exists for -- enforcement passes separated by rewrites, where only the passes following a rewrite that actually fired have work to do.
The rule derives everything from the plan and the config, and the config is fixed for an optimization run, so running it again on a plan it just produced cannot change anything. Without this the new trait method has no implementor in tree and rule lists that enforce requirements after their own rewrites -- the case the feature exists for -- get nothing.
The map was allocated on every physical planning run even with the feature off. Making it an Option keeps the disabled path free of it.
The optimizer consults the rule it holds, so a rule that runs other rules inside its own optimize() decides for all of them. A wrapper leaving this at the default silently opts its inner rules out of the skip, which is the same trap apache#25316 describes for schema_check(). Say so where an implementor reads it, with the all() form, since a wrapper is skippable only if every rule it would have run is.
The option was added to OptimizerOptions without the matching rows in information_schema.slt, which SHOW ALL asserts exhaustively.
The debug self-check asserted that re-running a skipped rule returned the same Arc. EnsureRequirements fails that: it rebuilds the tree and hands back a fresh object describing an identical plan, so enabling the optimization over the real rule panicked in debug builds. Identity was the wrong thing to assert. What the skip relies on is that a second pass would arrive at the same plan, which is idempotence; the check now compares the rendered plans. That a rule rebuilds the tree also sharpens the motivation, since the pass being skipped reconstructs the whole plan to end up where it started. Docs in the trait, the config option and EnsureRequirements said 'pure function' and 'returns its input untouched', both of which read as identity, and are corrected to say idempotent. Also collapses the nested if the skip introduced, which clippy rejects. Covers the mechanism with three further tests: a wrapper rule that forwards the opt-in is skipped while one that drops it is not; a query planned through the built-in rules with two extra EnsureRequirements passes appended produces an identical plan with the optimization on and off; and EXPLAIN VERBOSE renders the same output either way, since a skipped rule still reports to the observer.
Replaces the `PhysicalOptimizerRule::skip_if_unchanged` opt-in with a list of rule names in the config option, which now holds names rather than a bool. The opt-in was a trait method that exactly one built-in rule set. An audit of all 21 built-in rules, added here as a test, finds every one of them idempotent, so singling out EnsureRequirements was arbitrary and opting in all 21 would be 21 unverified claims plus a decision to make for each new rule. Whether a chain repeats a rule is a property of the chain, not of the rule, so the chain's owner is who can say it. Naming rules also removes a trap the trait version carried. The optimizer only asks the outermost rule, so a rule wrapped for timing or tracing had to forward the answer, and one that left the default in place silently turned the optimization off. Names need no such cooperation: a wrapper already reports the name it wraps, because that is what EXPLAIN VERBOSE shows. It costs a semver-visible trait method and gains reach over rules the caller does not own. The debug self-check still verifies every skip. Tests: skips a repeated pass; inert when the name is absent, unknown or misspelt; reads a list with the spacing people write; handles an interleaved enforce/rewrite chain; does not leak between plans; follows the name a wrapper reports and not the wrapper's own; leaves a corpus of nine plans byte-identical through the built-in chain plus two trailing EnsureRequirements passes; keeps EXPLAIN VERBOSE output unchanged; and holds every built-in rule to the idempotence the config asserts.
Every rule answering to a configured name shares one memo entry, so two rules that behave differently must not share a name. The built-in chain breaks that: OutputRequirements reports one name for the instance that adds requirements and the instance that removes them again, and naming it lets the first one's output suppress the second. The debug self-check catches it, but the config had claimed the built-in list holds no rule twice, which is wrong for OutputRequirements and ProjectionPushdown. Documents the constraint and pins the two repeated names in a test, so this is revisited if either rule is renamed. Giving OutputRequirements a distinct name per mode would make it nameable, and would disambiguate it in EXPLAIN VERBOSE too, but that is a separate change.
zhuqi-lucas
changed the title
github-actions
Bot
added
sqllogictest
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25356 +/- ##
========================================
Coverage 81.94% 81.94%
========================================
Files 1135 1135
Lines 428167 428605 +438
Branches 428167 428605 +438
========================================
+ Hits 350845 351210 +365
- Misses 56375 56403 +28
- Partials 20947 20992 +45 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The skip keyed on the plan a rule last returned, on the assumption that a rule handed back its own output has nothing left to do. That assumption is false: a rule is not required to reach its fixpoint in one pass, and EnsureRequirements routinely does not. Enabling it against a real chain tripped the debug self-check, with the second pass moving a RepartitionExec below a SortExec and switching the sort to per-partition, so the skipped pass would have silently cost that rewrite. Key on the plan the rule was *given* instead, and record it only after the rule has run and returned that same plan. A skip then replays an outcome already observed rather than predicting one, and a rule still converging records nothing and keeps running. Plans are compared by rendered form rather than by pointer. A rule that changes nothing still commonly rebuilds the tree, so pointer identity cannot see a fixpoint: on a real 34-node plan, of 23 passes that left the plan byte-identical only 7 also returned the input object. Collisions are handled by HashSet<String> comparing on hit rather than trusting a hash. The config stays out of the key because what is recorded is scoped to one planning run, where it cannot change. Measured through a downstream chain that enforces requirements six times: physical optimization 208.9ms -> 147.8ms, planning wall 350.7ms -> 295.8ms, two passes skipped, EXPLAIN VERBOSE byte-identical. Adds a test for the case that makes the old key wrong: a rule needing several passes to converge must keep running, and the plan must come out as it does with the optimization off.
zhuqi-lucas
changed the title
This was referenced Sep 16, 2026
Open
The generated configs.md table pads every cell to the widest one, so a description longer than the current maximum reflows all 150 rows and buries the one row that was actually added. Trims it back under that width; the reasoning it carried is in the optimizer loop's comments and in the issue.
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
2 participants
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?
Closes #25355.
Rationale for this change
The logical optimizer iterates and stops on convergence (
max_passes+LogicalPlanSignature). The physical optimizer runs its list once, which suits the default chain: every rule able to invalidate distribution or ordering requirements is deliberately ordered before the singleEnsureRequirements, as the per-rule comments inphysical-optimizer/src/optimizer.rsstate.Custom rule lists do not get that for free. A rewrite inserted after that point (a scan rewrite, a distributed-execution boundary, a view substitution) invalidates requirements again and needs its own enforcement pass behind it. Measured on a chain that enforces requirements six times over a 34-node plan, 23 of 31
EnsureRequirementscalls left the plan byte-identical and still paid the full tree walk.What changes are included in this PR?
datafusion.optimizer.skip_unchanged_physical_rules: comma separated rule names, empty by default.optimize_physical_planremembers the plans that rule has been observed to return unchanged. A later pass handed one of them gets it back instead of re-deriving it.observer, soEXPLAIN VERBOSEoutput is identical either way.The two design points that are easy to get wrong
I had both of these wrong in the first draft of this PR; each was caught by enabling the feature against a real rule chain.
Key on the plan the rule was given, not the plan it last returned. The tempting version skips when a rule is handed back its own output. That assumes a rule reaches its fixpoint in one pass.
EnsureRequirementsdoes not: two consecutive applications both changed the plan, the second moving aRepartitionExecbelow aSortExecand switching that sort to per-partition. The earlier design skipped exactly that pass, which would have silently cost the rewrite.Compare plans by content, not by pointer. A rule that changes nothing still commonly rebuilds the tree. Of the 23 no-op calls above, only 7 also returned the input object, so pointer identity would have found less than a third of them.
HashSet<String>compares on collision, so two plans that hash alike are not confused.Why names in the config rather than a method on the rule
The first draft added
PhysicalOptimizerRule::skip_if_unchanged()withEnsureRequirementsopting in. That puts the decision on the wrong object. Whether a chain repeats a rule is a property of the chain, and upstream cannot know what a downstream chain and node types will do to a rule's behaviour — the non-convergence above does not reproduce on the built-in chain with built-in sources, but is reliable in the chain that motivated this.It also carried a trap: the optimizer asks only the outermost rule, so a rule wrapped for timing or tracing had to forward the answer, and a wrapper leaving the default in place disabled the feature silently. Same shape as #25316, where a wrapper drops
schema_check(). Names need no cooperation from a wrapper, which already reports the name it wraps because that is whatEXPLAIN VERBOSEshows.No trait or other public API change results.
One sharp edge, documented and pinned
A configured name stands for a behaviour, because every rule answering to it shares one record.
OutputRequirementsbreaks that in the built-in chain: the instance that adds requirements and the instance that removes them again report the same name. The config documents it, andbuiltin_chain_repeats_two_rule_namespins both repeated names so a rename is noticed.Measurements
Through a downstream chain with six enforcement passes, on a 34-node plan. Release build, 20 warm samples per arm after three discarded:
Physical optimization -29.3%, planning wall -15.0%. The two wall-clock distributions do not overlap between p10 and p90.
EXPLAIN VERBOSEis byte-identical between the arms.Correctness was checked separately in a debug build with the self-check active over the same chain and endpoint: no violations, and the same identical plans. The self-check is compiled out in release, so the release run says nothing about correctness on its own.
Worth knowing for review: because the self-check re-runs a skipped rule, the saving appears in release and not in debug. The tests encode that explicitly rather than hiding it.
Are these changes tested?
Eleven tests in
physical_planner, including:EnsureRequirementspasses come out byte-identical with the optimization on and off;EXPLAIN VERBOSEstill lists every rule;Existing suites pass in debug and release.
Are there any user-facing changes?
One new config option, empty by default, so existing sessions behave exactly as before.