Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support String to Decimal 128 [databricks] #4172

Merged
merged 21 commits into from
Jan 5, 2022
Merged

Support String to Decimal 128 [databricks] #4172

merged 21 commits into from
Jan 5, 2022

Conversation

razajafri
Copy link
Collaborator

  • Change the check to appropriately check Decimal 128 instead of Decimal 64
  • Modified tests

addresses only part of #3879

@razajafri
Add support for string->decimal128
0dbba41 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
revans2
revans2 previously approved these changes Nov 22, 2021
View reviewed changes
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I would like to see us also file a follow on issue for us to look at how we could support rounding directly and support casting with a precision of 38.

@razajafri razajafri marked this pull request as ready for review November 22, 2021 18:45
@razajafri razajafri mentioned this pull request Nov 22, 2021
Closed
@razajafri
Copy link
Collaborator Author

build

@razajafri razajafri changed the title Support String to Decimal 128 Support String to Decimal 128 [databricks] Nov 23, 2021
@razajafri
Copy link
Collaborator Author

There seems to be a regression because of a performance improvement in how Strings were casted to Decimal by this PR. This is causing the tests to fail against Spark 3.1.1+

I have filed an issue for it

@razajafri
Added test for casting decimal 128 to string
298afc9 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
added method for enabling/disabling negative decimal support
681b938 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

build

@sameerz sameerz added the feature request New feature or request label Dec 1, 2021
@sameerz sameerz added this to the Nov 30 - Dec 10 milestone Dec 1, 2021
@sameerz sameerz mentioned this pull request Dec 1, 2021
Closed
3 tasks
@razajafri
upmerge
105fec9 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

I accidentally merged branch-21.12. I will have to rectify and force push

@razajafri razajafri changed the base branch from branch-22.02 to branch-21.12 December 3, 2021 17:48
@razajafri razajafri changed the base branch from branch-21.12 to branch-22.02 December 3, 2021 17:48
@razajafri razajafri changed the base branch from branch-22.02 to branch-21.12 December 6, 2021 21:28
@razajafri razajafri changed the base branch from branch-21.12 to branch-22.02 December 6, 2021 21:29
razajafri and others added 4 commits December 6, 2021 14:13
@razajafri
addressed review comments
7c3f842 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
refactored method to parent class
2e4f952 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
we should test the op on cpu and gpu if pre spark311
e73c12d 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Merge branch 'branch-22.02' into SP-3879-string-dec
476c67b
@razajafri
Copy link
Collaborator Author

build

revans2
revans2 reviewed Dec 9, 2021
View reviewed changes
...plugin/src/main/301until310-nondb/scala/com/nvidia/spark/rapids/shims/v2/Spark30XShims.scala Outdated Show resolved Hide resolved
integration_tests/src/main/python/cast_test.py Outdated Show resolved Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala Outdated
@@ -115,6 +115,11 @@ final class CastExprMeta[INPUT <: CastBase](
YearParseUtil.tagParseStringAsDate(conf, this)
case (_: StringType, _: DateType) =>
YearParseUtil.tagParseStringAsDate(conf, this)
case (_: StringType, dt:DecimalType) =>
if (dt.scale < 0 && !ShimLoader.getSparkShims.isNegativeDecimalScaleSupportEnabled) {
willNotWorkOnGpu("Rapids doesn't support negative decimal scale for this version of " +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"RAPIDS doesn't support casting string to decimal for negative scale decimal in this version of Spark because of SPARK-....

@sameerz sameerz linked an issue Dec 29, 2021 that may be closed by this pull request
[FEA] Decimal 128 Support: Casting #3879
Closed
3 tasks
@razajafri
addressed review comments
33221df 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Merge remote-tracking branch 'origin/branch-22.02' into HEAD
027f777 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

build

@razajafri
only test negative scale decimal if Spark version < 3.1.1
913efee 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

build

@razajafri
fixed the precedence of the predicates
d4568ea 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

build

@razajafri
Merge remote-tracking branch 'origin/branch-22.02' into HEAD
b35894b 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Don't skip the test for spark 3.3.0+
476396b 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

build

@razajafri
updated message if the test is skipped
e541906 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

build

revans2
revans2 previously approved these changes Jan 4, 2022
View reviewed changes
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor nit about a comment.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala Outdated Show resolved Hide resolved
@razajafri
added shim method and removed the comment
bf826dd 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
revans2
revans2 previously approved these changes Jan 4, 2022
View reviewed changes
@razajafri
Copy link
Collaborator Author

build

@razajafri
configs.md check in
9da7c31 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

build

@razajafri
updated copyright
d55faa1 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
remove decimal 128 from unsupported types
8af61e1 
Signed-off-by: Raza Jafri <rjafri@nvidia.com>
@razajafri
Copy link
Collaborator Author

build

@razajafri razajafri merged commit 56245ab into NVIDIA:branch-22.02 Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlowe jlowe jlowe left review comments

@revans2 revans2 revans2 approved these changes

@GaryShen2008 GaryShen2008 Awaiting requested review from GaryShen2008

@NvTimLiu NvTimLiu Awaiting requested review from NvTimLiu

@tgravescs tgravescs Awaiting requested review from tgravescs

Assignees

@razajafri razajafri

Labels
feature request New feature or request
Projects
None yet
Milestone
Dec 13 - Jan 7
Development

Successfully merging this pull request may close these issues.

[FEA] Decimal 128 Support: Casting
4 participants
@razajafri @jlowe @revans2 @sameerz