alamb commented on PR #25688: URL: https://github.com/apache/datafusion/pull/25688#issuecomment-5930525089
> I'm wondering what concrete issue or bug this PR is trying to solve, is it to prevent similar bugs like > > * [InterleaveExec::with_new_children panics when optimizer rewrites change children's partitioning #21826](https://github.com/apache/datafusion/issues/21826) > > Below are just some partial thoughts for reference. I'd like to probe into the target issue first to validate the reasoning, it's not suggesting a different implementation at this point. My understanding is that the driving rationale was to speed up query planning to the point where the optimizer rules can be run multiple times and stop when they have reached a fixed point (the plan is not changing) I think this is what @zhuqi-lucas describes in this ticket (though I agree the usecase is not clear) - https://github.com/apache/datafusion/issues/25355 > Placing `EnsureRequirements` in the middle may be a design that simplifies the overall implementation. The issue is that it introduces constraints on rewrites before and after it, and those constraints are not currently enforced explicitly, making the optimizer vulnerable to bugs. So I'm wondering whether there is a more flexible way to enforce the sanity check instead. > The gap now, I think, is that the invariants are not enforced explicitly in datafusion (for example, after `EnsureRequirements`, traversing the tree after each rewrite to verify that the distribution requirements still hold), so it seem to have many hidden knowledge to know if you want to add one new optimizer rule correctly. I agree invariants are not explicitly checked / easy to understand. @wiedld and I tried to add some invariant checking here: https://docs.rs/datafusion/latest/datafusion/physical_plan/trait.ExecutionPlan.html#method.check_invariants but it is not widely used nor expansive. > This is a common technique in compilers to organize very complex rewrites: split the pipeline into distinct phases and enforce the invariants. To keep overall transformation implementations simple, the general idea is to delay Stage 3 as late as possible, and try to put more rules in Stage 2, which seems to be in a different direction from this PR. This is a great point, and I think in Databases in general (and DataFusion in particular) it maps pretty well if you think about LogicalPlan --> PhysicalPlan as the "lowering" phase (e.g. the LogicalPlan is much simpler) > One possible direction is to make these phase boundaries explicit. Each phase would have a well-defined plan shape that its rules can assume, rather than relying on the implicit ordering between individual optimizer rules. Roughly, the optimizer rule list can be split into stages like: Yes, that is indeed similar to what I was thinking (and what I think the `PhysicalAnalayzerPass` is in my mind) I was hoping think the high level optimization flow is something like Phase 1: Logical Plan Optimization ** Stage 1: correctness: [AnalyzerRules](https://docs.rs/datafusion/latest/datafusion/optimizer/trait.AnalyzerRule.html) apply semantic changes to get the plan correct (type coercion, function rewrites, etc) ** Stage 2: optimization: [OptimizerRules](https://docs.rs/datafusion/latest/datafusion/optimizer/trait.OptimizerRule.html) -- tree rewrites that make the plan faster I was hoping to get a similar split in ExecutionPlan Optimization ** Stage 1: correctness (`PhysicalAnalyzerRule` -- this PR) -- that gets the plans executable (could be run) ** Stage 2: optimization [PhysicalOptimizerRule](https://docs.rs/datafusion/latest/datafusion/physical_optimizer/trait.PhysicalOptimizerRule.html) -- tree rewrites that make the plan faster -- 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]
