[GLUTEN-3432][VL] Add support for BroadcastNestedLoopJoinExec #4565

Surbhi-Vijay · 2024-01-29T09:49:54Z

What changes were proposed in this pull request?

Added support for BroadcastNestedLoopJoinTransformer

Supported join types => Inner, Cross, LeftOuter with BuildRight, RightOuter with BuildLeft

(Fixes: #3432)

How was this patch tested?

Tests in GlutenInnerJoin, GlutenOuterJoin and Various other tests already covers it.

Follow Up:

Update documentation

github-actions · 2024-01-29T09:50:12Z

#3432

github-actions · 2024-01-29T09:50:26Z

Run Gluten Clickhouse CI

github-actions · 2024-01-29T13:35:34Z

Run Gluten Clickhouse CI

zhztheplayer

Thanks @Surbhi-Vijay for working on this.

And broadcast's validation is now tricky. I'd suggest doing the work based on this ongoing patch.

Surbhi-Vijay · 2024-01-30T05:06:06Z

And broadcast's validation is now tricky. I'd suggest doing the work based on this ongoing patch.

Thanks @zhztheplayer for mentioning this. I will review this patch and change the current PR accordingly.

zhztheplayer · 2024-02-05T03:48:02Z

@Surbhi-Vijay #4544 was merged. Do you have a plan to continue this work? Thanks.

Surbhi-Vijay · 2024-02-05T06:28:50Z

@Surbhi-Vijay #4544 was merged. Do you have a plan to continue this work? Thanks.

Yes @zhztheplayer, I am working on it and will publish it soon in the current week.

github-actions · 2024-02-06T14:13:51Z

Run Gluten Clickhouse CI

Surbhi-Vijay · 2024-02-06T15:32:53Z

backends-velox/src/main/scala/org/apache/spark/sql/execution/BroadcastUtils.scala

+           */
+          while (rowIterator.hasNext) {
+            val unsafeRow = rowIterator.next().asInstanceOf[UnsafeRow]
+            rowArray.append(unsafeRow.copy())


@zhztheplayer can you please review this change and suggest if there is any better way to resolve it?

Array[InternalRow] requires all rows to be materialized so I think it's fine to use .copy() in the initial version of this feature. We can still find a way to optimize the code later.

And thank you for adding the detailed explanation in comment :)

github-actions · 2024-02-06T15:48:21Z

Run Gluten Clickhouse CI

github-actions · 2024-02-06T15:49:42Z

Run Gluten Clickhouse CI

github-actions · 2024-02-06T17:28:59Z

Run Gluten Clickhouse CI

github-actions · 2024-02-07T13:24:47Z

Run Gluten Clickhouse CI

github-actions · 2024-02-07T13:30:57Z

Run Gluten Clickhouse CI

github-actions · 2024-02-07T13:37:46Z

Run Gluten Clickhouse CI

Surbhi-Vijay · 2024-02-08T13:49:16Z

I will fix the testcases and publish again.

github-actions · 2024-02-09T10:45:25Z

Run Gluten Clickhouse CI

github-actions · 2024-02-12T16:32:14Z

Run Gluten Clickhouse CI

github-actions · 2024-02-13T10:43:06Z

Run Gluten Clickhouse CI

github-actions · 2024-02-13T11:57:17Z

Run Gluten Clickhouse CI

github-actions · 2024-02-13T12:00:59Z

Run Gluten Clickhouse CI

github-actions · 2024-02-19T04:58:09Z

Run Gluten Clickhouse CI

backends-velox/src/main/scala/io/glutenproject/backendsapi/velox/SparkPlanExecApiImpl.scala

...-core/src/main/scala/io/glutenproject/execution/BroadcastNestedLoopJoinExecTransformer.scala

zhztheplayer · 2024-02-20T03:37:17Z

...t/spark32/src/test/scala/org/apache/spark/sql/execution/joins/GlutenBroadcastJoinSuite.scala

+        assertJoinBuildSide("SELECT /*+ MAPJOIN(t1, t2) */ * FROM t1 JOIN t2", blt, BuildLeft)
        // FULL JOIN && t1Size < t2Size => BuildLeft
        assertJoinBuildSide("SELECT /*+ MAPJOIN(t1, t2) */ * FROM t1 FULL JOIN t2", bl, BuildLeft)
        // FULL OUTER && t1Size < t2Size => BuildLeft
        assertJoinBuildSide("SELECT * FROM t1 FULL OUTER JOIN t2", bl, BuildLeft)
        // LEFT JOIN => BuildRight
-        assertJoinBuildSide("SELECT /*+ MAPJOIN(t1, t2) */ * FROM t1 LEFT JOIN t2", bl, BuildRight)
+        assertJoinBuildSide("SELECT /*+ MAPJOIN(t1, t2) */ * FROM t1 LEFT JOIN t2", blt, BuildRight)
        // RIGHT JOIN => BuildLeft
-        assertJoinBuildSide("SELECT /*+ MAPJOIN(t1, t2) */ * FROM t1 RIGHT JOIN t2", bl, BuildLeft)
+        assertJoinBuildSide("SELECT /*+ MAPJOIN(t1, t2) */ * FROM t1 RIGHT JOIN t2", blt, BuildLeft)

        /* #### test with broadcast hint #### */
        // INNER JOIN && broadcast(t1) => BuildLeft
-        assertJoinBuildSide("SELECT /*+ MAPJOIN(t1) */ * FROM t1 JOIN t2", bl, BuildLeft)
+        assertJoinBuildSide("SELECT /*+ MAPJOIN(t1) */ * FROM t1 JOIN t2", blt, BuildLeft)
        // INNER JOIN && broadcast(t2) => BuildRight
-        assertJoinBuildSide("SELECT /*+ MAPJOIN(t2) */ * FROM t1 JOIN t2", bl, BuildRight)
+        assertJoinBuildSide("SELECT /*+ MAPJOIN(t2) */ * FROM t1 JOIN t2", blt, BuildRight)


Do the statements check on physical plan only? Do we need test with result-checking to cover the added code?

I see we had enabled Gluten...JoinSuites but I am not sure if the test cases work for Gluten's BNLJ either. Would you like to help check that? Thanks.

GlutenInnerJoinSuite and GlutenOuterJoinSuite are enabled in the Gluten. Both of these suites check the answer of test cases. In these test cases, explicit joins are selected and since now we have added BNLJTransformer, BNLJ is getting transformed into Gluten for inner, leftOuter with BuildRight and RightOuter with BuildLeft joins.

Here in GlutenBroadcastJoinSuite, it is not checking the answer and only checking physical plan. This test case is a rewrite of spark test case in Gluten and I had to make changes in to get it passed in pipeline.

please let me know if I should add more test cases.

The few testcases I can think of is to add fallback test cases for other remaining join types i.e. FullOuter, LeftOuter with BuildLeft & RightOuter with BuildRight.

Thanks for your explanation. If GlutenBroadcastJoinSuite mainly checks against plan then we don't have to add answer-checking code in this suite. I was just worried whether we actually have at least some code to verify the correctness of the patch's code. If GlutenInnerJoinSuite GlutenOuterJoinSuite do work for BNLJ transformer, then it's fine enough to me.

The few testcases I can think of is to add fallback test cases for other remaining join types i.e. FullOuter, LeftOuter with BuildLeft & RightOuter with BuildRight.

That's great idea. You can decide whether to put code in this PR or in a new one. Thanks.

@zhztheplayer I will create a follow up PR covering below:

Updating documentation

Adding testcases for fallback scenarios

Renaming broadcast API methods to generic names for readability.

Surbhi-Vijay · 2024-02-20T10:08:38Z

...-core/src/main/scala/io/glutenproject/execution/BroadcastNestedLoopJoinExecTransformer.scala

+    val broadcastRDD = {
+      val executionId = sparkContext.getLocalProperty(SQLExecution.EXECUTION_ID_KEY)
+      BackendsApiManager.getBroadcastApiInstance
+        .collectExecutionBroadcastHashTableId(executionId, buildTableId)


@zhztheplayer can I rename methods under BroadcastApi to make it more generic? Since now this is being used for both the modes i.e. hashedBroadcastMode & IdentityBroadcastMode. These method names containing Hashtable might confuse other developers who may read this file later.

Rename collectExecutionBroadcastHashTableId => collectExecutionBroadcastTableId
Rename cleanExecutionBroadcastHashtable=> cleanExecutionBroadcastTable

Rename collectExecutionBroadcastHashTableId => collectExecutionBroadcastTableId
Rename cleanExecutionBroadcastHashtable=> cleanExecutionBroadcastTable

Sounds fair to me. Thanks.

…BNLJTransformer

github-actions · 2024-02-20T15:48:27Z

Run Gluten Clickhouse CI

github-actions · 2024-02-20T16:06:35Z

Run Gluten Clickhouse CI

…BNLJTransformer

github-actions · 2024-02-21T04:40:01Z

Run Gluten Clickhouse CI

Surbhi-Vijay · 2024-02-21T06:24:14Z

@zhztheplayer @zhouyuan I have rebased this PR again, can you please review and provide feedback if there are any?

I will create follow up PR for remaining small task items like documentation, renaming broadcast api common methods for clarity etc.

zhztheplayer

LGTM

Surbhi-Vijay · 2024-02-22T04:32:53Z

@zhouyuan @zhztheplayer Can we please merge this PR?

GlutenPerfBot · 2024-02-22T05:27:40Z

===== Performance report for TPCH SF2000 with Velox backend, for reference only ====

query	log/native_4565_time.csv	log/native_master_02_22_2024_996ff4c89_time.csv	difference	percentage
q1	31.95	32.51	0.564	101.77%
q2	26.39	24.28	-2.118	91.97%
q3	37.71	38.12	0.413	101.10%
q4	39.04	38.44	-0.606	98.45%
q5	71.21	71.06	-0.153	99.79%
q6	5.50	5.36	-0.145	97.37%
q7	83.91	85.47	1.557	101.86%
q8	87.45	88.36	0.913	101.04%
q9	124.37	123.63	-0.741	99.40%
q10	43.09	44.24	1.156	102.68%
q11	20.63	21.09	0.465	102.26%
q12	27.53	26.06	-1.467	94.67%
q13	45.27	46.65	1.379	103.05%
q14	20.99	15.31	-5.688	72.90%
q15	29.96	31.22	1.259	104.20%
q16	14.51	13.80	-0.706	95.14%
q17	99.79	104.44	4.643	104.65%
q18	147.94	149.48	1.549	101.05%
q19	12.64	13.72	1.085	108.59%
q20	27.85	28.41	0.552	101.98%
q21	222.96	226.51	3.553	101.59%
q22	13.81	13.62	-0.191	98.61%
total	1234.51	1241.78	7.274	100.59%

Surbhi-Vijay changed the title ~~[GLUTEN-3432][VL]Add support for BroadcastNestedLoopJoinTransformer~~ [GLUTEN-3432][VL]Add support for BroadcastNestedLoopJoinExec Jan 29, 2024

zhztheplayer reviewed Jan 30, 2024

View reviewed changes

zhztheplayer changed the title ~~[GLUTEN-3432][VL]Add support for BroadcastNestedLoopJoinExec~~ [GLUTEN-3432][VL] Add support for BroadcastNestedLoopJoinExec Jan 30, 2024

Surbhi-Vijay force-pushed the AddBNLJTransformer branch from ef9b75b to 4049c36 Compare February 6, 2024 14:13

Surbhi-Vijay marked this pull request as ready for review February 6, 2024 15:30

Surbhi-Vijay commented Feb 6, 2024

View reviewed changes

Surbhi-Vijay force-pushed the AddBNLJTransformer branch from e7c1ce3 to 7dd7699 Compare February 7, 2024 13:24

Surbhi-Vijay marked this pull request as draft February 8, 2024 13:48

Surbhi-Vijay force-pushed the AddBNLJTransformer branch from 7b25883 to 37e2e33 Compare February 9, 2024 10:44

Surbhi-Vijay force-pushed the AddBNLJTransformer branch from 4da2eb2 to 7583869 Compare February 13, 2024 10:42

Surbhi-Vijay marked this pull request as ready for review February 14, 2024 05:10

Surbhi-Vijay added 10 commits February 19, 2024 10:23

Fix scala style

bb7ea5c

Fix formatting

e66425e

only support inner join for now

821facc

Fix testcase

373b867

minor change

e23936f

Fix testcases

1867970

Fix build failure

06c19d3

minor change

74880df

Added metrics

2520abf

Added outer join support and refactoring

1fd7294

Surbhi-Vijay force-pushed the AddBNLJTransformer branch from 4631e7b to 1fd7294 Compare February 19, 2024 04:57

ayushi-agarwal reviewed Feb 19, 2024

View reviewed changes

backends-velox/src/main/scala/io/glutenproject/backendsapi/velox/SparkPlanExecApiImpl.scala Show resolved Hide resolved

ayushi-agarwal reviewed Feb 19, 2024

View reviewed changes

...-core/src/main/scala/io/glutenproject/execution/BroadcastNestedLoopJoinExecTransformer.scala Outdated Show resolved Hide resolved

zhztheplayer reviewed Feb 20, 2024

View reviewed changes

Surbhi-Vijay commented Feb 20, 2024

View reviewed changes

Surbhi-Vijay added 2 commits February 20, 2024 15:57

Merge branch 'main' of https://github.com/oap-project/gluten into Add…

a664154

…BNLJTransformer

refactoring

10d9d7b

fix minor error

7706bee

Merge branch 'main' of https://github.com/oap-project/gluten into Add…

88036d4

…BNLJTransformer

zhztheplayer approved these changes Feb 22, 2024

View reviewed changes

zhli1142015 merged commit a1f83cd into apache:main Feb 22, 2024
19 checks passed

Surbhi-Vijay mentioned this pull request Feb 26, 2024

[VL] Updated documentation, refactoring and added more testcases for BNLJ #4782

Merged

kecookier self-assigned this Apr 1, 2024

[GLUTEN-3432][VL] Add support for BroadcastNestedLoopJoinExec #4565

[GLUTEN-3432][VL] Add support for BroadcastNestedLoopJoinExec #4565

Conversation

Surbhi-Vijay commented Jan 29, 2024 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Jan 29, 2024

github-actions bot commented Jan 29, 2024

github-actions bot commented Jan 29, 2024

zhztheplayer left a comment

Choose a reason for hiding this comment

Surbhi-Vijay commented Jan 30, 2024 • edited Loading

zhztheplayer commented Feb 5, 2024

Surbhi-Vijay commented Feb 5, 2024

github-actions bot commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 6, 2024

github-actions bot commented Feb 6, 2024

github-actions bot commented Feb 6, 2024

github-actions bot commented Feb 7, 2024

github-actions bot commented Feb 7, 2024

github-actions bot commented Feb 7, 2024

Surbhi-Vijay commented Feb 8, 2024

github-actions bot commented Feb 9, 2024

github-actions bot commented Feb 12, 2024

github-actions bot commented Feb 13, 2024

github-actions bot commented Feb 13, 2024

github-actions bot commented Feb 13, 2024

github-actions bot commented Feb 19, 2024

zhztheplayer Feb 20, 2024 • 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

github-actions bot commented Feb 20, 2024

github-actions bot commented Feb 20, 2024

github-actions bot commented Feb 21, 2024

Surbhi-Vijay commented Feb 21, 2024

zhztheplayer left a comment

Choose a reason for hiding this comment

Surbhi-Vijay commented Feb 22, 2024

GlutenPerfBot commented Feb 22, 2024

Surbhi-Vijay commented Jan 29, 2024 •

edited

Loading

Surbhi-Vijay commented Jan 30, 2024 •

edited

Loading

zhztheplayer Feb 20, 2024 •

edited

Loading