[SPARK-17494] [SQL] changePrecision() on compact decimal should respect rounding mode #15154
Conversation
|
Test build #65617 has finished for PR 15154 at commit
|
| } | ||
| case ROUND_HALF_UP => | ||
| if (math.abs(droppedDigits) * 2 >= pow10diff) { | ||
| longVal += (if (longVal < 0) -1L else 1L) |
There was a problem hiding this comment.
Is this correct if longVal is 0, after the division above
What happens if you round_half_up -0.6 ? Looks like you may get +1 ?
There was a problem hiding this comment.
Good catch, we should use droppedDigits here.
|
|
||
| test("changePrecision() on compact decimal should respect rounding mode") { | ||
| Seq(ROUND_FLOOR, ROUND_CEILING, ROUND_HALF_UP, ROUND_HALF_EVEN).foreach { mode => | ||
| Seq("1.0", "1.1", "1.6", "2.5", "5.5", "-1.0", "-1.1", "-1.6", "-2.5", "-5.5").foreach { n => |
There was a problem hiding this comment.
tests of the form 0.5, -0.5, 0.6 etc. might also be useful
There was a problem hiding this comment.
Currently, these numbers are copied from the java doc of RoundingMode, will add more for ROUND_HALF_EVEN
|
|
||
| test("changePrecision() on compact decimal should respect rounding mode") { | ||
| Seq(ROUND_FLOOR, ROUND_CEILING, ROUND_HALF_UP, ROUND_HALF_EVEN).foreach { mode => | ||
| Seq("1.0", "1.1", "1.6", "2.5", "5.5", "-1.0", "-1.1", "-1.6", "-2.5", "-5.5").foreach { n => |
There was a problem hiding this comment.
Currently, these numbers are copied from the java doc of RoundingMode, will add more for ROUND_HALF_EVEN
| } | ||
| case ROUND_HALF_UP => | ||
| if (math.abs(droppedDigits) * 2 >= pow10diff) { | ||
| longVal += (if (longVal < 0) -1L else 1L) |
There was a problem hiding this comment.
Good catch, we should use droppedDigits here.
|
Test build #65732 has finished for PR 15154 at commit
|
Fix typo
| test("changePrecision() on compact decimal should respect rounding mode") { | ||
| Seq(ROUND_FLOOR, ROUND_CEILING, ROUND_HALF_UP, ROUND_HALF_EVEN).foreach { mode => | ||
| Seq("0.4", "0.5", "0.6", "1.0", "1.1", "1.6", "2.5", "5.5").foreach { n => | ||
| Seq("", "-").foreach { sigh => |
There was a problem hiding this comment.
I think you mean sign, not sigh, though I can understand :)
|
Test build #65739 has finished for PR 15154 at commit
|
|
Merging in master/2.0. |
…t rounding mode ## What changes were proposed in this pull request? Floor()/Ceil() of decimal is implemented using changePrecision() by passing a rounding mode, but the rounding mode is not respected when the decimal is in compact mode (could fit within a Long). This Update the changePrecision() to respect rounding mode, which could be ROUND_FLOOR, ROUND_CEIL, ROUND_HALF_UP, ROUND_HALF_EVEN. ## How was this patch tested? Added regression tests. Author: Davies Liu <davies@databricks.com> Closes #15154 from davies/decimal_round. (cherry picked from commit 8bde03b) Signed-off-by: Reynold Xin <rxin@databricks.com>
What changes were proposed in this pull request?
Floor()/Ceil() of decimal is implemented using changePrecision() by passing a rounding mode, but the rounding mode is not respected when the decimal is in compact mode (could fit within a Long).
This Update the changePrecision() to respect rounding mode, which could be ROUND_FLOOR, ROUND_CEIL, ROUND_HALF_UP, ROUND_HALF_EVEN.
How was this patch tested?
Added regression tests.