-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Advertise CPU sort order and partitioning expressions to Catalyst [databricks] #3719
Conversation
Signed-off-by: Jason Lowe <jlowe@nvidia.com>
…ng issues Signed-off-by: Jason Lowe <jlowe@nvidia.com>
This depends on #3691. Marking as a draft until the dependency is merged. |
build |
build |
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuTransitionOverrides.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this makes sense, I had one comment on whether we wanted to fix or add more explicit asserts if the gpu order was complex (required an eval). I am not sure what the outcome of the failure is here => failed query, or less performant.
build |
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCoalesceBatches.scala
Outdated
Show resolved
Hide resolved
shims/spark320/src/main/scala/com/nvidia/spark/rapids/shims/spark320/Spark320Shims.scala
Outdated
Show resolved
Hide resolved
build |
build |
Ran into a couple of issues with passing the CPU expressions as part of the regular arguments to the case classes:
By passing them as a separate parameter list to the case classes, they no longer appear in the |
build |
db 8.2 failures: `
|
build |
build |
cpuLeftKeys, | ||
cpuRightKeys) { | ||
|
||
override def otherCopyArgs: Seq[AnyRef] = cpuLeftKeys :: cpuRightKeys :: Nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits:
prefer Seq.empty to Nil.
could also just have cpuLeftKeys ++ cpuRightKeys
?
child: SparkPlan)( | ||
override val cpuPartitionSpec: Seq[Expression]) extends GpuWindowInPandasExecBase { | ||
|
||
override def otherCopyArgs: Seq[AnyRef] = cpuPartitionSpec :: Nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why :: Nil
@@ -78,6 +78,9 @@ case class GpuFileSourceScanExec( | |||
|
|||
override def otherCopyArgs: Seq[AnyRef] = Seq(rapidsConf) | |||
|
|||
// All expressions are filter expressions used on the CPU. | |||
override def gpuExpressions: Seq[Expression] = Nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Seq.empty
Fixes #3713
Changes the GPU nodes in the Catalyst plan to advertise CPU
SortOrder
andPartitioning
classes and child expressions. This helps the Spark code better recognize that portions of the plan are still compatible from a sorting and partitioning perspective even after the GPU has updated the plan. Spark has a number of places in the code where it is matching against specific Spark CPU partition classes or checking if CPU sort/partition expressions match, Returning the CPU expressions from the GPU nodes'outputPartitioning
,outputOrdering
,requiredChildDistribution
andrequiredChildOrdering
methods helps us pass these checks in Apache Spark code.This is accomplished by passing both the GPU and CPU expressions to exec nodes that need to override their output ordering or partitioning. It uses the GPU expressions during computation but "advertises" the CPU expressions from the Catalyst methods to query the node's intentions.