jayzhan211 commented on issue #26481: URL: https://github.com/apache/datafusion/issues/26481#issuecomment-6379705061
@2010YOUY01 > I think this is what you mean by splitting the rule? Yes. The insert-only core becomes its own walk, used at the end of the analyzer and by any optimizer rule that needs to repair its output. The performance passes stay in one rule, since distribution and sorting are decided together (#21973), but each can be its own walk. > So the future direction I think is to establish the contract for analyzer Agreed, and I'd drop step 5 as I wrote it. Moving every performance rule after enforcement makes those rules match through repartitions and sorts and repair their output, without making anything more correct, since the core already runs after them. I'd place each rule by one question: does it need to see the repartitions and sorts that enforcement adds? If not, it runs in the analyzer before the core. If it does (for example `LimitPushdown`, `TopKRepartition`, `PushdownSort`), it runs in the optimizer after the core and keeps the plan valid. The analyzer contract could then be: - A rule must keep the query's results unchanged. It does not have to keep distribution and ordering requirements satisfied; the core at the end of the analyzer restores them. - A rule must still be correct when the plan already contains repartitions, sorts, or nodes it doesn't recognize. That happens whenever a plan is optimized again, for example by Ballista's adaptive planner after each stage, or by a fixed-point loop (#25572). The rule may skip its optimization there, but it may only decide what is safe around a node from what that node declares. Several rules get this wrong for custom nodes today: a custom partition merge under `LIMIT 10` returns 40 rows from a plan that passes every check. #26697 tracks that part. The final `ORDER BY` check stays here (step 4, #26461). With this, my earlier point about where `EliminateConstantJoin` goes in #25688 no longer applies: it can stay in the analyzer. > a better way for them is to collect them into a new crate Makes sense to me. It can be decided separately from this EPIC. -- 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]
