Clean-up the planner logic for moving join predicates from filters in sql/planner.rs which is moved to optimizer rule reduce_cross_join.
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 344
Description
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_
Contributor guide
Research direction
Start by comparing the planner logic in sql/planner.rs with the optimizer rule reduce_cross_join and the RewriteDisjunctivePredicate rewrite, using PR #3482 and its review as context. Determine which overlapping areas should be consolidated, then verify the result with tests covering more complicated OR predicates and queries involving more than two tables.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- databases
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 30/100