-
Notifications
You must be signed in to change notification settings - Fork 240
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
Skip the test for spark versions 3.1.1, 3.1.2 and 3.2.0 only [databricks] #4491
Conversation
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
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 expected to see a change to Spark31XShims to have isCastingStringToNegDecimalScaleSupported
return false, but it's not here. I don't see how the spark311 and spark312 shims are picking up the changes to mark them properly.
31xShim already has it set to false: I believe you can remove it from Spark330Shims as well now. |
@@ -129,5 +129,5 @@ trait Spark31XdbShimsBase extends SparkShims { | |||
|
|||
override def shouldFallbackOnAnsiTimestamp(): Boolean = false | |||
|
|||
override def isCastingStringToNegDecimalScaleSupported: Boolean = true | |||
override def isCastingStringToNegDecimalScaleSupported: Boolean = false |
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.
this does seem a bit dangerous in the sense if we add a databricks 3.1.3 shim we could easily miss this.
perhaps at least put a comment here. I assume the test won't fail if we get it wrong?
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.
The test isn't skipped for 3.1.3, it will blow up saying part of the plan isn't columnar
as there is a check in GpuCast
for it if we forget to override it for Spark-3.1.3-db.
I can move this to Spark312dbShims.
It's already there in the Spark31XShims https://github.com/razajafri/spark-rapids-1/blob/9e601d639caafef010e33717a7e46f82dd2c92e5/sql-plugin/src/main/311until320-nondb/scala/com/nvidia/spark/rapids/shims/v2/Spark31XShims.scala#L546. That should cover it? |
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
build |
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
build |
The fix was posted for 3.1.3+ and 3.2.1+ which makes the bug valid only in version 3.1.1, 3.1.2 and 3.2.0
Fixes #4488
Signed-off-by: Raza Jafri rjafri@nvidia.com