-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: Finalize support for RightMark
join + Mark
join swap
#16488
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
Conversation
RightMark
joinRightMark
join + Mark
join swap
If you have the time, are you able to take a look? Should be a straightforward review, thanks! @comphead @Dandandan |
Thanks @jonathanc-n its on my list! |
02)--FilterExec: t1_id@0 > 40 OR NOT mark@3, projection=[t1_id@0, t1_name@1, t1_int@2] | ||
03)----CoalesceBatchesExec: target_batch_size=2 | ||
04)------HashJoinExec: mode=CollectLeft, join_type=LeftMark, on=[(t1_id@0, t2_id@0)] | ||
04)------HashJoinExec: mode=CollectLeft, join_type=RightMark, on=[(t2_id@0, t1_id@0)] |
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.
why this suddenly changed?
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.
Swapping is now supported, so it was optimized to swap in the physical plan based on the table sizes
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.
lgtm thanks @jonathanc-n this makes to me
The extended tests started failing after this PR was merged: |
apache#16488)" This reverts commit d73f0e8.
Which issue does this PR close?
Rationale for this change
We need to support Sort Merge Join, Symmetric Hash Join and swapping join sides for Mark Joins.
What changes are included in this PR?
Support SMJ + SHJ
SortMergeJoin
andSymmetricHashJoin
Added Support for Swap
Are these changes tested?
Yes added unit tests, and sqllogictests shows that swapping support works