doc: Document the physical optimizer contract by 2010YOUY01 · Pull Request #26107 · apache/datafusion

2010YOUY01 · 2026-10-07T08:58:18Z

Which issue does this PR close?

Closes #.

Rationale for this change

This PR tries to captures the current state and explains it. Please let me know if I’ve missed anything.

What changes are included in this PR?

docs only

What is the testing strategy for this PR?

Are there any user-facing changes?

codecov-commenter · 2026-10-07T09:38:04Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.73%. Comparing base (7b1e55d) to head (10924ce).
⚠️ Report is 284 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #26107    +/-   ##
========================================
  Coverage   82.72%   82.73%            
========================================
  Files        1147     1147            
  Lines      449175   449509   +334     
  Branches   449175   449509   +334     
========================================
+ Hits       371598   371892   +294     
- Misses      54929    54945    +16     
- Partials    22648    22672    +24

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jayzhan211 · 2026-10-09T02:29:10Z

+//!       ```
+//!
+//!       Do not extend built-in rules or add unit tests within DataFusion
+//!       solely to support such downstream pipelines.


Two pipelines that differ from the default order already exist, and the doc doesn't say whether either is supported (3.3 also rules out the first):

Re-running the default sequence. Ballista AQE re-runs the whole rule list after every stage. Before fix(physical-optimizer): make OutputRequirements idempotent #22522, each run added another wrapper: OutputRequirementExec → OutputRequirementExec → SortExec. fix(physical-optimizer): make OutputRequirements idempotent #22522 made OutputRequirements idempotent, with tests, solely for that.

Appending. with_physical_optimizer_rule adds the rule after SanityCheckPlan, so its output is never checked. With a rule that drops SortExec:

SELECT a, ROW_NUMBER() OVER (ORDER BY a) AS rn FROM (VALUES (3), (1), (2)) AS t(a);

Inserted before SanityCheckPlan, it fails with does not satisfy order requirements. Appended, it silently returns (3,1) (1,2) (2,3).

Whether extra order is allowed is unspecified now, however allowing all possible composition is too permissive.

It means optimizer rules can be arbitrarily reordered, repeated, added, or removed, and everything should still work. In practice, many rules rely on implicit assumptions about the surrounding pipeline.

Ideally, we should explicitly define and enforce which compositions are valid. For example, a rule may be allowed to run multiple times, but must always run after rule X. Such constraints should be verifiable by the core, rather than relying on undocumented conventions.

So I'm thinking a practical approach might be to first restrict it, and next add mechanisms to specify allowed extensions. The goal is not to eliminate extensibility, but to make its guarantees explicit and maintainable.

2. Appending. with_physical_optimizer_rule adds the rule after SanityCheckPlan, so its output is never checked. With a rule that drops SortExec:

This is a good reason in my mind to remove SanityCheckPlan as an OptimizerPass and always run it after the optimizer is done. maybe we can file a ticket / PR to do so

It means optimizer rules can be arbitrarily reordered, repeated, added, or removed, and everything should still work.

Other than EnforceRequirements I think this is true today, as long as you define "everything should still work" as "the plan generates correct output answers".

In practice, many rules rely on implicit assumptions about the surrounding pipeline.

This is true for a bunch of the optimizations -- they look for specific plan patterns created by prior optimizer passes. However I don't think they rely on specific plan patters for generating correct answers

Re-running the default sequence. Ballista AQE re-runs the whole rule list after every stage.

Our system in InfluxDB3 also runs quite a few custom optimizers

alamb

Thank you for trying to help document this better @2010YOUY01 and @jayzhan211

I think having documentation is great, but I am not sure this PR currently reflects what DataFusion does or where we want to head

However, I think trying to document how it works is a perfect place to start building consensus, so it is really valuable

alamb · 2026-10-09T19:02:23Z

+//! ];
+//! ```
+//!
+//! 1. **Keep the default order.** Rules may rely on properties established by


I think this is overly pessimistic.

I think the only rules that have correctness requirements are OutputRequirements and EnforceRequirements.

Other rules may make assumptions about plan shape for performance but not for correctness that I understand.

This is part of the reason i would like to treat EnforceRequirements specially as an Analyzer ( #25688 / #25572 from @zhuqi-lucas) as it has a different function than the other optimizer rules

alamb · 2026-10-09T19:02:37Z

+//!       documented only in individual rules. Changes to the default pipeline
+//!       may require updates to rules that rely on them.
+//!
+//!    2. DataFusion aims to make these assumptions easier to understand and


Do we have a ticket that tracks this?

I'm formulating this idea 😄

alamb · 2026-10-09T19:11:18Z

+//!    2. DataFusion aims to make these assumptions easier to understand and
+//!       verify.
+//!
+//!    3. Extension rules should adapt to the built-in rules, not the other


I disagree with this -- we do aim to support arbitrary rules and I think many systems add their own custom rules already

I tried to propose a practical way to restrict it reasonably in

(see point 3) Discussion: Define the Contract for Custom Physical Optimizer Pipelines #26171

I think if we leave it unspecified, this assumption could be interpreted too broadly and eventually become a maintenance issue.

I am still not sure it is a good idea to add additional restrictions on semantics between optimizer passes. I think the semantics of what a ExecutionPlan needs to produce correct output should stay within the operator itself (e.g. a try_new method that verifies it).

I am aligned / agree with your point 3 on #26171 👍

alamb · 2026-10-09T19:12:50Z

+//!       ```
+//!
+//!       Do not extend built-in rules or add unit tests within DataFusion
+//!       solely to support such downstream pipelines.


2. Appending. with_physical_optimizer_rule adds the rule after SanityCheckPlan, so its output is never checked. With a rule that drops SortExec:

This is a good reason in my mind to remove SanityCheckPlan as an OptimizerPass and always run it after the optimizer is done. maybe we can file a ticket / PR to do so

alamb · 2026-10-09T19:14:31Z

+//!       ```
+//!
+//!       Do not extend built-in rules or add unit tests within DataFusion
+//!       solely to support such downstream pipelines.


It means optimizer rules can be arbitrarily reordered, repeated, added, or removed, and everything should still work.

Other than EnforceRequirements I think this is true today, as long as you define "everything should still work" as "the plan generates correct output answers".

In practice, many rules rely on implicit assumptions about the surrounding pipeline.

This is true for a bunch of the optimizations -- they look for specific plan patterns created by prior optimizer passes. However I don't think they rely on specific plan patters for generating correct answers

alamb · 2026-10-09T19:14:59Z

+//!       ```
+//!
+//!       Do not extend built-in rules or add unit tests within DataFusion
+//!       solely to support such downstream pipelines.


Re-running the default sequence. Ballista AQE re-runs the whole rule list after every stage.

Our system in InfluxDB3 also runs quite a few custom optimizers

2010YOUY01 · 2026-10-10T02:11:25Z

Thank you for the feedbacks @alamb and @jayzhan211 , I also have some great conversation with @zhuqi-lucas on a similar topic while reviewing #23599

I'd love to move the conversation into the below issue, I tried to explain a less restrictive and practical middle ground:

Discussion: Define the Contract for Custom Physical Optimizer Pipelines #26171
The key points are examples in point 2, and proposed contract in point 3

I'll rework this PR after reaching some agreements there.

@alamb

…roke it (apache#26461) ## Which issue does this PR close? - Closes apache#26460. Related: apache#26171 (option 3), apache#26351, apache#26360, apache#25688. ## Rationale for this change `SanityCheckPlan` is the last rule in `PhysicalOptimizer::new()`, so rules appended with `SessionStateBuilder::with_physical_optimizer_rule` run after it and are never checked. On `main`, an appended rule that removes a window's sort silently returns wrong `row_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 says `SanityCheckPlan`, so finding the rule that broke the plan has needed a manual bisect each time (for example apache#26391, apache#17077, apache#25921). @alamb suggested this change in the review of apache#26107: remove `SanityCheckPlan` as an optimizer pass and always run it after the optimizer is done ([comment](apache#26107 (comment))). apache#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 apache#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? - `SanityCheckPlan` leaves the default rule list. `DefaultPhysicalPlanner::optimize_physical_plan` runs the same check (unbounded inputs, ordering, distribution, `check_invariants(Executable)`) after the last rule, including appended rules. It replaces the planner's own `InvariantChecker(Executable)` walk, which `SanityCheckPlan` repeated. - 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: ```text SanityCheckPlan: order and distribution requirements stopped holding after physical optimizer rule 'ProjectionPushdown' caused by Error during planning: Plan: [...] does not satisfy order requirements: [...] ``` or, when the plan was already invalid as `EnsureRequirements` produced it: ```text SanityCheckPlan: the plan produced by physical optimizer rule 'EnsureRequirements' does not satisfy the order and distribution requirements ``` Plans before `EnsureRequirements` need 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 (apache#25688) would give a cleaner boundary. Errors about unbounded inputs keep the plain `SanityCheckPlan` context, so existing messages do not change. - `sanity_checker` exposes `check_plan_tree_sanity`, `check_input_requirements` and `input_requirements_hold`, which the planner uses. - `SanityCheckPlan` is deprecated. Tests that used it to validate a plan call `check_plan_tree_sanity` instead. - The upgrade guide, the optimizer rule reference and `explain.slt` are 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 without `EnsureRequirements` reports that no rule produced a valid plan. - `memory_limit/mod.rs`: the two sort merge join tests ran a `SortMergeJoinExec` over unsorted streams through a custom rule list without `EnsureRequirements`. 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. - The extended workspace suite passes. ## Are there any user-facing changes? - Plans produced by rules added with `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 contains `SanityCheckPlan`, and `EXPLAIN VERBOSE` no longer prints `physical_plan after SanityCheckPlan`. Code that looks the rule up in the default list no longer finds it. `SanityCheckPlan` itself is deprecated; code that runs optimizer rules without the planner can call `sanity_checker::check_plan_tree_sanity` on the final plan. - Errors for broken ordering or distribution requirements name the rule that broke them. This changes the public behavior of the default rule list, so it needs the `api change` label. --------- Co-authored-by: Andrew Lamb <[email protected]> Co-authored-by: Claude Fable 5.1 <[email protected]>

doc: Document the physical optimizer contract

833aebd

github-actions Bot added the optimizer Optimizer rules label Oct 7, 2026

2010YOUY01 requested a review from alamb October 7, 2026 08:58

2010YOUY01 added 2 commits October 8, 2026 10:37

improve wording

c3a2009

improve wording

10924ce

jayzhan211 reviewed Oct 9, 2026

View reviewed changes

alamb reviewed Oct 9, 2026

View reviewed changes

2010YOUY01 marked this pull request as draft October 10, 2026 02:11

andygrove mentioned this pull request Oct 14, 2026

[WIP][Experimental] feat: add datafusion.execution.isolated_partitions for distributed-safe join selection #26330

Draft

jayzhan211 mentioned this pull request Oct 18, 2026

Check the final physical plan in the planner and name the rule that broke it #26461

Merged

Conversation

2010YOUY01 commented Oct 7, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2010YOUY01 commented Oct 10, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Oct 7, 2026 •

edited

Loading