datafusion icon indicating copy to clipboard operation
datafusion copied to clipboard

Clean-up the planner logic for moving join predicates from filters in sql/planner.rs which is moved to optimizer rule reduce_cross_join.

Open DhamoPS opened this issue 3 years ago • 0 comments

PR#3482 Thank you @DhamoPS -- I reviewed the tests carefully and they look very good 👌 I think if we add a few more showing this PR works with some more complicated OR predicates and with more than 2 tables it would be ready to merge.

Thank you @avantgardnerio for your thorough review

I went over the code a bit, and it also looks quite reasonable. I left a few comments but they are all stylistic.

I can't help feeling after this there are three somewhat redundant overlapping areas of the code

  1. This one
  2. the planner logic identified by @xudong963 in https://github.com/apache/arrow-datafusion/blob/master/datafusion/sql/src/planner.rs#L630-L681
  3. The rewrite in RewriteDisjunctivePredicate

Maybe we can work as a follow on to consolidate / remove the redundancy.

All in all very nice work 👍

Originally posted by @alamb in https://github.com/apache/arrow-datafusion/pull/3482#pullrequestreview-1109803424

DhamoPS avatar Sep 20 '22 20:09 DhamoPS