[SPARK-8359][SQL] Fix incorrect decimal precision after multiplication

[viirya]

JIRA: https://issues.apache.org/jira/browse/SPARK-8359

[SparkQA]

Test build #34882 has finished for PR 6814 at commit 44c9348.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class JoinedRow extends InternalRow
  • class JoinedRow2 extends InternalRow
  • class JoinedRow3 extends InternalRow
  • class JoinedRow4 extends InternalRow
  • class JoinedRow5 extends InternalRow
  • class JoinedRow6 extends InternalRow
  • class BaseOrdering extends Ordering[InternalRow]

[davies]

This is kind of workaround, didn't fix the root cause.

The root cause should be in toBigDecimal, which doesn't carry on the information about precision to BigDecimal. Could you fix that?

cc @mateiz

[viirya]

toBigDecimal just creates scala BigDecimal with its information. I think it is correct. The problem looks like the scala BigDecimal produces wrong different result, compared with java BigDecimal when doing this multiplication.

To show that, we can create a scala BigDecimal. We find that it has correct precision as same as its underlying java BigDecimal:

 scala> val d = BigDecimal(Long.MaxValue, 0)
 d: scala.math.BigDecimal = 9223372036854775807

 scala> d.precision
 res16: Int = 19

 scala> d.underlying.precision
 res17: Int = 19

 scala> d.scale
 res18: Int = 0

 scala> d.underlying.scale
 res19: Int = 0

When we multiply two scala BigDecimal carrying Long.MaxValue, we get the wrong result:

 scala> val t = BigDecimal(Long.MaxValue, 0) * BigDecimal(Long.MaxValue, 0)
 t: scala.math.BigDecimal = 8.507059173023461584739690778423250E+37

 scala> t.precision
 res20: Int = 34

 scala> t.scale
 res21: Int = -4

 scala> t.underlying.unscaledValue.toString
 res22: String = 8507059173023461584739690778423250

When we multiply two java BigDecimal carrying Long.MaxValue, the result is different:

 scala> val j = d.underlying.multiply(d.underlying)
 j: java.math.BigDecimal = 85070591730234615847396907784232501249

 scala> j.precision
 res23: Int = 38

 scala> j.scale
 res24: Int = 0

 scala> j.toString
 res25: String = 85070591730234615847396907784232501249

[viirya]

OK. I just found their results are different due to the MathContext. As @davies said, its precision is 34. Since we ask to print the unscaledValue, so the scale -4 is not applied on the value. So this should not be a bug. Maybe we don't need to do this modification as we should not directly use the unscaledValue?

[viirya]

However, as decimal type in Hive is based on Java's BigDecimal. As I tested, Hive can output the accurate result of select CAST(9223372036854775807 as DECIMAL(38,0)) * CAST(9223372036854775807 as DECIMAL(38,0));

[davies]

Can Hive support higher precision than 38? I think should match the behavior in Hive.

[viirya]

It can't. But 85070591730234615847396907784232501249 can be handled by precision 38. As the MathContext has only precision 34, we will have scale -4 and get different result.

[davies]

I think this can be fixed by in toBigDecimal:

BigDecimal(longVal, _scale, new MathContext(precision, RoundingMode.HALF_EVEN))

Also, in order to have the same behavior as other datebase or Hive, we should throw an exception if precision is higher than 38, this could be another PR.

[mateiz]

Hive doesn't actually support BigDecimals with precision above 38. Why did you want to add these? It may be okay to add them, but I think the current code works fine for decimals with lower precision.

[davies]

@mateiz We use DECIMAL128 as the MathContext to create BigDecimal, which has precision as 34, it's lower than 38 in Hive.

scala> val d = Decimal(2L<<60, 38, 0)
d: org.apache.spark.sql.types.Decimal = 2305843009213693952
scala> d * d
res0: org.apache.spark.sql.types.Decimal = 5.316911983139663491615228241121378E+36
scala> (d * d).toJavaBigDecimal.unscaledValue
res5: java.math.BigInteger = 5316911983139663491615228241121378  // this is wrong

[mateiz]

Ah, okay. We should make sure we do exactly the same thing as Hive -- it's possible that Hive also uses this context internally.

[SparkQA]

Test build #35300 has finished for PR 6814 at commit a43bfc3.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[viirya]

The style error is from #3347.

[andrewor14]

retest this please

[SparkQA]

Test build #35308 has finished for PR 6814 at commit a43bfc3.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[davies]

I think we should use the _precesion in Decimal object.

[viirya]

_precesion is the current precision of this decimal. But here we want to assign a maximum precision that affects in later decimal operation like multiplication.

[SparkQA]

Test build #35362 has finished for PR 6814 at commit 071a757.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[davies]

LGTM, merging this into master, thanks!