jayzhan211 opened a new pull request, #26979: URL: https://github.com/apache/datafusion/pull/26979
## Which issue does this PR close? - Part of #26481. - An alternative to #26757, following @2010YOUY01's [suggestion there](https://github.com/apache/datafusion/pull/26757#discussion_r4431276937) 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 the rules before it as exceptions, 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](https://github.com/apache/datafusion/issues/26481#issuecomment-6472945196)). This PR records the side on each rule, with one method on the existing trait, and has the planner check it. ## What changes are included in this PR? - `PhysicalOptimizerRule::phase()` returns `PhysicalOptimizerPhase::Analyzer` for a rule that runs while the requirements are only declared, or `PhysicalOptimizerPhase::Optimizer` (the default) for a rule that receives a plan meeting them and must return one. - The eight 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: when a rule list puts an analyzer rule after an optimizer rule, planning fails with an error that names both rules and says how to fix it. - The final check from #26461 finds the end of the analyzer rules by phase instead of by the name `EnsureRequirements`. - FFI: `FFI_PhysicalOptimizerRule` keeps its layout, so it stays compatible with DataFusion 55 as #24492 intended. A rule received through FFI reports the default phase. The default list, its order, the rule names and `EXPLAIN VERBOSE` output do not change. Compared with #26757: | | this PR | #26757 | |---|---|---| | New public API | `PhysicalOptimizerPhase` and `PhysicalOptimizerRule::phase` | `PhysicalAnalyzerRule`, `PhysicalAnalyzer`, session and builder methods, two FFI types, three rule types | | A rule on the wrong side | planning error | compile error | | Moving a rule to the other side later | change its `phase()` | change the trait it implements (API break) | | Rules placed in front of the default list | planning error until they return `Analyzer` | compile and silently run after enforcement | | Empty rule list, rule wrappers, custom `Session`, observer, rule names | unchanged | change (see its upgrade guide) | | FFI layout | unchanged | `FFI_SessionRef` gains a field | | Size | +365 −42 | +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; the same rule declared `Analyzer` plans; the default list starts with the eight analyzer rules. - `sanity_checker.rs`: the final check names the last analyzer rule when the analyzer rules leave a requirement unmet; the existing #26461 tests pass unchanged. - `optimizer.rs`: every default rule up to and including `EnsureRequirements` declares `Analyzer`, every later rule `Optimizer`. - `sort_pushdown.slt` inserts a test-only rule before `EnsureRequirements`. It failed planning with the new error until it declared `Analyzer`, which is the one-line change downstream rules placed before enforcement need. ## Are there any user-facing changes? Yes, documented in the 56.0.0 upgrade guide. `phase()` has a default, so existing rules compile unchanged. A custom rule placed before `EnsureRequirements` (for example in front of `PhysicalOptimizer::new().rules`) now fails planning with an error that says to return `PhysicalOptimizerPhase::Analyzer` from its `phase()`. A rule list that runs an analyzer rule such as `EnsureRequirements` again after optimizer rules fails the same way. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
