[SPARK-40387][SQL] Improve the implementation of Spark Decimal #37830
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
The failure test case is not related to this PR. |
Contributor
|
I don't think |
Contributor
Author
OK |
cloud-fan
reviewed
Sep 9, 2022
| } else { | ||
| BigDecimal(longVal, _scale) | ||
| } | ||
| def toBigDecimal: BigDecimal = if (decimalVal.ne(null)) { |
Contributor
There was a problem hiding this comment.
can we avoid pure code style change? the previous code style was not wrong.
cloud-fan
reviewed
Sep 9, 2022
| def % (that: Decimal): Decimal = | ||
| if (that.isZero) null | ||
| else Decimal(toJavaBigDecimal.remainder(that.toJavaBigDecimal, MATH_CONTEXT)) | ||
| if (that.isZero) null else Decimal(toJavaBigDecimal.remainder(that.toJavaBigDecimal, |
Contributor
There was a problem hiding this comment.
let's avoid pure code style changes.
cloud-fan
reviewed
Sep 9, 2022
| } | ||
|
|
||
| def abs: Decimal = if (this.compare(Decimal.ZERO) < 0) this.unary_- else this | ||
| def abs: Decimal = if (this < Decimal.ZERO) this.unary_- else this |
Contributor
There was a problem hiding this comment.
does this have a real impact?
Contributor
Author
There was a problem hiding this comment.
this < Decimal.ZERO is more clear.
cloud-fan
reviewed
Sep 13, 2022
|
|
||
| def toFloat: Float = toBigDecimal.floatValue | ||
|
|
||
| private def rawLongValue: Long = longVal / POW_10(_scale) |
Contributor
There was a problem hiding this comment.
let's still use the old name actualLongVal
cloud-fan
approved these changes
Sep 13, 2022
Contributor
|
thanks, merging to master! |
Contributor
Author
|
@cloud-fan Thank you ! |
LuciferYang
pushed a commit
to LuciferYang/spark
that referenced
this pull request
Sep 20, 2022
### What changes were proposed in this pull request? This PR used to improve the implementation of Spark `Decimal`. The improvement points are as follows: 1. Use `toJavaBigDecimal` instead of `toBigDecimal.bigDecimal` 2. Extract `longVal / POW_10(_scale)` as a new method `def actualLongVal: Long` 3. Remove `BIG_DEC_ZERO` and use `decimalVal.signum` to judge whether or not equals zero. 4. Use `<` instead of `compare`. 5. Correct some code style. ### Why are the changes needed? Improve the implementation of Spark Decimal ### Does this PR introduce _any_ user-facing change? 'No'. Just update the inner implementation. ### How was this patch tested? N/A Closes apache#37830 from beliefer/SPARK-40387. Authored-by: Jiaan Geng <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR used to improve the implementation of Spark
Decimal. The improvement points are as follows:toJavaBigDecimalinstead oftoBigDecimal.bigDecimallongVal / POW_10(_scale)as a new methoddef actualLongVal: LongBIG_DEC_ZEROand usedecimalVal.signumto judge whether or not equals zero.<instead ofcompare.Why are the changes needed?
Improve the implementation of Spark Decimal
Does this PR introduce any user-facing change?
'No'.
Just update the inner implementation.
How was this patch tested?
N/A