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

2010YOUY01 · 2026-10-15T03:26:10Z

Which issue does this PR close?

Closes Discussion: Define the Contract for Custom Physical Optimizer Pipelines #26171

Rationale for this change

See issue for rationale, this PR documents what has been agreed on

What changes are included in this PR?

doc only

What is the testing strategy for this PR?

Are there any user-facing changes?

2010YOUY01 · 2026-10-15T03:33:15Z

+//!   example, [`EnsureRequirements`] enforces input distribution and ordering.
+//!   Some constraints remain implicit or are documented only in individual rules.
+//!
+//! - **Optimization Coverage:** A non-default rule order may expose rules to


A CI tool might help to verify optimization coverage is reachable from queries.

ci: add slt-only test coverage #26219

codecov-commenter · 2026-10-15T03:54:12Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.89%. Comparing base (3d9037c) to head (35bcb1b).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #26351      +/-   ##
==========================================
- Coverage   82.89%   82.89%   -0.01%     
==========================================
  Files        1154     1154              
  Lines      457961   457961              
  Branches   457961   457961              
==========================================
- Hits       379637   379620      -17     
- Misses      55285    55302      +17     
  Partials    23039    23039

☔ 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.

alamb

Thank you @2010YOUY01 and @jayzhan211 and @zhuqi-lucas

I think this is better than what we current have, so I am approving it. However, I found it somewhat hard to understand as written and have a few suggestions on how to improve it.

It would also be fine with me to merge this PR in as is and I can make a follow on PR with a proposal to improve the wording

alamb · 2026-10-16T13:58:53Z

+//! disables the distinct aggregation soft-limit optimization. Custom pipelines
+//! require more care, as described below.
+//!
+//! ## Custom Optimizer Pipelines


I found this somewhat confusingly worded. I would find it clearer if we started by stating the invariants:

All optimizer rules must preserve plan correctness (the output plan can be run and produces the same results as the input plan). We can note there are some built in plans that aren't yet there (like EnforceRequirements)

That reordering optimizer rules (other than the exceptions) should result in a plan that produces the same results

Then we could then move on to the explicit warning that adding / reordering rules may make some of the built in rules less effective as some built in rules (e.g. CombinePartialFinalAggregate) match certain plan shapes.

p.s. I would love to split out EnforceRequrements into its own Analyzer thing precisely so we don't have to have to mention the special case for EnforceRequirements above

Thank you for the review! I've addressed other feedbacks since they're smaller wording improvements, but not this one.

I think i don't fully get how you want to express it better, feel free to directly push changes, or do a follow-up PR 🙏🏼

Thank you

feel free to directly push changes, or do a follow-up PR 🙏🏼

I pushed cc2ab97 which is a fairly substantial rewording -- please feel free to revert it if you don't agree

This reads like a great improvement. Thank you.

alamb · 2026-10-18T12:37:25Z

 /// computes the same results, but in a potentially more efficient way.
 ///
-/// Use [`SessionState::add_physical_optimizer_rule`] to register additional
+/// The returned plan must be valid and produce the same results as the input


I also felt adding some more cross references here will make it easier to find

2010YOUY01 · 2026-10-19T07:37:57Z

+/// [`EnsureRequirements`] and only merges a `Partial` aggregate that is the
+/// direct child of a `Final` aggregate.
+///
+/// ```text


I feel this example isn't a straightforward demonstration of canonical vs. non-canonical forms, as it requires some knowledge of DataFusion-specific implementation details.

It would be better to find a simpler example, though I can't think of one yet. I'll update it when I come up with one.

@jayzhan211

… optimizer pipeline (apache#26479) ## Which issue does this PR close? - Related to apache#26351 (split out so it can be reviewed and merged independently) ## Rationale for this change While reviewing apache#26351 @jayzhan211 @2010YOUY01 and I have observed that `EnsureRequirements` is special among the physical optimizer rules, but that isn't documented on the rule itself ## What changes are included in this PR? Documentation only, in `datafusion/physical-optimizer/src/ensure_requirements/mod.rs`: ## What is the testing strategy for this PR? CI ## Are there any user-facing changes? No, only rustdoc text. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

@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 <andrew@nerdnetworks.org> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

2010YOUY01 added 2 commits October 15, 2026 11:19

doc: Document the physical optimizer contract

b890358

minor

3df6d48

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

2010YOUY01 mentioned this pull request Oct 15, 2026

Discussion: Define the Contract for Custom Physical Optimizer Pipelines #26171

Closed

2010YOUY01 commented Oct 15, 2026

View reviewed changes

jayzhan211 approved these changes Oct 15, 2026

View reviewed changes

zhuqi-lucas approved these changes Oct 16, 2026

View reviewed changes

alamb approved these changes Oct 16, 2026

View reviewed changes

review: improve wording

748e3b7

This was referenced Oct 18, 2026

Physical plan check does not cover appended optimizer rules and does not name the rule that broke the plan #26460

Closed

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

Merged

alamb mentioned this pull request Oct 18, 2026

docs(minor): Explain the role of EnsureRequirements in the physical optimizer pipeline #26479

Merged

docs: proposed update to optimizer doc

cc2ab97

alamb reviewed Oct 18, 2026

View reviewed changes

alamb mentioned this pull request Oct 18, 2026

EPIC: Separate optimizer rules from correctness transformation rules #26481

Open

3 of 8 tasks

alamb added 2 commits October 18, 2026 09:18

add epic link

bbb1fb5

Merge remote-tracking branch 'apache/main' into optimizer-contract

35bcb1b

2010YOUY01 commented Oct 19, 2026

View reviewed changes

2010YOUY01 added this pull request to the merge queue Oct 19, 2026

Merged via the queue into apache:main with commit 862843f Oct 19, 2026
42 checks passed

2010YOUY01 deleted the optimizer-contract branch October 19, 2026 08:19

github-actions Bot added the v56.0.0 label Oct 19, 2026

jayzhan211 mentioned this pull request Oct 24, 2026

[EPIC] Custom ExecutionPlan nodes: correct results, and the same optimizations as built-in nodes #26697

Open

7 tasks

Navigation Menu

Repository navigation

doc: Document the physical optimizer contract - #26351

doc: Document the physical optimizer contract#26351
2010YOUY01 merged 6 commits into
apache:mainfrom
2010YOUY01:optimizer-contract

2010YOUY01 commented Oct 15, 2026

Uh oh!

2010YOUY01 Oct 15, 2026

Uh oh!

codecov-commenter commented Oct 15, 2026 •

edited

Loading

Uh oh!

alamb left a comment

Uh oh!

alamb Oct 16, 2026

Uh oh!

2010YOUY01 Oct 17, 2026

Uh oh!

alamb Oct 18, 2026

Uh oh!

2010YOUY01 Oct 19, 2026

Uh oh!

Uh oh!

Uh oh!

alamb Oct 18, 2026

Uh oh!

2010YOUY01 Oct 19, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

2010YOUY01 commented Oct 15, 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!

2010YOUY01 Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

2010YOUY01 Oct 17, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Oct 18, 2026

Choose a reason for hiding this comment

Uh oh!

2010YOUY01 Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb Oct 18, 2026

Choose a reason for hiding this comment

Uh oh!

2010YOUY01 Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Oct 15, 2026 •

edited

Loading