feat(physical-optimizer): skip a named rule handed a plan it has been seen to leave alone - #25356
Draft
zhuqi-lucas wants to merge 14 commits into
Draft
zhuqi-lucas wants to merge 14 commits into
zhuqi-lucas wants to merge 14 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.97% 82.29% +0.31%
==========================================
Files 1137 1137
Lines 429889 430727 +838
Branches 429889 430727 +838
==========================================
+ Hits 352408 354447 +2039
+ Misses 56432 54803 -1629
- Partials 21049 21477 +428 ☔ 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
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Rendered-plan identity can skip semantically different plans, and the idempotence test does not inspect the duplicated rule’s intermediate output.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an opt-in physical optimizer rule-skipping cache keyed by previously observed unchanged plans.
Changes:
- Adds
skip_unchanged_physical_rulesconfiguration and documentation. - Implements per-planning-run memoization, debug validation, and observer preservation.
- Adds extensive physical planner tests and information-schema coverage.
File summaries
| File | Description |
|---|---|
datafusion/core/src/physical_planner.rs |
Implements caching and tests. |
datafusion/common/src/config.rs |
Defines the new optimizer option. |
docs/source/user-guide/configs.md |
Documents configuration behavior. |
datafusion/sqllogictest/test_files/information_schema.slt |
Updates configuration output expectations. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+3008
to
+3011
| let before = displayable(new_plan.as_ref()).indent(true).to_string(); | ||
| if seen | ||
| .get(optimizer.name()) | ||
| .is_some_and(|plans| plans.contains(&before)) |
Comment on lines
+4195
to
+4201
| for (position, rule) in stock.iter().enumerate() { | ||
| let mut doubled = stock.clone(); | ||
| doubled.insert(position + 1, Arc::clone(rule)); | ||
| assert_eq!( | ||
| corpus_plans("", doubled).await?, | ||
| baseline, | ||
| "running '{}' (position {position}) twice changed the plan, so it \ |
Two problems raised in review, both real. The memo keyed plans on displayable(...).indent(true). That renderer has show_schema off by default and prints only what each node chooses to, so a rule could return a plan with a different schema or different execution properties and still produce the same string. In release the later pass would be skipped and that rewrite silently lost, and the debug self-check repeated the same incomplete comparison so it could not catch it. plan_fingerprint now asks for the schema and appends each node's PlanProperties, partitioning, ordering, emission type and boundedness, in the same pre-order. Two tests pin what that buys: plans differing only in nullability, and plans differing only in partitioning, each render identically without it and compare unequal with it. This is still short of a structural identity and the PR says so: anything a node neither prints nor exposes through PlanProperties stays invisible, and ExecutionPlan has no structural comparison to use instead. The idempotence test compared the plan after the whole chain had run. A later rule can normalize away a rewrite the duplicated rule made on its second invocation, so a rule that is not idempotent could pass. It now captures the plan immediately after each rule through the observer and compares the two snapshots belonging to the duplicated rule. A further test proves that distinction is what makes the check work: a rule that rewrites on its second call, paired with a rule that strips that rewrite, leaves the final plan identical to the first pass, so comparing ends of chains sees nothing while comparing per rule does.
zhuqi-lucas
marked this pull request as draft
September 17, 2026 08:44
Ordering is the property this feature cares about most, since EnsureRequirements, the rule that motivates it, exists to rewrite ordering and distribution, yet nothing held the fingerprint to distinguishing it. Adds a constructor on the existing NoOpExecutionPlan that claims an output ordering. Its rendering is unchanged, which is the point: the test first asserts the two plans render identically with the schema shown, then that their orderings genuinely differ, then that the fingerprints do not match. Removing the properties from the fingerprint fails this test and the partitioning one, and leaves the nullability one passing, since that depends on show_schema rather than on the properties.
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?
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. Those passes are not wasted in principle, they are wasted whenever the rewrite in front of them did not actually fire, which for most rewrites is most queries.
Measured on a chain with six enforcement passes over a 34-node plan, four of the six left the plan byte-identical and still paid the full tree walk, about 30ms each in a debug build. This recovers two of those four, not all four: after anything changes the plan, the first pass that finds nothing to do still has to run in order to establish that the plan is a fixpoint.
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. That is not something the rule could report its way out of:
replace_children_if_necessaryalready hands back the original plan when the child pointers are unchanged, so the passes that truly did nothing do keep their input. The other 16 lost identity because the phases inside the rule rewrote the tree and then rewrote it back, which is #25360.HashSet<String>compares on collision, so two plans that hash alike are not confused.Bound on the guarantee: comparing rendered plans
ExecutionPlanhas no structural equality, so this comparesdisplayable(...).indent(true)output. That is not a perfect plan identity. Two plans differing only in something the display does not print would compare equal, and one of them would be skipped wrongly. The debug self-check uses the same comparison, so it cannot catch that class either.I am not aware of a better option today, and this is the same notion of plan identity the test suite already relies on throughout, but it is a bound rather than a proof and reviewers should weigh it as one. If
ExecutionPlanever grows a structural comparison this should move to it.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.