jayzhan211 commented on PR #27191: URL: https://github.com/apache/datafusion/pull/27191#issuecomment-6755387522
> I think this one is ok, thank you for working on it @jayzhan211 > > I am still not very happy with the current state of the EquivalenceProperties and SortWalkKey (I realize you did not cause this). Specifically the fact that there are now explicit "implied" orderings in addition to equivalence properties seems like they partially duplicate significant functionality. > > This PR seems to be adding more (complicated) code to add implied ordering in another path, rather than unifying the behavior. I realize unifying the logic is too much for a bug fix. It would be really nice to try and unify all this spaghetti as a follow on, otherwise we are going to be chasing bugs down in this code indefinitely > > > I also spent a while writing up what I think SortKeyWalk actually does > - https://github.com/apache/datafusion/pull/27266 I've also found equivalence properties overly complicated, and the "walk" PR is my first step toward simplifying them. I'd like to revisit them and come up with a version that's easier to understand! -- 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]
