2010YOUY01 commented on PR #25688: URL: https://github.com/apache/datafusion/pull/25688#issuecomment-6246223865
> > In #26360, I think our disagreement is whether more explicit invariants make maintenance easier. I think they do, while @alamb believes the opposite. > > In my mind, my position is more like "removing as many hidden assumptions makes maintenance easier" (as in I feel we should remove the hidden invariants rather than make them visible) I agree with this point — it's a great design principle. Moving in this direction requires careful design and review of optimizer changes. However, moving in the opposite direction can happen naturally through the development process, as we focus on solving today's problems as quickly as possible. My general feeling is that the latter force is stronger (and I'm tempted to blame AI again). As a result, the implementation seems to be accumulating more hidden invariants, with different rules undoing each other's work or defensively working around one another. > > For now, this invariant is only documented as a guideline, with automatic validation potentially added in the future. > > I think it is partially enforced by https://docs.rs/datafusion/latest/datafusion/physical_plan/trait.ExecutionPlan.html#method.check_invariants > > > If we want to enforce it, I think we need a machinery with similar complexity to #26360? If so, perhaps we could try to make it extensible to other invariant regions in the future. > > I do think adding additional invariants to the ExecutionPlans would be good I also agree that we should make better use of `check_invariants`, but it currently doesn't cover cross-rule invariants like those targeted by #26360. I don't think cross-rule invariants can be eliminated entirely. (Though this makes me wonder whether we could extend `check_invariants` to achieve what #26360 is trying to do.) Going forward, I'll try to better document these hidden cross-rule assumptions whenever I get a chance to work on related code, and see whether we can eliminate some of them. > I think given the discussion and complexity here this work deserves writing up as an EPIC -- if you agree I will write up something that tries to explain the current situation, so we can be sure we are aligned Thank you, this would be great! -- 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]
