[SPARK-44060][SQL] Code-gen for build side outer shuffled hash join by szehon-ho · Pull Request #41614 · apache/spark

szehon-ho · 2023-06-15T16:09:19Z

What changes were proposed in this pull request?

Codegen of shuffled hash join of build side outer join (ie, left outer join build left or right outer join build right)

Why are the changes needed?

The implementation of #41398 was only for non-codegen version, and codegen was disabled in this scenario.

Does this PR introduce any user-facing change?

No

How was this patch tested?

New unit test in WholeStageCodegenSuite

szehon-ho · 2023-06-15T16:10:59Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

I wasn't entirely sure if this is the right approach, but the existing flag was a bit explicit in FULL OUTER JOIN, and in the discussion like #41398 (comment) build-side outer join is taken to mean, if only one side is OUTER.

szehon-ho · 2023-06-15T16:11:55Z

sql/core/src/test/scala/org/apache/spark/sql/JoinSuite.scala

Note: this is not strictly necessary, but wanted to keep coverage of the non-codegen codepath.

However, it is true that the "SPARK-32399: Full outer shuffled hash join" test does not do this, iiuc leaving the non-codegen codepath actually untested for full-outer-join case.

+1 for keeping the test coverage.

Shall we test both the codegen and non-codegen codepath? This test seems to have more coverage than the test in WholeStageCodegenSuite.

OK let me make do both then

@c21 Shall we have a follow-up to also test non-codegen codepath in "SPARK-32399: Full outer shuffled hash join" test?

Done! I think that'd be valuable, I could add that too , to the test above, in a follow up pr

dongjoon-hyun

Thank you for making a PR again, @szehon-ho .

Also, cc @viirya , @huaxingao , @sunchao , @cloud-fan , too

sql/core/src/test/scala/org/apache/spark/sql/execution/WholeStageCodegenSuite.scala

huaxingao · 2023-06-16T21:59:13Z

+1, LGTM from my side. Thanks for working on this! @szehon-ho
Let's wait a couple of days for others to review.

sql/core/src/main/scala/org/apache/spark/sql/execution/joins/ShuffledHashJoinExec.scala

sql/core/src/test/scala/org/apache/spark/sql/JoinSuite.scala

sql/core/src/main/scala/org/apache/spark/sql/execution/joins/ShuffledHashJoinExec.scala

viirya · 2023-06-22T21:02:57Z

sql/core/src/main/scala/org/apache/spark/sql/execution/joins/ShuffledHashJoinExec.scala

Seems isFullOuterJoin could be pull to outside if?

if (!$foundMatch && $isFullOuterJoin) { $buildRow = null; $consumeFullOuterJoinRow(); }

I was afraid of side effect if I don't set buildRow to null, if not fullOuterJoin. Let me check.

I wanted to leave buildRow=null in both case if !foundMatch. I was thinking its cleaner, to clear the state, and more in line with uniqueKey side. Let me know what you think.

viirya

Looks okay but some minor comments.

szehon-ho · 2023-06-27T05:32:33Z

rebase to try to fix test error

szehon-ho · 2023-06-27T23:27:28Z

@viirya if you have some time could you take another look? Replied but didnt resolve to the last comment, let me know your thoughts. Thanks!

viirya · 2023-06-27T23:50:25Z

Thanks @szehon-ho for addressing the comments. I will take another look.

viirya · 2023-06-28T05:41:05Z

sql/core/src/main/scala/org/apache/spark/sql/execution/joins/ShuffledHashJoinExec.scala

nit: consumeOuterJoinRow.

### What changes were proposed in this pull request? Codegen of shuffled hash join of build side outer join (ie, left outer join build left or right outer join build right) ### Why are the changes needed? The implementation of apache#41398 was only for non-codegen version, and codegen was disabled in this scenario. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New unit test in WholeStageCodegenSuite

szehon-ho · 2023-06-30T17:33:54Z

Trying again, test failure didn't look very related though not entirely sure.

2023-06-30T06:10:24.5434667Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m- error notifier test *** FAILED *** (1 minute)�[0m�[0m
2023-06-30T06:10:24.5436553Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  java.nio.file.NoSuchFileException: /home/runner/work/spark/spark/target/tmp/spark-cc3d10d8-5d0b-4592-9971-74bba230b8a8�[0m�[0m
2023-06-30T06:10:24.5438354Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  at sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)�[0m�[0m
2023-06-30T06:10:24.5439946Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)�[0m�[0m
2023-06-30T06:10:24.5441984Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)�[0m�[0m
...
2023-06-30T06:41:10.1873780Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m*** 1 TEST FAILED ***�[0m�[0m
2023-06-30T06:41:10.2104588Z �[0m[�[0m�[31merror�[0m] �[0m�[0mFailed: Total 9312, Failed 1, Errors 0, Passed 9311, Ignored 27�[0m
2023-06-30T06:41:10.2332475Z �[0m[�[0m�[31merror�[0m] �[0m�[0mFailed tests:�[0m
2023-06-30T06:41:10.2334292Z �[0m[�[0m�[31merror�[0m] �[0m�[0m	org.apache.spark.sql.execution.streaming.MicroBatchExecutionSuite�[0m

viirya · 2023-06-30T17:42:11Z

Seems irrelevant.

huaxingao · 2023-07-01T05:05:15Z

Merged to master! Thanks @szehon-ho @viirya @dongjoon-hyun

cloud-fan · 2023-07-06T21:02:06Z

sql/core/src/main/scala/org/apache/spark/sql/execution/joins/ShuffledHashJoinExec.scala

         |  }
         |
-         |  $consumeFullOuterJoinRow();
+         |  if ($foundMatch || $isFullOuterJoin) {


nit: we can statically remove or simply the condition:

val consumeCode = if (isFullOuterJoin) { s"$consumeOuterJoinRow();" } else { s""" |if ($foundMatch) { | $consumeOuterJoinRow(); |} """ }

Thanks , let me make some follow ups a bit later

cloud-fan · 2023-07-06T21:02:43Z

late LGTM

github-actions bot added the SQL label Jun 15, 2023

szehon-ho commented Jun 15, 2023

View reviewed changes

dongjoon-hyun reviewed Jun 15, 2023

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/WholeStageCodegenSuite.scala Outdated Show resolved Hide resolved

viirya reviewed Jun 22, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/joins/ShuffledHashJoinExec.scala Outdated Show resolved Hide resolved

sql/core/src/test/scala/org/apache/spark/sql/JoinSuite.scala Outdated Show resolved Hide resolved

viirya reviewed Jun 22, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/joins/ShuffledHashJoinExec.scala Outdated Show resolved Hide resolved

viirya reviewed Jun 22, 2023

View reviewed changes

szehon-ho force-pushed the same_side_outer_join_codegen_master branch from cee52fe to 4213175 Compare June 27, 2023 05:32

viirya reviewed Jun 28, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/joins/ShuffledHashJoinExec.scala Outdated

Copy link

Member

viirya Jun 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: consumeOuterJoinRow.

viirya approved these changes Jun 28, 2023

View reviewed changes

szehon-ho added 8 commits June 30, 2023 10:32

Small fixups

769266b

Add explicit config in WholeStageCodegenSuite new test

efbae53

Test both codegen and non-codegen in JoinSuite

2162131

Review comments

221c1a3

Fix error

f0eeca2

remove unnecessary change

0adbfc6

consumeFullOuterJoinRow => consumeOuterJoinRow

f9dccff

szehon-ho force-pushed the same_side_outer_join_codegen_master branch from 4a060da to f9dccff Compare June 30, 2023 17:32

huaxingao approved these changes Jul 1, 2023

View reviewed changes

huaxingao closed this in 2db8cfb Jul 1, 2023

cloud-fan reviewed Jul 6, 2023

View reviewed changes

Comments

Conversation

szehon-ho commented Jun 15, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szehon-ho Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

huaxingao commented Jun 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szehon-ho Jun 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

szehon-ho commented Jun 27, 2023

Uh oh!

szehon-ho commented Jun 27, 2023

Uh oh!

viirya commented Jun 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szehon-ho commented Jun 30, 2023

Uh oh!

viirya commented Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huaxingao commented Jul 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jul 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

szehon-ho Jun 15, 2023 •

edited

Loading

szehon-ho Jun 24, 2023 •

edited

Loading

viirya commented Jun 30, 2023 •

edited

Loading