[jayzhan211]

Which issue does this PR close?

• Part of EPIC: Separate optimizer rules from correctness transformation rules #26481.
• An alternative to feat: add a physical analyzer phase that hands the optimizer a valid plan #26757, following @2010YOUY01's suggestion there to mark the phase on the existing trait instead of adding a second one. Only one of the two should land; this one is a draft so the two can be compared.

Rationale for this change

The physical plan meets the distribution and ordering requirements of its operators only after EnsureRequirements. Whether a rule is correct depends on which side of it the rule runs, but nothing records or checks that: the optimizer contract in optimizer.rs (every rule returns a valid plan, and the rules can be reordered) lists EnsureRequirements and OutputRequirements as exceptions and says nothing about the other rules that run before enforcement, and the planner's final check (#26461) finds the boundary by looking for the name EnsureRequirements. A rule on the wrong side can return wrong results that the final check does not catch: with LimitPushdown before enforcement, 11 sqllogictest queries return wrong rows (measurements).

This PR records the side on each rule, with one required method on the existing trait, and has the planner check it.

What changes are included in this PR?

Three commits:

1. PhysicalOptimizerRule::phase(), a required method, returns PhysicalOptimizerPhase::Analyzer for a rule that runs while the requirements are only declared, or PhysicalOptimizerPhase::Optimizer for a rule that receives a plan meeting them and must return one. The eight default rules up to and including EnsureRequirements return Analyzer; OutputRequirements and FilterPushdown return the phase of their mode, so they stay one type each. The planner checks the order once per plan and fails planning when an optimizer rule runs before an analyzer rule, with an error that names both rules and their positions. The final check from Check the final physical plan in the planner and name the rule that broke it #26461 finds the boundary by phase instead of by the name EnsureRequirements. FFI_PhysicalOptimizerRule and FFI_SessionRef keep their layouts, so they stay compatible with DataFusion 55 (fix(ffi): prevent recursive session physical planning #24492 kept FFI_SessionRef compatible on purpose); a rule received through FFI reports Optimizer.
2. The upgrade guide and the optimizer rule reference, whose old "phase" column (add/remove, pre/post, early/late) is renamed "pass" so it doesn't clash with the new phases.
3. A third phase, PhysicalOptimizerPhase::Enforcement, for EnsureRequirements. With two phases, an analyzer rule placed right after EnsureRequirements passes the order check and silently gives worse plans (for example WindowTopN keeps the window's sort). With three, the planner requires Analyzer, then Enforcement, then Optimizer, and the final check names the enforcement rule, or no rule when there is none, instead of whichever analyzer rule comes last. This commit needs agreement on the extra phase and can be dropped without changing the first two. If the planner later runs enforcement itself, as EPIC: Separate optimizer rules from correctness transformation rules #26481 plans, this variant goes away with it.

The default list, its order, the rule names and EXPLAIN VERBOSE output do not change.

Open question on naming: most rules in the Analyzer phase are performance rules, unlike the logical analyzer, so BeforeEnforcement, Enforcement and AfterEnforcement would describe the phases by position instead. I kept Analyzer and Optimizer because #26481 and the discussion use them.

Compared with #26757:

	this PR (phase())	#26757 (two traits)
How a rule declares its side	phase(), required, on the existing trait	the trait it implements
Interfaces of the two kinds of rules	one trait	two traits with the same four methods
New public API	PhysicalOptimizerPhase and PhysicalOptimizerRule::phase	PhysicalAnalyzerRule, PhysicalAnalyzer, session and builder methods, two FFI types, three rule types
An optimizer rule placed before enforcement	planning error	compile error
An analyzer rule placed after enforcement	planning error (commit 3)	not caught
Moving a rule to the other side later	change its phase()	change the trait it implements
Loud changes for existing code	every rule must implement phase() (compile error); an optimizer rule before enforcement, an analyzer rule after it (commit 3), EnsureRequirements run again after optimizer rules, and a rule received through FFI placed before enforcement (planning errors)	calls to the moved rules, renamed constructors and the observer signature (compile errors)
Silent changes for existing code	none in plans or results; in a session received through FFI every rule counts as Optimizer, so the order check passes there without checking anything	rules in front of the default list run after enforcement; an empty optimizer list still enforces; name filters and rule wrappers miss the analyzer rules; a custom Session that doesn't return the analyzer rules skips them for plans that need no enforcement (plans that need it fail the final check)
FFI	no layout change	FFI_SessionRef gains a field, with no version check
Size	+670 −78 (+598 −78 without commit 3)	+1630 −532

What is the testing strategy for this PR?

• The full sqllogictest suite passes with no plan changes and no explain.slt changes.
• physical_planner.rs: an optimizer rule in front of the default rules fails planning and the error names both rules and their positions; the same rule declared Analyzer plans; LimitPushdown moved before EnsureRequirements, an analyzer rule appended with with_physical_optimizer_rule, and EnsureRequirements appended again after the optimizer rules each fail planning; with commit 3, an analyzer rule right after EnsureRequirements fails planning.
• sanity_checker.rs: the final check names the enforcement rule when it leaves a requirement unmet, and names no rule when the list has analyzer rules but no enforcement rule; the existing Check the final physical plan in the planner and name the rule that broke it #26461 tests pass unchanged.
• optimizer.rs: every default rule declares the phase that matches its position.
• ffi: a rule received through FFI reports Optimizer, and a rule that comes back to its own library keeps its phase.
• sort_pushdown.slt inserts a test-only rule before EnsureRequirements. It failed planning with the new error until it declared Analyzer, which is the change downstream rules placed before enforcement need.

Are there any user-facing changes?

Yes, documented in the 56.0.0 upgrade guide. Every PhysicalOptimizerRule implementation must add phase(); most return PhysicalOptimizerPhase::Optimizer, and a wrapper that only observes another rule returns that rule's phase. A rule list that puts a rule on the wrong side of EnsureRequirements, or runs EnsureRequirements again after optimizer rules, fails planning with an error that names both rules. A rule received through FFI reports Optimizer; to run one before enforcement, wrap it in a local rule that returns Analyzer.

[jayzhan211]

Another idea is 3 phase design, analyzer → enforcement → optimizer 🤔

[jayzhan211]

I'll try 3 phase design, turn it to draft

[github-actions]

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

     Cloning 5db4b233efde58f6b34299fb15b756e7bdef2698
    Building datafusion v55.1.0 (current)
       Built [  51.776s] (current)
     Parsing datafusion v55.1.0 (current)
      Parsed [   0.031s] (current)
    Building datafusion v55.1.0 (baseline)
       Built [  16.223s] (baseline)
     Parsing datafusion v55.1.0 (baseline)
      Parsed [   0.032s] (baseline)
    Checking datafusion v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.715s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  70.508s] datafusion
    Building datafusion-ffi v55.1.0 (current)
       Built [  26.572s] (current)
     Parsing datafusion-ffi v55.1.0 (current)
      Parsed [   0.063s] (current)
    Building datafusion-ffi v55.1.0 (baseline)
       Built [  18.424s] (baseline)
     Parsing datafusion-ffi v55.1.0 (baseline)
      Parsed [   0.063s] (baseline)
    Checking datafusion-ffi v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.304s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  46.716s] datafusion-ffi
    Building datafusion-physical-optimizer v55.1.0 (current)
       Built [  17.339s] (current)
     Parsing datafusion-physical-optimizer v55.1.0 (current)
      Parsed [   0.019s] (current)
    Building datafusion-physical-optimizer v55.1.0 (baseline)
       Built [  13.705s] (baseline)
     Parsing datafusion-physical-optimizer v55.1.0 (baseline)
      Parsed [   0.020s] (baseline)
    Checking datafusion-physical-optimizer v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.125s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  31.978s] datafusion-physical-optimizer
    Building datafusion-session v55.1.0 (current)
       Built [  12.817s] (current)
     Parsing datafusion-session v55.1.0 (current)
      Parsed [   0.007s] (current)
    Building datafusion-session v55.1.0 (baseline)
       Built [  12.491s] (baseline)
     Parsing datafusion-session v55.1.0 (baseline)
      Parsed [   0.007s] (baseline)
    Checking datafusion-session v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.233s] 229 checks: 228 pass, 1 fail, 0 warn, 31 skip

--- failure trait_method_added: pub trait method added ---

Description:
A non-sealed public trait added a new method without a default implementation, which breaks downstream implementations of the trait
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/trait_method_added.ron

Failed in:
  trait method datafusion_session::physical_optimizer::PhysicalOptimizerRule::phase in file /home/runner/work/datafusion/datafusion/datafusion/session/src/physical_optimizer.rs:192
  trait method datafusion_session::PhysicalOptimizerRule::phase in file /home/runner/work/datafusion/datafusion/datafusion/session/src/physical_optimizer.rs:192

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  26.465s] datafusion-session