Skip to content

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

@DhamoPS

Description

@DhamoPS

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 #3482 (review)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions