Fix inconsistencies in AQE support for broadcast joins #1042
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Andy Grove <andygrove@nvidia.com>
Signed-off-by: Andy Grove <andygrove@nvidia.com>
Signed-off-by: Andy Grove <andygrove@nvidia.com>
Signed-off-by: Andy Grove <andygrove@nvidia.com>
andygrove
changed the title
Signed-off-by: Andy Grove <andygrove@nvidia.com>
revans2
reviewed
Oct 29, 2020
sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsMeta.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: Andy Grove <andygrove@nvidia.com>
Signed-off-by: Andy Grove <andygrove@nvidia.com>
Signed-off-by: Andy Grove <andygrove@nvidia.com>
tgravescs
reviewed
Oct 29, 2020
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/execution/GpuBroadcastJoinMeta.scala
Outdated
Show resolved
Hide resolved
|
also can we update the description or issue to have your findings on what the problem is |
Signed-off-by: Andy Grove <andygrove@nvidia.com>
|
build |
tgravescs
reviewed
Oct 29, 2020
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuBroadcastJoinMeta.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: Andy Grove <andygrove@nvidia.com>
|
build |
revans2
approved these changes
Oct 29, 2020
tgravescs
reviewed
Oct 30, 2020
| @@ -119,6 +119,13 @@ class Spark300Shims extends SparkShims { | |||
| } | |||
| } | |||
|
|
|||
| override def isGpuBroadcastNestedLoopJoin(plan: SparkPlan): Boolean = { | |||
| plan match { | |||
| case _: GpuBroadcastNestedLoopJoinExecBase => true | |||
There was a problem hiding this comment.
sorry for my late comment I was looking at something else and realized this doesn't need to be in the shim, you are using the Base class here so there is nothing shim specific about it.
sperlingxx
pushed a commit
to sperlingxx/spark-rapids
that referenced
this pull request
Nov 20, 2020
* Fix inconsistencies with AQE support for broadcast joins Signed-off-by: Andy Grove <andygrove@nvidia.com> * code cleanup and change test behavior for Spark 3.0.0 Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix inconsistency Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix test failure with Spark 3.1.0 Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix inconsistency Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix imports Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix regression Signed-off-by: Andy Grove <andygrove@nvidia.com> * tighten up rules Signed-off-by: Andy Grove <andygrove@nvidia.com> * Move GpuBroadcastJoinMeta to com.nvidia.spark.rapids package Signed-off-by: Andy Grove <andygrove@nvidia.com> * Move GpuBroadcastJoinMeta to com.nvidia.spark.rapids package Signed-off-by: Andy Grove <andygrove@nvidia.com>
nartal1
pushed a commit
to nartal1/spark-rapids
that referenced
this pull request
Jun 9, 2021
* Fix inconsistencies with AQE support for broadcast joins Signed-off-by: Andy Grove <andygrove@nvidia.com> * code cleanup and change test behavior for Spark 3.0.0 Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix inconsistency Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix test failure with Spark 3.1.0 Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix inconsistency Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix imports Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix regression Signed-off-by: Andy Grove <andygrove@nvidia.com> * tighten up rules Signed-off-by: Andy Grove <andygrove@nvidia.com> * Move GpuBroadcastJoinMeta to com.nvidia.spark.rapids package Signed-off-by: Andy Grove <andygrove@nvidia.com> * Move GpuBroadcastJoinMeta to com.nvidia.spark.rapids package Signed-off-by: Andy Grove <andygrove@nvidia.com>
nartal1
pushed a commit
to nartal1/spark-rapids
that referenced
this pull request
Jun 9, 2021
* Fix inconsistencies with AQE support for broadcast joins Signed-off-by: Andy Grove <andygrove@nvidia.com> * code cleanup and change test behavior for Spark 3.0.0 Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix inconsistency Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix test failure with Spark 3.1.0 Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix inconsistency Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix imports Signed-off-by: Andy Grove <andygrove@nvidia.com> * fix regression Signed-off-by: Andy Grove <andygrove@nvidia.com> * tighten up rules Signed-off-by: Andy Grove <andygrove@nvidia.com> * Move GpuBroadcastJoinMeta to com.nvidia.spark.rapids package Signed-off-by: Andy Grove <andygrove@nvidia.com> * Move GpuBroadcastJoinMeta to com.nvidia.spark.rapids package Signed-off-by: Andy Grove <andygrove@nvidia.com>
tgravescs
pushed a commit
to tgravescs/spark-rapids
that referenced
this pull request
Nov 30, 2023
…IDIA#1042) Signed-off-by: spark-rapids automation <70000568+nvauto@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes some inconsistencies in the logic in broadcast joins for determining whether they can run on the GPU or not when AQE is enabled.
The existing logic for
BroadcastNestedLoopJoinwas expecting the build side to beGpuBroadcastExchangeExecBaseand would fail if the build side was aBroadcastQueryStageExecas is the case when AQE is enabled and when the build side has already been materialized.Although the logic for
BroadcastHashJoinwas taking AQE into account, the code was inconsistent and incomplete so this PR addresses that too.Changes in this PR:
GpuBroadcastJoinMetabase class introduced so that common logic for broadcast nested-loop and hash joins could be introduced for determining when these operators can run on GPU.GpuBroadcastNestedLoopJoinwith AQE on and off to confirm that the fixes work.TestUtils.operatorCounttofindOperatorssince the name was misleading (it returns a list of operators, not a count) and this change only impacted a few lines of existing code.I manually tested these changes with TPC-DS q90 and confirmed that the query now runs without error.
This closes #1035