jayzhan211 commented on issue #26481: URL: https://github.com/apache/datafusion/issues/26481#issuecomment-6472945196
@2010YOUY01 yes, that was the idea, and the PoC says no. I moved each rule to the other side of `EnsureRequirements` and ran all sqllogictests. Every analyzer rule lost plans when moved after enforcement (`WindowTopN` keeps the window's sort, `LimitedDistinctAggregation` loses its limit), so I agree that performance rules belong in the analyzer. On the goal, I'd put it slightly differently: make the optimizer safe to change. Where a rule runs and what it may assume should be visible and checked, so that adding, moving or re-running a rule fails loudly instead of silently giving a worse plan or wrong results. Maintainability follows from that. The split helps because the boundary is real: `join_selection` after enforcement gives 767 invalid plans, and `LimitPushdown` and `CombinePartialFinalAggregate` before it return wrong results for 11 and 2 queries, from plans that still pass `SanityCheckPlan`. Two lists make that boundary part of the code instead of a position in a vector, give each side one contract that can be checked, and turn ordering assumptions such as #21596 and #26106 into guarantees within one planning pass. So I'd use shape diversity as the tie-breaker rather than the main test. Some rules add no diversity but read what enforcement produces: `CombinePartialFinalAggregate` removes a node, yet moved before enforcement it makes 243 plans lose their two-phase aggregation. None of today's optimizer rules moved earlier without changing plans, apart from `HashJoinBuffering`, which is off by default. The guideline I'd propose: - A rule that reads what enforcement produces goes after it. - A rule whose result enforcement reads, or that needs the plan before enforcement, goes before it. - For the rest, your heuristic decides, and earlier is better. On canonical shapes: they help a rule find rewrites, but a rule can't rely on them for correctness, because the analyzer also sees plans that are optimized again (e.g. Ballista AQE) and custom operators. I think that is the other technique for the `WindowTopN` and `join_selection` invariants: on such plans they only need to skip shapes they don't recognize. I'll follow up with the per-rule numbers and what this means for #26757. -- 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]
