Repository navigation
feat: add a physical analyzer phase that hands the optimizer a valid plan - #26757
Draft
jayzhan211 wants to merge 9 commits into
Draft
jayzhan211 wants to merge 9 commits into
jayzhan211 wants to merge 9 commits into
Conversation
github-actions
Bot
added
documentation
Contributor
Author
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing physical-analyzer-core (ab53c9f) to 8ea0dbf (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 physical-analyzer-core (ab53c9f) to 8ea0dbf (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
physical-analyzer-core
branch
from
October 26, 2026 05:00
ab53c9f to
3638b40
Compare
Contributor
Author
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing physical-analyzer-core (3638b40) to 8ea0dbf (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 physical-analyzer-core (3638b40) to 8ea0dbf (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
physical-analyzer-core
branch
from
October 26, 2026 12:43
3638b40 to
2c410b5
Compare
1 of 8 tasks
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 optimizer rules run in one hand-ordered list, and the plan is only valid after
EnsureRequirements. A custom rule list that drops or movesEnsureRequirementsgets a plan that cannot run, and a rule that breaks a requirement has no shared way to repair it.This PR splits physical planning into a
PhysicalAnalyzerphase that makes the plan executable, as the logical analyzer makes a logical plan valid, and thePhysicalOptimizerphase that makes it faster. The analyzer phase ends withEnsureRequirements, so everyPhysicalOptimizerRulereceives a valid plan and must return one. Some performance rules, such asWindowTopNand the statistics-based choices injoin_selection, still have to run beforeEnsureRequirements; they run in the analyzer list throughRunInAnalyzer. Every built-in rule is still aPhysicalOptimizerRule, so for now the correctness rules run throughRunInAnalyzertoo. Spark and Trino also split their rules at enforcement, by comment; here the split is two lists, checked in debug builds.The default lists keep today's rule order, so no plan changes. This PR moves no rule across enforcement; moving
WindowTopNorjoin_selectionneeds fixes in those rules first (#26481).It also adds
satisfy_requirements, which inserts only the missing repartitions and sorts, and returns a plan that already meets its requirements unchanged. Debug builds call it at the end of the analyzer phase to check the phase. No rule calls it yet; it is the repair step for rules that later move after enforcement.EnsureRequirementsdoes not call it, because it also makes performance choices (round-robin repartitioning, join-key reordering, sort rewrites).What changes are included in this PR?
One commit per step:
PhysicalAnalyzerRuletrait and thePhysicalAnalyzerrule list, wired throughSession,SessionStateandSessionStateBuilder. The planner runs the analyzer rules before the optimizer rules.RunInAnalyzerruns aPhysicalOptimizerRulein the analyzer. It replaces eight copiedPhysicalAnalyzerRuleimpls. Invariant-check errors now say which phase the failing rule ran in.DefaultPhysicalPlanner::optimize_physical_plankeeps its signature. It sees each analyzer rule as aPhysicalOptimizerRulewith the same name, so existing callers compile unchanged.ForeignSessionforwards the analyzer rules through FFI. Without this, a join planned through aForeignSessionafter commit 1 keepsPartitionMode::Autoand fails at execution.OutputRequirementExecleft after the optimizer rules. After commit 1 the analyzer adds the marker even when a custom optimizer list has no remove pass, and the marker then reached the final plan, which could not be serialized to proto.EquivalencePropertiescould not tell that a plan sorted onsqrt(a)satisfies a requirement onsqrt(a); the same goes forln,log2,log10,acos,asin,acoshandatanh. These functions fail to derive an ordering when the input may be outside their domain, and that failure also discarded the ordering the expression already had. No plan in the test suite changes.satisfy_requirements, a public function that insertsRepartitionExec,CoalescePartitionsExec,SortPreservingMergeExecandSortExeconly where a declared requirement is unmet. It uses the same check for "satisfied" asSanityCheckPlan, which now calls the shared code.satisfy_requirementshas nothing to insert. A session with no analyzer rules is not checked.What is the testing strategy for this PR?
Full sqllogictest suite with no golden changes, and in debug builds every query passes the new analyzer-phase check. Without commit 6, the check fails on
ORDER BY ACOSH(SIN(v1))inscalar.slt.In
core/tests/physical_optimizer/ensure_requirements.rs, every test plan is also checked withsatisfy_requirements: on the input it makes the plan passSanityCheckPlanand then does nothing more, and onEnsureRequirements' output it does nothing.New unit tests:
test_known_ordering_when_expr_properties_failfor commit 6, andanalyzer_that_leaves_a_requirement_unmet_fails_in_debugfor commit 8. Both fail without their commit.FFI:
test_foreign_session_runs_physical_analyzersfails without commit 4 (HashJoinExec: mode=Auto), andtest_ffi_sessionchecks that both sessions list the same analyzer rules.Commit 1 cleared the analyzer list in proto's
analyze_roundtrip_unoptimizedand in the FFI query planner tests; after commit 5 they pass without that.output_requirements::scalar_subquery_root_preserves_global_ordering_end_to_endnow runs the analyzer rules before the optimizer rules, as the planner does.Planning time,
sql_planneron the benchmark bot, this head against main:Across all 103 physical benchmarks the ratio is 0.96 to 1.06, median 1.00. The
logical_*benchmarks run no code from this PR and range from 0.93 to 1.01 in the same run, so these differences are within noise.Are there any user-facing changes?
Yes, documented in the 56.0.0 upgrade guide. Default plans do not change, and no public signature changes. New public API:
PhysicalAnalyzerRule,PhysicalAnalyzer,RunInAnalyzer,satisfy_requirements,Session::physical_analyzers,SessionState::physical_analyzersandadd_physical_analyzer_rule, andSessionStateBuilder::with_physical_analyzer_rule(s).EXPLAIN VERBOSEshows the analyzer rules. When a rule in the analyzer list fails the schema or invariant check, the error namesPhysicalAnalyzerwhere it used to namePhysicalOptimizer.Downstream impact
I searched public code on GitHub for the APIs this PR touches. Private code can have the same patterns.
iox_queryputs its rules before DataFusion's), LaminarDB (puts a streaming validator first)RunInAnalyzerwith_physical_optimizer_rules(vec![])no longer gives a plan without enforcementarroyo-plannerclears the list)with_physical_analyzer_rules(vec![])SessionState::physical_optimizers()no longer lists the analyzer rulesEXPLAINoutput from each rule), datafusion-dft (lists rule names)physical_analyzers()Sessionthat overridesphysical_optimizersgets no enforcement unless it also overridesphysical_analyzers; planning or execution then failsSessionState::physical_analyzers()EnsureRequirements, or repair withsatisfy_requirementsBallista, delta-rs, Sail, Spice and optd call
optimize_physical_planwith an observer; its signature is unchanged, so they compile as before.Notes for reviewers
EliminateConstantJoin, added to main after feat: run physical requirement enforcement first, as a PhysicalAnalyzer phase #25688 was opened, keeps its place beforejoin_selection. Moved after enforcement, it would run afterFilterPushdown, and the three plans ineliminate_constant_join.sltwould lose their parquetpredicate=andpruning_predicate=.FFI_SessionRefgets a new field at the end, asFFI_AggregateUDFdid in fix(ffi): forward distinct_handling through FFI_AggregateUDF #25876. fix(ffi): prevent recursive session physical planning #24492 kept this struct compatible with consumers built against DataFusion 55, and the new field breaks that. If 55 compatibility matters here, I can skip the call when the other side reportsversion() < 56.