-
Notifications
You must be signed in to change notification settings - Fork 241
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
Add Support for Renaming of PythonMapInArrow [databricks] #10931
Add Support for Renaming of PythonMapInArrow [databricks] #10931
Conversation
Signed-off-by: Raza Jafri <[email protected]>
@@ -0,0 +1,72 @@ | |||
/* | |||
* Copyright (c) 2022-2024, NVIDIA CORPORATION. |
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.
* Copyright (c) 2022-2024, NVIDIA CORPORATION. | |
* Copyright (c) 2024, NVIDIA CORPORATION. |
import org.apache.spark.sql.execution.python.MapInArrowExec | ||
import org.apache.spark.sql.rapids.execution.python.GpuMapInBatchExec | ||
|
||
class GpuMapInArrowExecMetaBase( |
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.
I'm confused why this is here and not in GpuMapInArrowExecMeta, and also confusing why the base class has a convertToGpu that needs to be overridden by the only class that actually uses it. Why have a base class if there's only one user, or why have a method that is not used in the base class?
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.
Ugh, thank you!
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.
I was fighting the compiler for potentially including a common base class, I gave up but then had some remnants left
build |
build |
1 similar comment
build |
ExecChecks((TypeSig.commonCudfTypes + TypeSig.ARRAY + TypeSig.STRUCT).nested(), | ||
TypeSig.all), | ||
(mapPy, conf, p, r) => new GpuMapInArrowExecMeta(mapPy, conf, p, r) { | ||
override def tagPlanForGpu(): Unit = { |
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.
Any specific reason this method is overridden here as opposed to sql-plugin/src/main/spark400/scala/org/apache/spark/sql/rapids/shims/GpuMapInArrowExecMeta.scala
?
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.
Thanks for the pointer. I have addressed it. PTAL
build |
* Add support for the renaming of PythonMapInArrow to MapInArrow * Signing off Signed-off-by: Raza Jafri <[email protected]> * Removed the unnecessary base class from 400 * addressed review comments --------- Signed-off-by: Raza Jafri <[email protected]>
In Spark change apache/spark@ed9a3a8,
PythonMapInArrow
has been changed toMapInArrow
. This PR adds shims to support that change.fixes #10673