zhuqi-lucas commented on PR #21976: URL: https://github.com/apache/datafusion/pull/21976#issuecomment-4468997723
> Thank you @zhuqi-lucas > > cc @ozankabak @berkaysynnada @mustafasrepo as I believe you have spent quite a lot of time in / on this code as well > > I went through the code and tests (except the new ones in enforce_distribution) carefully and I think this is a very nice code refactor and I think the PR could be merged as is. It is especially impressive that all the existing slt tests do not change. > > I think there are several improvements we should make, though given how large this PR is already is is pretty unwieldy (and it is so large it will be hard to keep re-reviewing) I recommend we get it in as soon as possible (or break it into some smaller pieces) and then iterate / cleanup as follow on PRs > > I suggest > > 1. We wait until we have cut the DataFusion 54 release to merge this one in (as I think given its size and importance, it has potential for unintended consequences) > 2. Remove dead `EnsureRequirementsNoPushdown` > 3. Re-review the tests added in ensure_distribution and remove anything that is redundant and port the rest to datafusion/core/tests/physical_optimizer/ensure_distribution.rs so the tests stay together (also maybe leave a comment in ensure_distribution.rs to point at the test locations) > 4. Plan a few follow on PRs to clean up the tests (eg the tests that currently seem to test only EnsureDistribution and EnsureSorting individually) Thank you @alamb for review, sure, i will address remaining comments and wait until we have cut the DataFusion 54 release . -- 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]
