From 2de9afe804ab0c35fa3bffe8d26d3e989bc34000 Mon Sep 17 00:00:00 2001 From: Yin Huai Date: Wed, 15 Jul 2015 12:03:49 -0700 Subject: [PATCH 1/3] Revert "[SPARK-8800] [SQL] Fix inaccurate precision/scale of Decimal division operation" This reverts commit 4b5cfc988f23988c2334882a255d494fc93d252e. --- .../scala/org/apache/spark/sql/types/Decimal.scala | 14 +++----------- .../spark/sql/types/decimal/DecimalSuite.scala | 10 +--------- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala index f5bd068d60dc..5a169488c97e 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala @@ -145,14 +145,6 @@ final class Decimal extends Ordered[Decimal] with Serializable { } } - def toLimitedBigDecimal: BigDecimal = { - if (decimalVal.ne(null)) { - decimalVal - } else { - BigDecimal(longVal, _scale) - } - } - def toJavaBigDecimal: java.math.BigDecimal = toBigDecimal.underlying() def toUnscaledLong: Long = { @@ -277,9 +269,9 @@ final class Decimal extends Ordered[Decimal] with Serializable { if (that.isZero) { null } else { - // To avoid non-terminating decimal expansion problem, we get scala's BigDecimal with limited - // precision and scala. - Decimal(toLimitedBigDecimal / that.toLimitedBigDecimal) + // To avoid non-terminating decimal expansion problem, we turn to Java BigDecimal's divide + // with specified ROUNDING_MODE. + Decimal(toJavaBigDecimal.divide(that.toJavaBigDecimal, ROUNDING_MODE.id)) } } diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/types/decimal/DecimalSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/types/decimal/DecimalSuite.scala index f0c849d1a156..9e8cd026d2a3 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/types/decimal/DecimalSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/types/decimal/DecimalSuite.scala @@ -179,14 +179,6 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester { test("fix non-terminating decimal expansion problem") { val decimal = Decimal(1.0, 10, 3) / Decimal(3.0, 10, 3) - // The difference between decimal should not be more than 0.001. - assert(decimal.toDouble - 0.333 < 0.001) - } - - test("fix loss of precision/scale when doing division operation") { - val a = Decimal(2) / Decimal(3) - assert(a.toDouble < 1.0 && a.toDouble > 0.6) - val b = Decimal(1) / Decimal(8) - assert(b.toDouble === 0.125) + assert(decimal.toString === "0.333") } } From cfda7e4a831301856e1c2591dc7f5452e1c06cbe Mon Sep 17 00:00:00 2001 From: Yin Huai Date: Wed, 15 Jul 2015 12:03:56 -0700 Subject: [PATCH 2/3] Revert "[SPARK-8677] [SQL] Fix non-terminating decimal expansion for decimal divide operation" This reverts commit 24fda7381171738cbbbacb5965393b660763e562. --- .../scala/org/apache/spark/sql/types/Decimal.scala | 11 ++--------- .../apache/spark/sql/types/decimal/DecimalSuite.scala | 5 ----- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala index 5a169488c97e..bd9823bc0542 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala @@ -265,15 +265,8 @@ final class Decimal extends Ordered[Decimal] with Serializable { def * (that: Decimal): Decimal = Decimal(toBigDecimal * that.toBigDecimal) - def / (that: Decimal): Decimal = { - if (that.isZero) { - null - } else { - // To avoid non-terminating decimal expansion problem, we turn to Java BigDecimal's divide - // with specified ROUNDING_MODE. - Decimal(toJavaBigDecimal.divide(that.toJavaBigDecimal, ROUNDING_MODE.id)) - } - } + def / (that: Decimal): Decimal = + if (that.isZero) null else Decimal(toBigDecimal / that.toBigDecimal) def % (that: Decimal): Decimal = if (that.isZero) null else Decimal(toBigDecimal % that.toBigDecimal) diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/types/decimal/DecimalSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/types/decimal/DecimalSuite.scala index 9e8cd026d2a3..9e74bbb50aaa 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/types/decimal/DecimalSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/types/decimal/DecimalSuite.scala @@ -176,9 +176,4 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester { val decimal = (Decimal(Long.MaxValue, 38, 0) * Decimal(Long.MaxValue, 38, 0)).toJavaBigDecimal assert(decimal.unscaledValue.toString === "85070591730234615847396907784232501249") } - - test("fix non-terminating decimal expansion problem") { - val decimal = Decimal(1.0, 10, 3) / Decimal(3.0, 10, 3) - assert(decimal.toString === "0.333") - } } From 651264d9e5e4db7c77fd2b867181e4bff3823eab Mon Sep 17 00:00:00 2001 From: Yin Huai Date: Wed, 15 Jul 2015 12:04:02 -0700 Subject: [PATCH 3/3] Revert "[SPARK-8359] [SQL] Fix incorrect decimal precision after multiplication" This reverts commit 31bd30687bc29c0e457c37308d489ae2b6e5b72a. --- .../src/main/scala/org/apache/spark/sql/types/Decimal.scala | 6 ++---- .../org/apache/spark/sql/types/decimal/DecimalSuite.scala | 5 ----- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala index bd9823bc0542..a85af9e04aed 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala @@ -17,8 +17,6 @@ package org.apache.spark.sql.types -import java.math.{MathContext, RoundingMode} - import org.apache.spark.annotation.DeveloperApi /** @@ -139,9 +137,9 @@ final class Decimal extends Ordered[Decimal] with Serializable { def toBigDecimal: BigDecimal = { if (decimalVal.ne(null)) { - decimalVal(MathContext.UNLIMITED) + decimalVal } else { - BigDecimal(longVal, _scale)(MathContext.UNLIMITED) + BigDecimal(longVal, _scale) } } diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/types/decimal/DecimalSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/types/decimal/DecimalSuite.scala index 9e74bbb50aaa..1d297beb3868 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/types/decimal/DecimalSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/types/decimal/DecimalSuite.scala @@ -171,9 +171,4 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester { assert(new Decimal().set(100L, 10, 0).toUnscaledLong === 100L) assert(Decimal(Long.MaxValue, 100, 0).toUnscaledLong === Long.MaxValue) } - - test("accurate precision after multiplication") { - val decimal = (Decimal(Long.MaxValue, 38, 0) * Decimal(Long.MaxValue, 38, 0)).toJavaBigDecimal - assert(decimal.unscaledValue.toString === "85070591730234615847396907784232501249") - } }