Support split non-AST-able join condition for BroadcastNestedLoopJoin #9635

winningsix · 2023-11-06T00:08:12Z

This is to fix #8832. This PR addressed the case that non-ast-able join condition can be extracted and pushed down to join's child nodes.

It's needed to have two follow-ups: 1. code refactor to decouple broadcast batch build logic and nested loop join implementation; 2. bump up cudf version and update related changes in Plugin to address changes from rapidsai/cudf#13072 tracked in #8157

Signed-off-by: Ferdinand Xu <[email protected]>

winningsix · 2023-11-06T04:50:05Z

build

revans2

In general this looks really good. My main concerns are

this is only for broadcast nested loop joins. I really would like to see this apply to conditional equi-joins too, as I think that is going to be more common than worst case broadcast nested loop join.
I would like to see some integration tests, not just unit tests. The unit tests are okay tests, but the integration tests run in more places and on more versions of Spark than the unit tests do.
Could we add in some tests that deal with higher order functions? We had some issues with higher order functions for tiered project. I don't think there are any problems here, but I would feel a lot better if we actually tested with them in the conditional part of some of these joins.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/AstUtil.scala

sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsMeta.scala

sql-plugin/src/main/scala/com/nvidia/spark/rapids/AstUtil.scala

winningsix · 2023-11-07T12:59:13Z

Still failed some AQE cases. Will take care of them before addressing comments.

winningsix · 2023-11-08T02:57:21Z

build

winningsix · 2023-11-08T12:04:56Z

build

winningsix · 2023-11-08T13:36:47Z

Thanks @revans2 for the reviews.

For 1 together with fallback message improvement, I filed a follow-up umbrella issue in #9661.

For 2 and 3, updated a few more integration tests.

In general this looks really good. My main concerns are

this is only for broadcast nested loop joins. I really would like to see this apply to conditional equi-joins too, as I think that is going to be more common than worst case broadcast nested loop join.

I would like to see some integration tests, not just unit tests. The unit tests are okay tests, but the integration tests run in more places and on more versions of Spark than the unit tests do.

Could we add in some tests that deal with higher order functions? We had some issues with higher order functions for tiered project. I don't think there are any problems here, but I would feel a lot better if we actually tested with them in the conditional part of some of these joins.

integration_tests/src/main/python/join_test.py

revans2 · 2023-11-08T14:55:36Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/AstUtil.scala

+
+    // 2nd step to replace expression pushing down to child plans in depth first fashion
+    (condition.map(
+      _.convertToGpu().mapChildren(


I think this should be fine, but I am not 100% sure that convertToGpu will be idempotent. It might be better to just convert it to the GPU up front and cache it instead of recalling it inside of splitNonAstInternal. To make that work we probably would have to zip the GPU expression and along with condition.childExprs. I don't think this is a blocker for the patch to go in. Just being overly cautious about it.

Hmm, it seems not very straight forward to eliminate non-idempotent risk here. convertToGpu only happens when we need to replace it with a new GpuAlias node. We can probably cache those converted expression but in the end (L100), the root BaseExprMeta will do a real convertToGpu which converts the entire tree. It seems we can hardly reuse the cache here.

I think this is fine for now, so lets file a follow on issue to look into it. I see a few ways to fix it, but none of them are that simple, and may have their own hidden pitfalls.

winningsix · 2023-11-09T08:00:33Z

build

revans2 · 2023-11-13T16:31:52Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/AstUtil.scala

+
+    // 2nd step to replace expression pushing down to child plans in depth first fashion
+    (condition.map(
+      _.convertToGpu().mapChildren(


I think this is fine for now, so lets file a follow on issue to look into it. I see a few ways to fix it, but none of them are that simple, and may have their own hidden pitfalls.

revans2 · 2023-11-13T17:04:51Z

I filed #9680 as a follow on issue for this.

razajafri · 2023-11-13T19:09:36Z

Please remember to build against Databricks by adding [databricks] to the title

…LoopJoin (NVIDIA#9635)" This reverts commit c20a843.

…LoopJoin (NVIDIA#9635)" This reverts commit c20a843. Signed-off-by: Ferdinand Xu <[email protected]>

…LoopJoin (#9635)" (#9695) This reverts commit c20a843. Signed-off-by: Ferdinand Xu <[email protected]>

…NVIDIA#9635) Signed-off-by: Ferdinand Xu <[email protected]>

…databricks] (#9702) * Update Databricks GpuBroadcastNestedLoopJoinExec for AST splitting change Signed-off-by: Jason Lowe <[email protected]> * Hot fix Signed-off-by: Ferdinand Xu <[email protected]> * Support split non-AST-able join condition for BroadcastNestedLoopJoin (#9635) Signed-off-by: Ferdinand Xu <[email protected]> * Address comments --------- Signed-off-by: Jason Lowe <[email protected]> Signed-off-by: Ferdinand Xu <[email protected]> Co-authored-by: Jason Lowe <[email protected]>

winningsix added 2 commits November 6, 2023 07:34

Support split non-AST-able join condition

3f50f88

Signed-off-by: Ferdinand Xu <[email protected]>

Fix Spark 340 above

2638ce8

sameerz added the feature request New feature or request label Nov 6, 2023

Fix

a40f9a5

revans2 reviewed Nov 6, 2023

View reviewed changes

revans2 mentioned this pull request Nov 6, 2023

[FEA] Add AST for safe upcasts #9645

Open

Fix output nullable

3e19600

winningsix added 2 commits November 8, 2023 07:25

Use ExprId to compare equality

542e1ae

Address comments

f3ad734

winningsix added 3 commits November 8, 2023 17:05

Fix

8be33c1

Add more integration test

d2d6277

Correct comments

d63bca6

winningsix changed the title ~~Support split non-AST-able join condition~~ Support split non-AST-able join condition for BroadcastNestedLoopJoin Nov 8, 2023

winningsix mentioned this pull request Nov 8, 2023

[FEA][Follow On] Support non AST rewrite as join condition #9661

Open

6 tasks

revans2 reviewed Nov 8, 2023

View reviewed changes

Address comments

6546c3c

revans2 approved these changes Nov 13, 2023

View reviewed changes

revans2 mentioned this pull request Nov 13, 2023

[BUG] Update AST join split condition code to avoid idempotent issues #9680

Open

revans2 merged commit c20a843 into NVIDIA:branch-23.12 Nov 13, 2023
36 checks passed

razajafri mentioned this pull request Nov 13, 2023

Databricks Build Failing For 330db+ #9681

Closed

jlowe mentioned this pull request Nov 13, 2023

Update Databricks GpuBroadcastNestedLoopJoinExec for AST splitting change [databricks] #9688

Closed

winningsix added a commit to winningsix/spark-rapids that referenced this pull request Nov 14, 2023

Revert "Support split non-AST-able join condition for BroadcastNested…

e110466

…LoopJoin (NVIDIA#9635)" This reverts commit c20a843.

winningsix mentioned this pull request Nov 14, 2023

Revert "Support split non-AST-able join condition for BroadcastNested… [databricks] #9695

Merged

winningsix added a commit to winningsix/spark-rapids that referenced this pull request Nov 14, 2023

Revert "Support split non-AST-able join condition for BroadcastNested…

745e583

…LoopJoin (NVIDIA#9635)" This reverts commit c20a843. Signed-off-by: Ferdinand Xu <[email protected]>

winningsix added a commit that referenced this pull request Nov 14, 2023

Revert "Support split non-AST-able join condition for BroadcastNested…

56de325

…LoopJoin (#9635)" (#9695) This reverts commit c20a843. Signed-off-by: Ferdinand Xu <[email protected]>

winningsix added a commit to winningsix/spark-rapids that referenced this pull request Nov 14, 2023

Support split non-AST-able join condition for BroadcastNestedLoopJoin (…

1364b97

…NVIDIA#9635) Signed-off-by: Ferdinand Xu <[email protected]>

winningsix mentioned this pull request Nov 14, 2023

Support split BroadcastNestedLoopJoin condition for AST and non-AST [databricks] #9702

Merged

revans2 mentioned this pull request Jul 9, 2024

[FEA] Support date cast and date_sub as AST expressions #7030

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support split non-AST-able join condition for BroadcastNestedLoopJoin #9635

Support split non-AST-able join condition for BroadcastNestedLoopJoin #9635

winningsix commented Nov 6, 2023 •

edited

Loading

winningsix commented Nov 6, 2023

revans2 left a comment

winningsix commented Nov 7, 2023

winningsix commented Nov 8, 2023

winningsix commented Nov 8, 2023

winningsix commented Nov 8, 2023

revans2 Nov 8, 2023

winningsix Nov 9, 2023

revans2 Nov 13, 2023

winningsix commented Nov 9, 2023

revans2 Nov 13, 2023

revans2 commented Nov 13, 2023

razajafri commented Nov 13, 2023

Support split non-AST-able join condition for BroadcastNestedLoopJoin #9635

Support split non-AST-able join condition for BroadcastNestedLoopJoin #9635

Conversation

winningsix commented Nov 6, 2023 • edited Loading

winningsix commented Nov 6, 2023

revans2 left a comment

Choose a reason for hiding this comment

winningsix commented Nov 7, 2023

winningsix commented Nov 8, 2023

winningsix commented Nov 8, 2023

winningsix commented Nov 8, 2023

revans2 Nov 8, 2023

Choose a reason for hiding this comment

winningsix Nov 9, 2023

Choose a reason for hiding this comment

revans2 Nov 13, 2023

Choose a reason for hiding this comment

winningsix commented Nov 9, 2023

revans2 Nov 13, 2023

Choose a reason for hiding this comment

revans2 commented Nov 13, 2023

razajafri commented Nov 13, 2023

winningsix commented Nov 6, 2023 •

edited

Loading