Skip to content

Conversation

@JoshRosen
Copy link
Contributor

There are certain BigDecimals that can be converted into Spark SQL's Decimal class but which produce Decimals that cannot be converted back to BigDecimal without throwing NumberFormatException.

For instance:

val x = BigDecimal(BigInt("18889465931478580854784"), -2147483648)
assert(Decimal(x).toBigDecimal === x)

fails with the following exception:

java.lang.NumberFormatException
    at java.math.BigDecimal.<init>(BigDecimal.java:511)
    at java.math.BigDecimal.<init>(BigDecimal.java:757)
    at scala.math.BigDecimal$.apply(BigDecimal.scala:119)
    at scala.math.BigDecimal.apply(BigDecimal.scala:324)
    at org.apache.spark.sql.types.Decimal.toBigDecimal(Decimal.scala:142)

This PR adds a failing regression test for this issue. I'll try to fix this shortly.

@SparkQA
Copy link

SparkQA commented Jul 2, 2015

Test build #36432 has finished for PR 7198 at commit 6d41282.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor Author

@viirya, could you take a look at this to help me figure out whether it's a bug? It looks like the behavior started happening after #6814. Note that I didn't find this while executing a real user query; this failing test was contrived using some random data generated by ScalaCheck.

@viirya
Copy link
Member

viirya commented Jul 3, 2015

Ok. I will check this problem.

@JoshRosen
Copy link
Contributor Author

It would also be fine to conclude that this is not an issue as long as it only happens for contrived BigDecimal values which can't actually appear in practice.

@viirya
Copy link
Member

viirya commented Jul 3, 2015

I have run some tests about this. It is interesting. The exception is thrown when java.math.BigDecimal is constructed with special String, e.g.

val x = BigDecimal(BigInt("18889465931478580854784"), -2147483648)
new java.math.BigDecimal(x.bigDecimal.round(MathContext.UNLIMITED).toString)
java.lang.NumberFormatException
        at java.math.BigDecimal.<init>(BigDecimal.java:554)
        at java.math.BigDecimal.<init>(BigDecimal.java:383)
        at java.math.BigDecimal.<init>(BigDecimal.java:806)

Decimal(x).toBigDecimal actually is calling scala.math.BigDecimal.apply(mc: MathContext) which is defined as def apply(mc: MathContext): BigDecimal = new BigDecimal(this.bigDecimal round mc, mc).

x.apply(MathContext.UNLIMITED)
        at java.math.BigDecimal.<init>(BigDecimal.java:554)
        at java.math.BigDecimal.<init>(BigDecimal.java:824)
        at scala.math.BigDecimal$.apply(BigDecimal.scala:119)
        at scala.math.BigDecimal.apply(BigDecimal.scala:324)

x.apply(MathContext.DECIMAL128)
        at java.math.BigDecimal.<init>(BigDecimal.java:554)
        at java.math.BigDecimal.<init>(BigDecimal.java:824)
        at scala.math.BigDecimal$.apply(BigDecimal.scala:119)
        at scala.math.BigDecimal.apply(BigDecimal.scala:324)

However, we can successfully run this:

new BigDecimal(x.bigDecimal.round(MathContext.UNLIMITED), MathContext.UNLIMITED)
res59: scala.math.BigDecimal = 1.8889465931478580854784E+2147483670

But if we do this, the exception will be thrown:

new BigDecimal(new java.math.BigDecimal(x.bigDecimal.round(MathContext.UNLIMITED).toString), MathContext.UNLIMITED)
java.lang.NumberFormatException
        at java.math.BigDecimal.<init>(BigDecimal.java:554)
        at java.math.BigDecimal.<init>(BigDecimal.java:383)
        at java.math.BigDecimal.<init>(BigDecimal.java:806)

Because x.bigDecimal.round(MathContext.UNLIMITED) is already a java.math.BigDecimal, currently I am not sure why it will construct another java.math.BigDecimal with the string version of rounded value when calling scala.math.BigDecimal.apply(mc: MathContext).

@viirya
Copy link
Member

viirya commented Jul 3, 2015

Even with MathContext.DECIMAL128, the problem is still there:

new java.math.BigDecimal(x.bigDecimal.round(MathContext.DECIMAL128).toString)
java.lang.NumberFormatException
        at java.math.BigDecimal.<init>(BigDecimal.java:554)
        at java.math.BigDecimal.<init>(BigDecimal.java:383)
        at java.math.BigDecimal.<init>(BigDecimal.java:806)

@JoshRosen
Copy link
Contributor Author

Closing this for now, pending other work on decimal support by other folks.

@JoshRosen JoshRosen closed this Jul 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants