Fix SMJ Left Anti Join when the join filter is set #10724

comphead · 2024-05-30T15:51:44Z

Which issue does this PR close?

Rationale for this change

Fix issues when LeftAnti Join returned nothing when the join filter was set

What changes are included in this PR?

Handled scenarios for LeftAntiJoin when join filter is set. Output the streaming row(left join side row) only if there is no corresponding filtered match from buffered data(right join side)

Are these changes tested?

Are there any user-facing changes?

alamb

Thank you @comphead -- this looks like an improvement to me. Queries now get correct answers that did not before.

I remember that Anti join semantics are quite subtle, and I am not sure I would catch subtle bugs in my review, but it seems to me that this code is an improvement to what is on main

nice work

alamb · 2024-05-31T10:37:59Z

datafusion/physical-plan/src/joins/sort_merge_join.rs

@@ -1181,7 +1189,10 @@ impl SMJStream {
            let filter_columns = if chunk.buffered_batch_idx.is_some() {
                if matches!(self.join_type, JoinType::Right) {
                    get_filter_column(&self.filter, &buffered_columns, &streamed_columns)
-                } else if matches!(self.join_type, JoinType::LeftSemi) {
+                } else if matches!(


I wonder if we have to check (in the lines above) for RightAnti and RightSemi? 🤔

I have a separate ticket for Right* joins, however I'm not sure how to build a right join on the sql level tbh. I will check it once we have Left* stabilized

alamb · 2024-05-31T10:38:29Z

datafusion/physical-plan/src/joins/sort_merge_join.rs

@@ -1397,6 +1416,9 @@ fn get_filter_column(
            .map(|i| buffered_columns[i.index].clone())
            .collect::<Vec<_>>();

+        //        dbg!(&left_columns);


this looks left over and could probably be removed

alamb · 2024-05-31T10:41:00Z

datafusion/physical-plan/src/joins/sort_merge_join.rs

+        JoinType::LeftAnti => {
+            // have we seen a filter match for a streaming index before
+            for i in 0..streamed_indices_length {
+                if mask.value(i) && !seen_as_true {


I vaguely remember there are subtle semantics with respect to NULLs and Anti joins. It looks like this doe the right thing (looks for true values in the mask). However i wonder if you also should check if mask.valid() as well first ?

This may not be necessary if mask is always non nullable. I am not sure

There was a confusion between 0 and nulls, good point, but not sure if its applicanble for booleans, I'll check it

UPD: there is a collision between nulls and default values. so for boolean null value will be fetched as false which is okay for this logic as we seeking for the true, and null index cannot be fetched as true

comphead · 2024-05-31T15:40:01Z

Thank you @comphead -- this looks like an improvement to me. Queries now get correct answers that did not before.

I remember that Anti join semantics are quite subtle, and I am not sure I would catch subtle bugs in my review, but it seems to me that this code is an improvement to what is on main

nice work

Thanks @alamb I have added the basic left anti join test cases to slt file. However once we resolve the #10659 it gonna be easier to find issues then we cannot imagine right away

alamb · 2024-05-31T21:33:09Z

Let's keep iterating on this code in subsequent PRs

viirya · 2024-05-31T22:25:57Z

datafusion/physical-plan/src/joins/sort_merge_join.rs

+        // LeftAnti semantics: return true if for every x in the collection, p(x) is false.
+        // the true(if any) flag needs to be set only once per streaming index
+        // to prevent duplicates in the output


It is unclear what p(x) is as there is no such variable. You can say "join filter" instead of a obscure term.

viirya · 2024-05-31T22:26:42Z

datafusion/physical-plan/src/joins/sort_merge_join.rs

+            Some((corrected_mask.finish(), filter_matched_indices))
+        }
+        // LeftAnti semantics: return true if for every x in the collection, p(x) is false.
+        // the true(if any) flag needs to be set only once per streaming index


"the true(if any) flag" -> "The corrected_mask values for same streaming index". We should keep the comment as direct and clear as possible.

comphead · 2024-05-31T22:28:43Z

Thanks @alamb and @viirya I'll address all of the comments in followup PR

viirya · 2024-05-31T22:33:57Z

datafusion/physical-plan/src/joins/sort_merge_join.rs

+                    || (i == streamed_indices_length - 1
+                        && *scanning_buffered_batch_idx == buffered_batches_len - 1)


The second condition is not in the above comment.

Hmm, the second condition is for the last index in streamed_indices and last buffered batch. But in last buffered batch, there are many buffered indices. It is possible we are in the last buffered batch, but we still have more buffered indices to process. Isn't this put a value into corrected_mask too early so it could be incorrect?

Hmm, the second condition is for the last index in streamed_indices and last buffered batch. But in last buffered batch, there are many buffered indices. It is possible we are in the last buffered batch, but we still have more buffered indices to process. Isn't this put a value into corrected_mask too early so it could be incorrect?

Good point, I'll check that

* Fix: Sort Merge Join crashes on TPCH Q21 * Fix LeftAnti SMJ join when the join filter is set * rm dbg

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label May 30, 2024

comphead requested review from viirya and alamb May 30, 2024 15:52

comphead changed the title ~~Fix Left Anti Join when the join filter is set~~ Fix SMJ Left Anti Join when the join filter is set May 30, 2024

comphead added 2 commits May 30, 2024 16:15

Fix: Sort Merge Join crashes on TPCH Q21

3f2daf2

Fix LeftAnti SMJ join when the join filter is set

a88a8e7

comphead force-pushed the dev0 branch from 745e24e to a88a8e7 Compare May 30, 2024 23:15

alamb approved these changes May 31, 2024

View reviewed changes

rm dbg

5660869

alamb merged commit d6ddd23 into apache:main May 31, 2024
23 checks passed

viirya reviewed May 31, 2024

View reviewed changes

comphead mentioned this pull request Jun 3, 2024

SMJ: Add more tests and improve comments #10784

Merged

findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024

Fix SMJ Left Anti Join when the join filter is set (apache#10724)

215fb4b

* Fix: Sort Merge Join crashes on TPCH Q21 * Fix LeftAnti SMJ join when the join filter is set * rm dbg

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SMJ Left Anti Join when the join filter is set #10724

Fix SMJ Left Anti Join when the join filter is set #10724

comphead commented May 30, 2024

alamb left a comment

alamb May 31, 2024

comphead May 31, 2024

alamb May 31, 2024

alamb May 31, 2024

comphead May 31, 2024 •

edited

Loading

comphead commented May 31, 2024

alamb commented May 31, 2024

viirya May 31, 2024

viirya May 31, 2024 •

edited

Loading

comphead commented May 31, 2024

viirya May 31, 2024

viirya May 31, 2024

comphead Jun 1, 2024

comphead Jun 3, 2024

		\|\| (i == streamed_indices_length - 1
		&& *scanning_buffered_batch_idx == buffered_batches_len - 1)

Fix SMJ Left Anti Join when the join filter is set #10724

Fix SMJ Left Anti Join when the join filter is set #10724

Conversation

comphead commented May 30, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead May 31, 2024 • edited Loading

Choose a reason for hiding this comment

comphead commented May 31, 2024

alamb commented May 31, 2024

Choose a reason for hiding this comment

viirya May 31, 2024 • edited Loading

Choose a reason for hiding this comment

comphead commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead May 31, 2024 •

edited

Loading

viirya May 31, 2024 •

edited

Loading