-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Move filtered SMJ right join out of join_partial
phase
#13053
Conversation
@@ -746,21 +750,21 @@ fn last_index_for_row( | |||
// `false` - the row sent as NULL joined row | |||
fn get_corrected_filter_mask( | |||
join_type: JoinType, | |||
indices: &UInt64Array, | |||
ids: &[usize], | |||
row_indices: &UInt64Array, |
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.
renamed variables for readability
@@ -100,14 +100,13 @@ Alice 100 Alice 2 | |||
Alice 50 Alice 1 | |||
Alice 50 Alice 2 | |||
|
|||
# Uncomment when filtered RIGHT moved | |||
# right join with join filter |
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.
reenabling tests
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.
Thanks @comphead -- the only question I have is why some of the tests are removed
async fn test_right_join_1k_filtered() { | ||
JoinFuzzTestCase::new( | ||
make_staggered_batches(1000), | ||
make_staggered_batches(1000), | ||
JoinType::Right, | ||
Some(Box::new(col_lt_col_filter)), | ||
) | ||
.run_test(&[JoinTestType::NljHj], false) | ||
.run_test(&[JoinTestType::HjSmj, JoinTestType::NljHj], false) |
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.
🎉
@@ -3643,102 +3664,12 @@ mod tests { | |||
|
|||
#[tokio::test] | |||
async fn test_left_semi_join_filtered_mask() -> Result<()> { | |||
let schema = Arc::new(Schema::new(vec![ |
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 are these tests removed?
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.
they are not removed, the common test data preparation was factored out to the separate helper method
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.
Thank you @comphead
if matches!(self.join_type, JoinType::Left | JoinType::LeftSemi) { | ||
if matches!( | ||
self.join_type, | ||
JoinType::Left | JoinType::LeftSemi | JoinType::Right |
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.
Do we also have to handle https://docs.rs/datafusion/latest/datafusion/common/enum.JoinType.html#variant.RightSemi 🤔
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.
At the end of the day all types of filtered joins will be moved.
I'm planning to cover this week RightSemi, LeftAnti, RightAnti and Full, lets see how it goes
Which issue does this PR close?
Related #12359 .
Follow up on #12764
Rationale for this change
Same as #12764 but for Right Outer join
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?