feat: add RightMark Join #13252

jonathanc-n · 2024-11-04T17:53:12Z

Which issue does this PR close?

Closes #13138 .

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jonathanc-n · 2024-11-04T17:55:11Z

@eejbyfeldt Just leaving it as a draft for now, if you have any pointers feel free to add on.

Dandandan · 2024-11-04T19:33:48Z

datafusion/core/src/physical_optimizer/join_selection.rs

@@ -136,6 +136,9 @@ fn swap_join_type(join_type: JoinType) -> JoinType {
        JoinType::LeftMark => {
            unreachable!("LeftMark join type does not support swapping")
        }
+        JoinType::RightMark => {
+            unreachable!("RightMark join type does not support swapping")


I think we can support it now, I think this should be relatively easy.

Yeah, supporting swap is partly why we are adding RightMark. So would be good to have that fixed in this PR.

eejbyfeldt

Left some minor comments, otherwise it looks good so far to me.

eejbyfeldt · 2024-11-04T19:52:10Z

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

+            let probe_indices = (0..prune_length)
+                .map(R::Native::from_usize)
+                .collect::<PrimitiveArray<R>>();
+            let build_indices = (0..prune_length)
+                .map(|idx| {
+                    // For mark join we output a dummy index 0 to indicate the row had a match
+                    if visited_rows.contains(&(idx + deleted_offset)) {
+                        Some(L::Native::from_usize(0).unwrap())
+                    } else {
+                        None
+                    }
+                })
+                .collect();
+            (build_indices, probe_indices)


This looks very similar to what we do or the LeftMark could we move this code into a function and call it for both?

datafusion/sql/src/unparser/plan.rs

datafusion/substrait/src/logical_plan/producer.rs

datafusion/optimizer/src/optimize_projections/mod.rs

eejbyfeldt · 2024-11-04T20:09:52Z

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

+            let left_field = once((
+                Field::new("mark", arrow_schema::DataType::Boolean, false),
+                ColumnIndex {
+                    index: 0, // 'mark' is not associated with either side
+                    side: JoinSide::None,
+                },
+            ));


Maybe this could also be moved to a function that is used for both LeftMark and RightMark.

eejbyfeldt · 2024-11-04T20:12:15Z

datafusion/proto-common/src/generated/pbjson.rs

@@ -3911,6 +3913,7 @@ impl<'de> serde::Deserialize<'de> for JoinType {
                    "RIGHTSEMI" => Ok(JoinType::Rightsemi),
                    "RIGHTANTI" => Ok(JoinType::Rightanti),
                    "LEFTMARK" => Ok(JoinType::Leftmark),
+                    "RIGHTMARK" => Ok(JoinTYpe::Rightmark),


This is causing CI failures. I belive this file should be generated by running: https://github.com/apache/datafusion/blob/main/datafusion/proto-common/regen.sh

Suggested change

"RIGHTMARK" => Ok(JoinTYpe::Rightmark),

"RIGHTMARK" => Ok(JoinType::Rightmark),

Is this included in the docs?

eejbyfeldt · 2024-11-04T20:13:50Z

datafusion/core/src/physical_optimizer/join_selection.rs

@@ -136,6 +136,9 @@ fn swap_join_type(join_type: JoinType) -> JoinType {
        JoinType::LeftMark => {
            unreachable!("LeftMark join type does not support swapping")
        }
+        JoinType::RightMark => {
+            unreachable!("RightMark join type does not support swapping")


Yeah, supporting swap is partly why we are adding RightMark. So would be good to have that fixed in this PR.

Co-authored-by: Emil Ejbyfeldt <[email protected]>

jonathanc-n · 2024-11-05T22:04:51Z

@eejbyfeldt I implemented the swapping, would be nice to see if I did that correctly.

I made a change to adjust_indices_by_join_type and combined the logic for the right join and rightmark join for indices:

 JoinType::Right | JoinType::RightMark => {
          // combine the matched and unmatched right result together
          append_right_indices(
              left_indices,
              right_indices,
              adjust_range,
              preserve_order_for_right,
          )
      }

join_right_mark test had previously not given any results and is now giving a table with values. However, it is marking all the values as true and I cannot seem to track the issue down. Would you be able to look into this?

Instead of this:

"+----+----+----+-------+",
"| a2 | b1 | c2 | mark  |",
"+----+----+----+-------+",
"| 10 | 4  | 70 | true  |",
"| 20 | 5  | 80 | true  |",
"| 30 | 6  | 90 | false |",
"+----+----+----+-------+",

it is giving this:
"+----+----+----+-------+",
"| a2 | b1 | c2 | mark  |",
"+----+----+----+-------+",
"| 10 | 4  | 70 | true  |",
"| 20 | 5  | 80 | true  |",
"| 30 | 6  | 90 | true |",
"+----+----+----+-------+",

draft right mark

a6c2e06

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate substrait common Related to common crate proto Related to proto crate labels Nov 4, 2024

jonathanc-n marked this pull request as draft November 4, 2024 17:54

Merge branch 'main' into implement-rightmark-join

beda8d7

jonathanc-n changed the title ~~feat: RightMark Join~~ feat: add RightMark Join Nov 4, 2024

Dandandan reviewed Nov 4, 2024

View reviewed changes

eejbyfeldt reviewed Nov 4, 2024

View reviewed changes

jonathanc-n and others added 4 commits November 4, 2024 15:32

Update datafusion/sql/src/unparser/plan.rs

8e64786

Co-authored-by: Emil Ejbyfeldt <[email protected]>

Update datafusion/proto-common/src/generated/pbjson.rs

8f38fb4

Co-authored-by: Emil Ejbyfeldt <[email protected]>

add swap

03d1aa1

adjusted tests

feac971

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add RightMark Join #13252

feat: add RightMark Join #13252

jonathanc-n commented Nov 4, 2024

jonathanc-n commented Nov 4, 2024

Dandandan Nov 4, 2024

eejbyfeldt Nov 4, 2024

eejbyfeldt left a comment

eejbyfeldt Nov 4, 2024

eejbyfeldt Nov 4, 2024

eejbyfeldt Nov 4, 2024

jonathanc-n Nov 5, 2024

eejbyfeldt Nov 4, 2024

jonathanc-n commented Nov 5, 2024 •

edited

Loading

	"RIGHTMARK" => Ok(JoinTYpe::Rightmark),
	"RIGHTMARK" => Ok(JoinType::Rightmark),

feat: add RightMark Join #13252

Are you sure you want to change the base?

feat: add RightMark Join #13252

Conversation

jonathanc-n commented Nov 4, 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?

jonathanc-n commented Nov 4, 2024

Dandandan Nov 4, 2024

Choose a reason for hiding this comment

eejbyfeldt Nov 4, 2024

Choose a reason for hiding this comment

eejbyfeldt left a comment

Choose a reason for hiding this comment

eejbyfeldt Nov 4, 2024

Choose a reason for hiding this comment

eejbyfeldt Nov 4, 2024

Choose a reason for hiding this comment

eejbyfeldt Nov 4, 2024

Choose a reason for hiding this comment

jonathanc-n Nov 5, 2024

Choose a reason for hiding this comment

eejbyfeldt Nov 4, 2024

Choose a reason for hiding this comment

jonathanc-n commented Nov 5, 2024 • edited Loading

jonathanc-n commented Nov 5, 2024 •

edited

Loading