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.

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

Fix rounds over decimal in Spark 330+ #5786

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

sperlingxx
Copy link
Collaborator

SPARK-39226 changed the way to compute output precision and scale for round-like functions, which fails the round-like tests: #5769.

This PR passes the datatype of round-like functions directly to GPU overrides, so as to adapt different Spark versions. The shim method approach doesn't work very well in this case because the computition of decimal precision and scale requiring member variables of rounds-like functions.
This PR also modifies the round logic over decimal type to adapt both original and new manner of datatype. In addition, some test cases are skipped in Spark 330+ since they are not supported by Spark any more.

Signed-off-by: sperlingxx [email protected]

@sperlingxx sperlingxx requested a review from nartal1 June 8, 2022 10:00
@sperlingxx
Copy link
Collaborator Author

build

@tgravescs tgravescs added the bug Something isn't working label Jun 8, 2022
@tgravescs tgravescs added this to the Jun 6 - Jun 17 milestone Jun 8, 2022
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Like #5766, this has a post-round cast that probably can be avoided in most cases. Not must-fix but would be good to have a followup issue to investigate if not addressed here.

Comment on lines +586 to +587
rounded.castTo(
DecimalUtil.createCudfDecimal(dataType.asInstanceOf[DecimalType]))
Copy link
Member

Choose a reason for hiding this comment

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

This cast seems unnecessary in most cases, may be worth checking if the desired cudf type is already the type computed to avoid an unnecessary copy.

@sperlingxx sperlingxx merged commit 51856cb into NVIDIA:branch-22.06 Jun 8, 2022
@sperlingxx sperlingxx deleted the fix_roundlike_330 branch June 8, 2022 15:56
tgravescs added a commit that referenced this pull request Jun 8, 2022
* Correct the error message for test_mod_pmod_by_zero (#5781)

Signed-off-by: Firestarman <[email protected]>

* Update the error string for test_cast_neg_to_decimal_err on 330[databricks] (#5784)

* Update the error string for test_cast_neg_to_decimal_err on 330

Signed-off-by: Firestarman <[email protected]>

* address comments

Signed-off-by: Firestarman <[email protected]>

* Throw an exception when attempting to read columnar encrypted Parquet files on the GPU [databricks] (#5761)

* Throw useful message when parquet columnar encryption enabled

* update message

* fix message

* handle native encrypted

* move variable

* cleanup

* fix native check

* cleanup imports

* fix import order

* Sign off

Signed-off-by: Thomas Graves <[email protected]>

* Shim the parquet crypto exception check

Signed-off-by: Thomas Graves <[email protected]>

* shim 320cdh

* Add test for parquet encryption

Signed-off-by: Thomas Graves <[email protected]>

* fix rounds over decimal in Spark 330+ (#5786)

Passes the datatype of round-like functions directly to GPU overrides, so as to adapt different Spark versions.

Signed-off-by: sperlingxx <[email protected]>

* Fix the overflow of container type when casting floats to decimal (#5766)

Fixes #5765

Fix the potential overflow when casting float/double to decimal. The overflow occurs on the container decimal for HALF_UP round.
 
Signed-off-by: sperlingxx <[email protected]>

Co-authored-by: Liangcai Li <[email protected]>
Co-authored-by: Alfred Xu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants