apache / apache/datafusion

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
#3,554 0 comments 1 reaction 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.