-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support semi join #470
Support semi join #470
Conversation
Codecov Report
@@ Coverage Diff @@
## master #470 +/- ##
==========================================
+ Coverage 75.30% 75.80% +0.50%
==========================================
Files 152 153 +1
Lines 25275 25924 +649
==========================================
+ Hits 19033 19652 +619
- Misses 6242 6272 +30
Continue to review full report at Codecov.
|
I plan to review this tomorrow |
Related PR for anti join here: #482 |
should the readme be updated to include semi join as well? |
I think for now we don't - I would say that at least need to have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me @Dandandan ! Nice work
@@ -187,6 +187,7 @@ impl OptimizerRule for HashBuildProbeOrder { | |||
| LogicalPlan::CreateExternalTable { .. } | |||
| LogicalPlan::Explain { .. } | |||
| LogicalPlan::Union { .. } | |||
| LogicalPlan::Join { .. } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should you also possibly conditionalize this on being a SemiJoin? like
| LogicalPlan::Join { .. } | |
| LogicalPlan::Join if *join_type == JoinType::Semi { .. } |
So if we add a new join type in the future it doesn't accidentally get allowed / disallowed in some confusing way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried that in earlier commit... But than the exhaustiveness checking gets in the way unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only way the compiler can do it now is by explicitly matching on all the join types, but as this feature is not added https://github.com/rust-lang/rfcs/blob/master/text/2535-or-patterns.md that would be quite verbose currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rewrote it a bit to use a function in the body of the join match
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@@ -106,6 +106,13 @@ fn should_swap_join_order(left: &LogicalPlan, right: &LogicalPlan) -> bool { | |||
} | |||
} | |||
|
|||
fn supports_swap(join_type: JoinType) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Which issue does this PR close?
Closes #461
Rationale for this change
Adds semi join support. This will enable us to rewrite
WHERE id IN (..)
andWHERE EXISTS
to a (semi) join.What changes are included in this PR?
Add semi join to hash join implementation.
What's not in this PR
Are there any user-facing changes?
No