feat: initial HashJoinRel support #187

vibhatha · 2023-10-04T16:13:15Z

This PR includes the base work for integrating HashEquiJoin.

danepitkin

Nice work!

CHANGELOG.md

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

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

EpsilonPrime · 2023-10-18T21:21:58Z

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

@@ -174,6 +175,21 @@ void join() {
    verifyRoundTrip(rel);
  }

+  @Test
+  void hashJoin() {
+    int[] left_keys = {};


Should we have some tests with non-empty keys to make sure that's working correctly?

+1 to using non-empty keys.

@vbarua @EpsilonPrime
Looking into the commonTable initialization, we do have empty lists to create it, so if we use non-empty lists for keys, we may need to introduce a commonTable with non-empty initialization? Am I following this incorrectly?

final Rel commonTable = b.namedScan(Collections.emptyList(), Collections.emptyList(), Collections.emptyList());

we may need to introduce a commonTable with non-empty initialization?

This is correct. You can use the b.namedScan method to create a table with columns, something like

b.namedScan(List.of("example"), List.of("a"), List.of(R.STRING)

One small structural thing. The tests in ExtensionRoundtripTest.java verify that extensions roundtrip. The test that you've added already does this perfectly well.

For adding a test using non-empty keys, could I ask that you introduce something like JoinRountripTest.java (in a similar vein to AggregateRoundtripTest.java) and do it there. That will also be a good place to add nested join tests later.

@vbarua I will defintely add a test class as you have susggested here. 👍

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

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

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

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

vbarua

Overall this looks really good. Left some comments.

vibhatha · 2023-10-19T01:00:02Z

@vbarua Thank you for the review. I will address the comments.

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

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

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

vibhatha · 2023-10-20T03:16:47Z

@vbarua ready for another round of review and left a few questions too 🙂

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

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

vbarua

Overall change looks good. Left some final comments. I think we can get this in next week 🙂

vibhatha · 2023-10-21T00:44:23Z

@vbarua I will make these changes and thank you for the quick response 🙏

vibhatha · 2023-10-25T01:43:23Z

@vbarua addressed the reviews 🙂

vbarua

LGMT! Thanks for contribution 🙇

vibhatha · 2023-10-25T15:02:36Z

Appreciate your support 🙏

* fix: pojo to proto JoinRel conversion skipped postJoinFilter

danepitkin reviewed Oct 10, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

core/src/main/java/io/substrait/relation/AbstractJoin.java Outdated Show resolved Hide resolved

danepitkin reviewed Oct 10, 2023

View reviewed changes

core/src/main/java/io/substrait/relation/physical/HashJoin.java Outdated Show resolved Hide resolved

vibhatha requested review from EpsilonPrime, jacques-n, vbarua and westonpace October 11, 2023 01:12

vibhatha marked this pull request as ready for review October 11, 2023 02:09

vibhatha requested a review from danepitkin October 11, 2023 02:09

EpsilonPrime reviewed Oct 18, 2023

View reviewed changes

vbarua reviewed Oct 18, 2023

View reviewed changes

core/src/main/java/io/substrait/relation/physical/HashJoin.java Outdated Show resolved Hide resolved

vbarua reviewed Oct 18, 2023

View reviewed changes

core/src/main/java/io/substrait/relation/RelProtoConverter.java Show resolved Hide resolved

vbarua reviewed Oct 18, 2023

View reviewed changes

core/src/main/java/io/substrait/relation/RelCopyOnWriteVisitor.java Show resolved Hide resolved

vbarua reviewed Oct 18, 2023

View reviewed changes

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

vbarua requested changes Oct 18, 2023

View reviewed changes

vibhatha commented Oct 19, 2023

View reviewed changes

core/src/main/java/io/substrait/relation/RelProtoConverter.java Outdated Show resolved Hide resolved

vibhatha requested review from EpsilonPrime and vbarua October 19, 2023 03:48

vbarua reviewed Oct 19, 2023

View reviewed changes

core/src/main/java/io/substrait/relation/ProtoRelConverter.java Outdated Show resolved Hide resolved

vbarua reviewed Oct 19, 2023

View reviewed changes

core/src/main/java/io/substrait/relation/RelCopyOnWriteVisitor.java Show resolved Hide resolved

vibhatha requested a review from vbarua October 20, 2023 03:16

vbarua reviewed Oct 20, 2023

View reviewed changes

core/src/main/java/io/substrait/relation/ProtoRelConverter.java Outdated Show resolved Hide resolved

vbarua reviewed Oct 20, 2023

View reviewed changes

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

vbarua reviewed Oct 20, 2023

View reviewed changes

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

vbarua reviewed Oct 20, 2023

View reviewed changes

vibhatha added 3 commits October 25, 2023 06:28

feat: initial commit

0b4ba3b

feat: initial feature v1

985b102

fix: formatting

aa3e390

vibhatha added 9 commits October 25, 2023 06:28

feat: include protorel conerter

1695370

fix: remove abstract join interface

00c8c74

fix: unwanted line

37e775f

fix: adding anti and semi conditions

5f10b13

feat: adding test cases

9064d3e

fix: addressing reviews v1

4162cc2

fix: getting rid of loop and using streaming instead

5154990

fix: address review v2

4c6b42c

fix: addressing reviews v3

aeaa0f2

vibhatha force-pushed the feat-hashjoin branch from 9832689 to aeaa0f2 Compare October 25, 2023 00:59

vibhatha requested a review from vbarua October 25, 2023 01:42

vbarua approved these changes Oct 25, 2023

View reviewed changes

vbarua changed the title ~~Adding HashJoin~~ feat: initial HashJoinRel support Oct 25, 2023

vbarua merged commit 46d03ca into substrait-io:main Oct 25, 2023
8 checks passed

This was referenced Nov 3, 2023

[Isthmus] Produce HashJoinRel #197

Closed

Implement MergeJoinRel #198

Closed

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

feat: initial HashJoinRel support (substrait-io#187)

d6328a6

* fix: pojo to proto JoinRel conversion skipped postJoinFilter

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial HashJoinRel support #187

feat: initial HashJoinRel support #187

vibhatha commented Oct 4, 2023 •

edited

Loading

danepitkin left a comment

EpsilonPrime Oct 18, 2023

vbarua Oct 18, 2023

vibhatha Oct 19, 2023

vbarua Oct 19, 2023

vibhatha Oct 20, 2023

vbarua left a comment

vibhatha commented Oct 19, 2023

vibhatha commented Oct 20, 2023

vbarua left a comment

vibhatha commented Oct 21, 2023

vibhatha commented Oct 25, 2023

vbarua left a comment

vibhatha commented Oct 25, 2023

feat: initial HashJoinRel support #187

feat: initial HashJoinRel support #187

Conversation

vibhatha commented Oct 4, 2023 • edited Loading

danepitkin left a comment

Choose a reason for hiding this comment

EpsilonPrime Oct 18, 2023

Choose a reason for hiding this comment

vbarua Oct 18, 2023

Choose a reason for hiding this comment

vibhatha Oct 19, 2023

Choose a reason for hiding this comment

vbarua Oct 19, 2023

Choose a reason for hiding this comment

vibhatha Oct 20, 2023

Choose a reason for hiding this comment

vbarua left a comment

Choose a reason for hiding this comment

vibhatha commented Oct 19, 2023

vibhatha commented Oct 20, 2023

vbarua left a comment

Choose a reason for hiding this comment

vibhatha commented Oct 21, 2023

vibhatha commented Oct 25, 2023

vbarua left a comment

Choose a reason for hiding this comment

vibhatha commented Oct 25, 2023

vibhatha commented Oct 4, 2023 •

edited

Loading