feat: add NestedLoopJoin rel #188

danepitkin · 2023-10-10T23:14:12Z

Requires substrait-io/substrait#561 to be merged and released.

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java

vbarua · 2023-10-20T20:57:32Z

substrait-io/substrait#561 has been merged. After it's released over the weekend, we can update the submodule to point to it (preferably as it's own PR).

vbarua · 2023-10-23T14:53:58Z

I went ahead and updated substrait-java with the latest version of substrait, which includes the NestedLoopJoin changes.

danepitkin

Thanks for updating to the latest version of Substrait! I reviewed the HashJoin PR and tried to implement everything that is applicable for NLJ.

danepitkin · 2023-10-26T02:26:47Z

core/src/main/java/io/substrait/relation/ProtoRelConverter.java

+            .condition(
+                // defaults to true if the join expression is unassigned, resulting in a cartesian
+                // join
+                Optional.of(
+                    rel.hasExpression()
+                        ? converter.from(rel.getExpression())
+                        : Expression.BoolLiteral.builder().value(true).build()))


One thing to note for NLJ is that the spec says the Join Expression is optional, but defaults to True which becomes a cartesian join. I've added the code to default to True here.

vibhatha · 2023-10-31T05:56:07Z

core/src/test/java/io/substrait/type/proto/JoinRoundtripTest.java

+  Rel roundTrip(Rel rel) {
    io.substrait.proto.Rel protoRel = relProtoConverter.toProto(rel);
-    Rel relReturned = protoRelConverter.from(protoRel);
-    assertEquals(rel, relReturned);
+    return protoRelConverter.from(protoRel);
+  }


Looks better. 👍

Thanks! I split up roundtrip functionality from verification-of-roundtrip so I could test inequality for NLJ more easily.

core/src/test/java/io/substrait/type/proto/ExtensionRoundtripTest.java

vbarua · 2023-10-31T21:28:40Z

core/src/main/java/io/substrait/relation/physical/NestedLoopJoin.java

+@Value.Immutable
+public abstract class NestedLoopJoin extends BiRel implements HasExtension {
+
+  public abstract Optional<Expression> getCondition();


What do you think about making this

public abstract Expression getCondition();

The spec indicates that

// optional, defaults to true (a cartesian join) Expression expression = 4;

but we can be stricter within the POJO layer and require it. You're already inserting a boolean literal condition if the protobuf doesn't contain one https://github.com/substrait-io/substrait-java/pull/188/files#r1372502462

Sounds good to me! I wonder if the spec should not have made it optional here? https://substrait.io/relations/physical_relations/#nlj-operator

I can go ahead and change the spec, since I just added it recently.

Hmm, it's worth noting that JoinRel uses public abstract Optional<Expression> getCondition();, which is required according to the spec. Maybe it is best to implement NLJ similarly? I'll defer to your choice!

I made the change in the latest commit if it is the preferred choice!

The JoinRel in the protobuf spec doesn't have any information about whether condition is required or not:
https://github.com/substrait-io/substrait/blob/b3071bc9cd77cf916568641c83056a285f8123be/proto/substrait/algebra.proto#L156-L160

I'm guessing that's why it's Optional currently. I think it would make sense to make the JoinRel condition be required as well. That can be a separate change though.

vbarua · 2023-11-01T21:02:55Z

core/src/main/java/io/substrait/relation/ProtoRelConverter.java

+        NestedLoopJoin.builder()
+            .left(left)
+            .right(right)
+            .condition(converter.from(rel.getExpression()))


I think we still want your original code that checks if the condition is null and then sets the condition to True if it is. The expression field in the protobuf isn't marked as required, so we can't assume it's present when reading the protos in.

How I'm thinking about this is that this is effectively required, because when it is null it should be treated as True. But we can handle that behaviour at our serialisation boundary and have it be properly required internally, which makes it easier to work with within our code.

Thanks for the explanation! +1

vbarua · 2023-11-01T21:08:16Z

core/src/test/java/io/substrait/type/proto/JoinRoundtripTest.java

+        NestedLoopJoin.builder()
+            .from(
+                b.nestedLoopJoin(
+                    __ -> b.bool(true), NestedLoopJoin.JoinType.INNER, leftTable, rightTable))


The test in ExtensionRoundtripTest.java use a bool true condition as well. Could you use a more interesting condition here like a key equality comparison between the two tables (ie. leftTable.a = rightTable.f) so that we can have a test that verifies that the condition expressions are being converted correctly.

Good point! Will update.

vbarua

Thanks for the updates. Left some small comments.

danepitkin · 2023-11-03T15:59:01Z

I can repro the failure locally. I'm still digging into it, it seems the test case is surfacing an actual bug.

The issue is in the last param in this equal comparator output:

condition=ScalarFunctionInvocation{declaration=equal:any_any, arguments=[FieldReference{segments=[StructField{offset=0}], type=I64{nullable=false}}, FieldReference{segments=[StructField{offset=2}], type=I64{nullable=false}}]

condition=ScalarFunctionInvocation{declaration=equal:any_any, arguments=[FieldReference{segments=[StructField{offset=0}], type=I64{nullable=false}}, FieldReference{segments=[StructField{offset=2}], type=Str{nullable=false}}]

vbarua · 2023-11-03T16:17:59Z

core/src/test/java/io/substrait/type/proto/JoinRoundtripTest.java

+        NestedLoopJoin.builder()
+            .from(
+                b.nestedLoopJoin(
+                    __ -> b.equal(b.fieldReference(leftTable, 0), b.fieldReference(rightTable, 2)),


Ah! This is subtle and weird (and something to improve in the builder actually).

The condition on the join is evaluated relative to the joins record shape, which is the union of the left and right table records. It would look like:

0, 1, 2, 3, 4, 5 (a, b, c, d, e, f)

b.fieldReference(rightTable, 2) here ends up referring to c

Ah that makes sense! Thank you

I'm realising there's nothing else in the code that uses this, which is why it hasn't come up before.

For the test case, should I create a unioned named table to refer to? 5 is out of bounds when creating the Rel, but in bounds after the roundtrip

Hmm, we have newInputRelReference(int index, List<Rel> rels), which might handle this. I'll dig into this more myself.

vbarua · 2023-11-03T16:53:39Z

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java

+    return Arrays.stream(indexes)
+        .mapToObj(index -> fieldReference(inputs, index))
+        .collect(java.util.stream.Collectors.toList());
+  }


Good call on these, they will be quite helpful.

vbarua

Looks good! Thanks for adding this.

danepitkin · 2023-11-03T16:54:49Z

Thank you so much for the help!

* feat: more builder support for field references

danepitkin force-pushed the danepitkin/nestedloopjoin branch 2 times, most recently from 4aad77f to 90d7b66 Compare October 10, 2023 23:47

danepitkin marked this pull request as draft October 10, 2023 23:48

EpsilonPrime reviewed Oct 18, 2023

View reviewed changes

core/src/main/java/io/substrait/dsl/SubstraitBuilder.java Outdated Show resolved Hide resolved

danepitkin force-pushed the danepitkin/nestedloopjoin branch from 560136d to ef9c234 Compare October 25, 2023 19:46

danepitkin marked this pull request as ready for review October 26, 2023 02:23

danepitkin commented Oct 26, 2023

View reviewed changes

vbarua self-requested a review October 30, 2023 15:25

vibhatha reviewed Oct 31, 2023

View reviewed changes

vbarua reviewed Oct 31, 2023

View reviewed changes

core/src/test/java/io/substrait/type/proto/ExtensionRoundtripTest.java Outdated Show resolved Hide resolved

vbarua reviewed Oct 31, 2023

View reviewed changes

vbarua reviewed Nov 1, 2023

View reviewed changes

danepitkin added 9 commits November 3, 2023 11:07

feat: add NestedLoopJoin rel

cea494f

fix: record type for anti, semi joins

dc5c1fe

fix: move nlj operator into physical subfolder

90e62d9

feat: add test

f7cec8c

fix: improve tests

3ba7fc0

fix: improve test case further

bb82de7

fix: use camelCase

d75f2a2

feat: make expression required

53d1021

fix: use equality expression in test case

47aa9d8

danepitkin force-pushed the danepitkin/nestedloopjoin branch from fa3aa86 to 47aa9d8 Compare November 3, 2023 15:54

fix: remove debug output and fix indices

476a3e1

vbarua reviewed Nov 3, 2023

View reviewed changes

fix: use list of rels as input to equal expression

9f6c6ee

danepitkin mentioned this pull request Nov 3, 2023

[Isthmus] Prototype of physical rels in plans #196

Open

vbarua reviewed Nov 3, 2023

View reviewed changes

vbarua approved these changes Nov 3, 2023

View reviewed changes

vbarua merged commit b66d5b1 into substrait-io:main Nov 3, 2023
8 checks passed

danepitkin mentioned this pull request Nov 3, 2023

Implement MergeJoinRel #198

Closed

ajegou pushed a commit to ajegou/substrait-java that referenced this pull request Mar 29, 2024

feat: initial NestedLoopJoin support (substrait-io#188)

1dcbadf

* feat: more builder support for field references

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add NestedLoopJoin rel #188

feat: add NestedLoopJoin rel #188

danepitkin commented Oct 10, 2023

vbarua commented Oct 20, 2023

vbarua commented Oct 23, 2023

danepitkin left a comment

danepitkin Oct 26, 2023

vibhatha Oct 31, 2023

danepitkin Oct 31, 2023

vbarua Oct 31, 2023

danepitkin Nov 1, 2023

danepitkin Nov 1, 2023

danepitkin Nov 1, 2023 •

edited

Loading

danepitkin Nov 1, 2023

vbarua Nov 1, 2023

vbarua Nov 1, 2023

vbarua Nov 1, 2023

danepitkin Nov 3, 2023

vbarua Nov 1, 2023

danepitkin Nov 2, 2023

vbarua left a comment

danepitkin commented Nov 3, 2023 •

edited

Loading

vbarua Nov 3, 2023 •

edited

Loading

danepitkin Nov 3, 2023

vbarua Nov 3, 2023

danepitkin Nov 3, 2023 •

edited

Loading

danepitkin Nov 3, 2023

vbarua Nov 3, 2023

vbarua left a comment

danepitkin commented Nov 3, 2023

feat: add NestedLoopJoin rel #188

feat: add NestedLoopJoin rel #188

Conversation

danepitkin commented Oct 10, 2023

vbarua commented Oct 20, 2023

vbarua commented Oct 23, 2023

danepitkin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danepitkin Nov 1, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbarua left a comment

Choose a reason for hiding this comment

danepitkin commented Nov 3, 2023 • edited Loading

vbarua Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danepitkin Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbarua left a comment

Choose a reason for hiding this comment

danepitkin commented Nov 3, 2023

danepitkin Nov 1, 2023 •

edited

Loading

danepitkin commented Nov 3, 2023 •

edited

Loading

vbarua Nov 3, 2023 •

edited

Loading

danepitkin Nov 3, 2023 •

edited

Loading