Repository navigation
Check the final physical plan in the planner and name the rule that broke it - #26461
Merged
Merged
Conversation
github-actions
Bot
added
documentation
jayzhan211
marked this pull request as ready for review
October 18, 2026 03:14
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26461 +/- ##
==========================================
- Coverage 82.91% 82.91% -0.01%
==========================================
Files 1155 1155
Lines 458091 458146 +55
Branches 458091 458146 +55
==========================================
+ Hits 379821 379859 +38
- Misses 55231 55244 +13
- Partials 23039 23043 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
Thank you, I have a quick look and I think its overall beneficial. Will have a in-depth review after this PR getting some other +1s. Specifically, I think the benefits is prevent internal tests or simple extensions from building invalid optimizer pipeline; but for more advanced extension (that overrides sanity check), we might need additional API to set a custom sanity checker. |
alamb
reviewed
Oct 18, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
I quickly looked at this and it seems like a good idea. I will try and find more time to review later on.
My only concern is plan speed -- I will run some planning benchmarks to check
| | 19 | `EnsureCooperative` | - | Wraps non-cooperative plan parts so long-running tasks yield fairly. | | ||
| | 20 | `FilterPushdown(Post)` | post-optimization phase | Pushes dynamic filters at the end of optimization, after plan references stop moving. | | ||
| | 21 | `SanityCheckPlan` | - | Validates that the final physical plan meets ordering, distribution, and infinite-input safety requirements. | | ||
| After the last rule, including any rule added by the user, the physical planner |
Contributor
There was a problem hiding this comment.
I feel like this document is outdated and redundand and hard to find -- maybe as a follow on PR we can fold it into the documentation in #26351
Contributor
Author
There was a problem hiding this comment.
Agreed. #26351 has merged, so a follow-up PR can move this list next to the contract docs.
This PR is now rebased on it. I also removed SanityCheckPlan from the contract's list of rules that must keep a fixed order, and added a sentence saying the planner checks the plan after the last rule.
Contributor
Author
There was a problem hiding this comment.
TIL about this document 🤯
+1 😆
| // given query plan; i.e. it only acts as a final | ||
| // gatekeeping rule. | ||
| Arc::new(SanityCheckPlan::new()), | ||
| // There is no `SanityCheckPlan` rule at the end of this list. The |
Contributor
There was a problem hiding this comment.
how about we deprecate SanityCheckplan rule so it is clearer we intend to remove it
Contributor
Author
There was a problem hiding this comment.
Done. SanityCheckPlan is now deprecated since 56.0.0, and the note points to check_plan_tree_sanity for code that runs optimizer rules without the planner. Tests that only used the rule to validate a plan now call check_plan_tree_sanity directly, and the upgrade guide says the rule is deprecated.
I deprecated it rather than removing it because several downstream projects construct it in their own rule lists, for example greptimedb, cube, sail and openobserve. They should see a deprecation warning before they hit a compile error.
Contributor
|
run benchmark sql_planner |
1 similar comment
Contributor
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing planner-final-plan-check (2689cbb) to 290ad15 (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing planner-final-plan-check (2689cbb) to 290ad15 (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing planner-final-plan-check (2689cbb) to 290ad15 (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing planner-final-plan-check (2689cbb) to 290ad15 (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
jayzhan211
force-pushed
the
planner-final-plan-check
branch
from
October 19, 2026 13:53
2689cbb to
184d94f
Compare
Contributor
Author
|
@alamb Thanks for running these. The second run is the quieter one. In it, The change should not add planning work. Before, the planner walked the final plan twice: once in |
Contributor
Author
|
@2010YOUY01 Thanks. To see who would need that API, I looked at how downstream projects use the rule today:
The last group is the one your comment is about. If the check rejects a correct plan, for example one whose ordering holds but cannot be proven from the plan's properties, those projects can no longer skip the check. I'd suggest adding a way to skip or replace the check in a follow-up, once someone needs it, rather than guessing at the shape of the API now. I'm happy to add it here instead if you'd prefer it to ship in the same release. |
|
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 |
jayzhan211
force-pushed
the
planner-final-plan-check
branch
from
October 21, 2026 12:20
184d94f to
ff53db5
Compare
Contributor
|
After all the discussion on #26481 (comment) is this one we should review / merge @jayzhan211 in your opinion? |
Contributor
Author
We can review/merge this, they could be parallel |
zhuqi-lucas
reviewed
Oct 27, 2026
| let ensure_requirements = EnsureRequirements::new(); | ||
| let enforced = plan_after_rule | ||
| .iter() | ||
| .rposition(|(rule, _)| rule.name() == ensure_requirements.name()); |
Contributor
There was a problem hiding this comment.
Once #26757 lands, this search could start at the enforcement phase boundary instead of matching EnsureRequirements by name.
Contributor
Author
There was a problem hiding this comment.
#26757 is still a draft, so I'll make that switch there: once it is rebased on this PR, the search will start at the analyzer/optimizer boundary and the name match goes away.
| // This runs once after all optimizer runs are complete, including | ||
| // rules appended by the user, to verify that the plan is executable. | ||
| let optimizer_options = &session_state.config_options().optimizer; | ||
| if let Err(e) = check_plan_tree_sanity(&new_plan, optimizer_options) { |
Contributor
There was a problem hiding this comment.
Since this check can no longer be skipped, an opt-out flag could be a follow-up if a downstream needs it.
Contributor
There was a problem hiding this comment.
I actually think we shouldn't allow plans to opt out of plan sanity checking -- if the generated plan doesn't pass sanity check then it will fail in some other way (either wrong results or worse). Catching it early is better I think
…rom main Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
alamb
reviewed
Oct 29, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
@zhuqi-lucas Thanks @jayzhan211 @zh -- I went through this carefully
The only thing I think we need to change before merging is to avoid holding all intermediate plans (i will run benchmarks to double check)
I left a bunch of other suggestions, but I don't think any of them are required to merge
| let optimizer_context = SessionOptimizerContext::new(session_state); | ||
| // The plan after each rule, so that a failed final check can name the | ||
| // rule that broke the plan. Plans share unchanged subtrees, so this | ||
| // costs one pointer per rule. |
Contributor
There was a problem hiding this comment.
it also pins the plan memory in place for longer, so it would make peak memory usage higher which maybe doesn't matter, but it is a differene
Contributor
There was a problem hiding this comment.
Also there are a bunch of rules that try to unwrap the Arc to avoid allocations which this will prevent.
If you only want to get the plans on failure, I recommend saving only the input plan and if there is a failure then go back and run each of the passes (again) to get a more specific message
| .with_memory_limit(1_000) | ||
| .with_config(config) | ||
| .with_scenario(Scenario::AccessLogStreaming) | ||
| .with_scenario(Scenario::AccessLogStreamingByPodTime) |
Contributor
There was a problem hiding this comment.
This change seems unrelated to the rest of this PR? Or on more review it seems related to forcing an error. However it is really strange to me that we are reusing the tests for memory limitiing to test optimizer errors. I would suggest we move the optimizer error stuff into an optimizer_integration style test
Contributor
Author
There was a problem hiding this comment.
These two tests aren't checking optimizer errors. They still check the sort merge join's memory use. Their rule list is only JoinSelection, so the SortMergeJoinExec read unsorted input, and it ran only because that list left out SanityCheckPlan. The planner now rejects that plan, so the tests sort the generated rows by the join keys and declare that order on the table. That keeps the plan valid without adding a SortExec, whose memory budget would be hit before the join's. The optimizer error tests are in core/tests/physical_optimizer/sanity_checker.rs, so I'd keep these two here as they are.
| Ok(()) | ||
| } | ||
|
|
||
| /// Removes every [`SortExec`], standing in for a rule that breaks ordering |
Contributor
There was a problem hiding this comment.
FWIW I was confused at first why this is needed. It is a test it turns out. Maybe calling it MockRule or adding a comment like '/// Test rule to check messages' would be more clear
|
|
||
| /// The default rules produce a plan that passes the final check. | ||
| #[tokio::test] | ||
| async fn test_final_check_passes_default_rules() -> Result<()> { |
Contributor
There was a problem hiding this comment.
this I think is redundant as it is tested many times in the rest of the code
|
|
||
| /// A custom rule list is checked too. | ||
| #[tokio::test] | ||
| async fn test_final_check_names_rule_in_custom_pipeline() -> Result<()> { |
Contributor
There was a problem hiding this comment.
How is this different than test_final_check_names_appended_rule ?
| // This runs once after all optimizer runs are complete, including | ||
| // rules appended by the user, to verify that the plan is executable. | ||
| let optimizer_options = &session_state.config_options().optimizer; | ||
| if let Err(e) = check_plan_tree_sanity(&new_plan, optimizer_options) { |
Contributor
There was a problem hiding this comment.
I actually think we shouldn't allow plans to opt out of plan sanity checking -- if the generated plan doesn't pass sanity check then it will fail in some other way (either wrong results or worse). Catching it early is better I think
| physical_plan after PushdownSort SAME TEXT AS ABOVE | ||
| physical_plan after EnsureCooperative SAME TEXT AS ABOVE | ||
| physical_plan after FilterPushdown(Post) SAME TEXT AS ABOVE | ||
| physical_plan after SanityCheckPlan SAME TEXT AS ABOVE |
Contributor
There was a problem hiding this comment.
I do like that this is no longer an OptimizerRule
|
|
||
| ### `SanityCheckPlan` is deprecated and no longer in the default physical optimizer rules | ||
|
|
||
| The physical planner now checks the final physical plan itself, after the last |
Contributor
There was a problem hiding this comment.
I think we could probably shorten this and note that the SanityCheckPlan has been replaced with a method and if custom pipelines need to check the plan they should call the new method directly
alamb
approved these changes
Oct 29, 2026
Contributor
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing planner-final-plan-check (0909e8e) to e8940ac (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing planner-final-plan-check (0909e8e) to e8940ac (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
Contributor
Author
|
Thanks @alamb @zhuqi-lucas for review! |
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?
Related: #26171 (option 3), #26351, #26360, #25688.
Rationale for this change
SanityCheckPlanis the last rule inPhysicalOptimizer::new(), so rules appended withSessionStateBuilder::with_physical_optimizer_rulerun after it and are never checked. Onmain, an appended rule that removes a window's sort silently returns wrongrow_number()values. Custom pipelines have to keep the rule last by hand, and a custom list without it runs plans whose requirements do not hold. When the check does fail, the error only saysSanityCheckPlan, so finding the rule that broke the plan has needed a manual bisect each time (for example #26391, #17077, #25921).@alamb suggested this change in the review of #26107: remove
SanityCheckPlanas an optimizer pass and always run it after the optimizer is done (comment).#26171 agreed (option 3) that a custom pipeline must keep the plan valid, while optimization coverage is not guaranteed. This PR makes that validity part checked for every pipeline. The review on #26360 asked for the same shape: one rule,
EnsureRequirements, makes the plan valid, and every later rule keeps it valid.What changes are included in this PR?
SanityCheckPlanleaves the default rule list.DefaultPhysicalPlanner::optimize_physical_planruns the same check (unbounded inputs, ordering, distribution,check_invariants(Executable)) after the last rule, including appended rules. It replaces the planner's ownInvariantChecker(Executable)walk, whichSanityCheckPlanrepeated.When the ordering or distribution requirements fail, the planner runs the rules again from the input plan to name the rule that broke them, so planning that succeeds keeps no intermediate plans:
or, when the plan was already invalid as
EnsureRequirementsproduced it:Plans before
EnsureRequirementsneed not satisfy the requirements, so only plans from its last run onwards are checked. The rule is recognized by its name; a separate enforcement phase (feat: run physical requirement enforcement first, as a PhysicalAnalyzer phase #25688) would give a cleaner boundary. Errors about unbounded inputs keep the plainSanityCheckPlancontext, so existing messages do not change.sanity_checkerexposescheck_plan_tree_sanity,check_input_requirementsandinput_requirements_hold, which the planner uses.SanityCheckPlanis deprecated. Tests that used it to validate a plan callcheck_plan_tree_sanityinstead.The upgrade guide, the optimizer rule reference and
explain.sltare updated.What is the testing strategy for this PR?
sanity_checker.rs: a rule appended after the default rules that removes sorts is reported by name; a custom list withoutEnsureRequirementsreports that no rule produced a valid plan.memory_limit/mod.rs: the two sort merge join tests ran aSortMergeJoinExecover unsorted streams through a custom rule list withoutEnsureRequirements. They now sort the generated data by the join keys and declare that order, so the plan is valid and the tests still measure the join's memory use.Are there any user-facing changes?
with_physical_optimizer_rule, or by a custom rule list, are now checked. A plan whose requirements do not hold, such as a sort merge join over unsorted input, fails to plan instead of running and possibly returning wrong results.PhysicalOptimizer::new()no longer containsSanityCheckPlan, andEXPLAIN VERBOSEno longer printsphysical_plan after SanityCheckPlan. Code that looks the rule up in the default list no longer finds it.SanityCheckPlanitself is deprecated; code that runs optimizer rules without the planner can callsanity_checker::check_plan_tree_sanityon the final plan.This changes the public behavior of the default rule list, so it needs the
api changelabel.