alamb commented on PR #26360: URL: https://github.com/apache/datafusion/pull/26360#issuecomment-6233098404
> 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. To be clear, I don't think the problem is with explicit invariants -- the problem is with some of the invariants themselves (e.g. the assumption that there are no embedded projections). I would rather remove as many of the invariants as possible rather than try and make them explicit > * **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. Yes, this is a real problem -- but I think a better solution is to remove the implicit assumptions rather make them explicit > Generally, I think it's better to enforce the `EnsureRequirements` invariant rather than just document it as part of an analyzer/optimizer split. I think it is basically enforced via https://docs.rs/datafusion/latest/datafusion/physical_plan/trait.ExecutionPlan.html#method.check_invariants / https://docs.rs/datafusion-physical-optimizer/55.2.0/datafusion_physical_optimizer/sanity_checker/struct.SanityCheckPlan.html As well as each pass here https://github.com/apache/datafusion/blob/b6f6990ed5f926fd04fe04c46410f8c6ddfb7674/datafusion/core/src/physical_planner.rs#L3377-L3379 -- 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]
