2010YOUY01 commented on PR #25688: URL: https://github.com/apache/datafusion/pull/25688#issuecomment-5928406180
I'm wondering what concrete issue or bug this PR is trying to solve, is it to prevent similar bugs like - 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. ---- 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. 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: # Optimizer rule list ```text // Stage 1: canonicalization // Normalize away enforced distribution so later rewrites operate on a // simpler canonical form without nodes such as `RepartitionExec`. initial_physical_planning // Stage 2: optimization // Easy-to-transform zone: rules can assume there is no `RepartitionExec`, // the rewrites are easier to implement. rule1 rule2 // Stage 3: lowering // Insert `RepartitionExec` where required. Rules after this point must // account for its presence. EnsureRequirements ``` 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. 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. -- 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]
