jayzhan211 commented on PR #26757: URL: https://github.com/apache/datafusion/pull/26757#issuecomment-6551846533
@2010YOUY01 @zhuqi-lucas thanks for the reviews. I opened #26979 with the `phase()` design so the two can be compared on real code, and I'd like to continue with #26979. **Why I opened this PR with two traits.** Two traits give a clear analyzer/optimizer concept that mirrors the logical plan's `AnalyzerRule` and `OptimizerRule`. Looking at it again, I don't think the physical layer needs that today. The logical traits differ for a reason: `analyze` returns a plan, while `rewrite` returns `Transformed` and has `apply_order`, because the logical optimizer loops to a fixpoint. The two physical traits in this PR have the same four methods. The logical analyzer also holds only correctness rules such as type coercion, while most physical rules before enforcement are performance rules. So the analogy is weaker than it looks. **The end state I'm aiming for (#26481)** - The physical planner owns the steps a plan needs to be correct: it records the query's output ordering, picks each join's mode when it creates the join, and runs enforcement. - Rules before enforcement keep the results unchanged, may leave requirements unmet, and stay correct on plans that already contain repartitions and sorts. - Rules after enforcement receive a valid plan and return one, without relying on plan shapes for correctness, so they can be reordered and run again. - Both contracts are checked: the phase order on every plan, the analyzer output in debug builds, the final plan (#26461), and the sqllogictests under a second rule order (#26152). - Which performance rules run before enforcement is decided per rule by plan quality. `LimitedDistinctAggregation` and `WindowTopN` can move after it once they handle enforced plans; the others lose plans after it today, so they stay. **How I'd choose** I'm fine with breaking changes when they buy a better design: fixing a compile error is cheap, especially with coding agents. What I want to avoid is a behavior change that downstream code can't see, and the same goes for FFI. So the questions are: 1. Do analyzer and optimizer rules need different interfaces? Not today; a rule only needs to declare its phase. 2. Will rules move between the phases? Yes: the end state moves at least two, and #26481 plans more. With two traits each move is another API break; with #26979 it is a `phase()` change. 3. Does existing code change behavior silently? With this PR it does, because the rule list is split: rules in front of the default list run after enforcement, an empty optimizer list still enforces, and name filters and rule wrappers miss the analyzer rules. With #26979 it doesn't; the only change is a planning error for rules placed before enforcement. 4. Does FFI fail loudly on a mismatch? DataFusion doesn't check `datafusion_ffi::version()` itself, so a host and a library built for different versions read a changed struct differently unless the host checks. This PR adds a field to `FFI_SessionRef`; #26979 changes no layout. If #26979 later passes the phase through FFI, the version check should come with it. 5. Is a compile-time error worth it over a planning-time one? Not by itself: the order check runs on every plan, so tests catch a misplaced rule before release. All five point to #26979. If analyzer rules later need their own interface, for example if the planner passes them the output requirement, a second trait can be added then. | | #26979 (`phase()`) | this PR (two traits) | |---|---|---| | How a rule declares its side | `phase()` 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 | | 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) | | Loud changes for existing code | rules placed before enforcement must return `Analyzer` (planning error) | calls to the moved rules, renamed constructors and the observer signature (compile errors); a custom `Session` without `physical_analyzers` (planning or execution error) | | Silent changes for existing code | none | 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 | | FFI | no layout change | `FFI_SessionRef` gains a field, with no version check | | Size | +365 −42 | +1630 −532 | With #26979, zhuqi's four comments here don't come up: moving a rule is a `phase()` change, nothing is renamed, the tests that edit the optimizer list still see every rule, and the FFI layout doesn't change. @alamb does this match what you had in mind for #26481, or would you rather keep two traits? I'll close the PR we don't continue with. -- 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]
