2010YOUY01 commented on PR #26360: URL: https://github.com/apache/datafusion/pull/26360#issuecomment-6228067530
Thanks for sharing your perspective, @alamb! I agree we should be more selective about adding invariant regions. `EnsureRequirements` is an obviously good candidate. > what I don't like about this PR is that I think it makes the optimizer more complicated and commits us to non trivial invariants / rules going forward rather than driving towards something simpler. I'm not convinced that more explicit invariants necessarily mean more complexity. However, since I have less experience working with optimizers, I'll trust your judgment here and look for more problem-specific, compelling invariant regions to add in the future. My current intuition is that more explicit constraints can actually reduce uncertainty and make extending the optimizer easier: - **Without invariant regions:** When adding a new rule, it's hard to know where to insert it safely, since the existing assumptions between rules are mostly implicit or, at best, documented in comments. - **With invariant regions:** The explicit constraints narrow down the possible insertion points to fewer, safer candidate slots, and automatic checks can catch violations of those assumptions. When we find some invariants getting in the way, we can remove them relatively cheaply. Making them explicit also encourages reviewers to think more carefully about the design and existing assumptions. ### Next step I'll continue the discussion in https://github.com/apache/datafusion/pull/25688. Generally, I think it's better to enforce the `EnsureRequirements` invariant rather than just document it as part of an analyzer/optimizer split. However, enforcing it seems to require machinery of similar complexity to this PR. If we're going to introduce that machinery, perhaps we should make it extensible to other invariants as well, and see if we can add a few major, less controversial invariant fences in the future. -- 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]
