Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: BigDecimal is broken on 2.13.0, works on 2.12.8 #11590

Closed
35VLG84 opened this issue Jun 25, 2019 · 13 comments
Labels
Milestone

Comments

@35VLG84
Copy link

@35VLG84 35VLG84 commented Jun 25, 2019

Hello,

Following code works with Scala 2.12.8 but it doesn't work with 2.13.0.

Scala 2.12.8 is ok:

Welcome to Scala 2.12.8 (OpenJDK 64-Bit Server VM, Java 1.8.0_212).
Type in expressions for evaluation. Or try :help.

scala> :paste
// Entering paste mode (ctrl-D to finish)

object BigDecimalTest {
  def main(args: Array[String]): Unit = {
    test()
  }

  def test(): Unit = {

     val bd1 = scala.math.BigDecimal("1000000000000000000000000.1")
     val bd2 = scala.math.BigDecimal("2000000000000000000000.0002")
     val bd3 = scala.math.BigDecimal("3000000000000000000.0000003")
     val bd4 = scala.math.BigDecimal("4000000000000000.0000000004")
     val bd5 = scala.math.BigDecimal("5000000000000.0000000000005")
     val bd6 = scala.math.BigDecimal("6000000000.0000000000000006")
     val bd7 = scala.math.BigDecimal("7000000.0000000000000000007")
     val bd8 = scala.math.BigDecimal("8000.0000000000000000000008")
     val bd9 = scala.math.BigDecimal("9.0000000000000000000000009")


     assert(bd9  == scala.math.BigDecimal("9.0000000000000000000000009"))
     assert(bd8 + bd9 == scala.math.BigDecimal("8009.0000000000000000000008009"))
     assert(bd7 + bd8 + bd9 == scala.math.BigDecimal("7008009.0000000000000000007008009"))
     assert(bd6 + bd7 + bd8 + bd9 == scala.math.BigDecimal("6007008009.0000000000000006007008009"))
     assert(bd5 + bd6 + bd7 + bd8 + bd9 == scala.math.BigDecimal("5006007008009.0000000000005006007008009"))
     assert(bd4 + bd5 + bd6 + bd7 + bd8 + bd9 == scala.math.BigDecimal("4005006007008009.0000000004005006007008009"))
     assert(bd3 + bd4 + bd5 + bd6 + bd7 + bd8 + bd9 == scala.math.BigDecimal("3004005006007008009.0000003004005006007008009"))
     assert(bd2 + bd3 + bd4 + bd5 + bd6 + bd7 + bd8 + bd9 == scala.math.BigDecimal("2003004005006007008009.0002003004005006007008009"))
     assert(bd1 + bd2 + bd3 + bd4 + bd5 + bd6 + bd7 + bd8 + bd9 == scala.math.BigDecimal("1002003004005006007008009.1002003004005006007008009"))
     println("BigDecimal works")
  }
}

// Exiting paste mode, now interpreting.

defined object BigDecimalTest

scala> BigDecimalTest.test
BigDecimal works

But Scala 2.13.0 breaks (it will truncate value and reduces scale, hence error):

Welcome to Scala 2.13.0 (OpenJDK 64-Bit Server VM, Java 1.8.0_212).
Type in expressions for evaluation. Or try :help.

scala> :paste
// Entering paste mode (ctrl-D to finish)

object BigDecimalTest {
  def main(args: Array[String]): Unit = {
    test()
  }

  def test(): Unit = {

     val bd1 = scala.math.BigDecimal("1000000000000000000000000.1")
     val bd2 = scala.math.BigDecimal("2000000000000000000000.0002")
     val bd3 = scala.math.BigDecimal("3000000000000000000.0000003")
     val bd4 = scala.math.BigDecimal("4000000000000000.0000000004")
     val bd5 = scala.math.BigDecimal("5000000000000.0000000000005")
     val bd6 = scala.math.BigDecimal("6000000000.0000000000000006")
     val bd7 = scala.math.BigDecimal("7000000.0000000000000000007")
     val bd8 = scala.math.BigDecimal("8000.0000000000000000000008")
     val bd9 = scala.math.BigDecimal("9.0000000000000000000000009")


     assert(bd9  == scala.math.BigDecimal("9.0000000000000000000000009"))
     assert(bd8 + bd9 == scala.math.BigDecimal("8009.0000000000000000000008009"))
     assert(bd7 + bd8 + bd9 == scala.math.BigDecimal("7008009.0000000000000000007008009"))
     assert(bd6 + bd7 + bd8 + bd9 == scala.math.BigDecimal("6007008009.0000000000000006007008009"))
     assert(bd5 + bd6 + bd7 + bd8 + bd9 == scala.math.BigDecimal("5006007008009.0000000000005006007008009"))
     assert(bd4 + bd5 + bd6 + bd7 + bd8 + bd9 == scala.math.BigDecimal("4005006007008009.0000000004005006007008009"))
     assert(bd3 + bd4 + bd5 + bd6 + bd7 + bd8 + bd9 == scala.math.BigDecimal("3004005006007008009.0000003004005006007008009"))
     assert(bd2 + bd3 + bd4 + bd5 + bd6 + bd7 + bd8 + bd9 == scala.math.BigDecimal("2003004005006007008009.0002003004005006007008009"))
     assert(bd1 + bd2 + bd3 + bd4 + bd5 + bd6 + bd7 + bd8 + bd9 == scala.math.BigDecimal("1002003004005006007008009.1002003004005006007008009"))
     println("BigDecimal works")
  }
}

// Exiting paste mode, now interpreting.

defined object BigDecimalTest

scala> BigDecimalTest.test
java.lang.AssertionError: assertion failed
  at scala.Predef$.assert(Predef.scala:267)
  at BigDecimalTest$.test(<console>:22)
  ... 28 elided

It breaks first time on on step bd6 + bd7 + bd8 + bd9 because resulting value has reduced scale and value.

@35VLG84 35VLG84 changed the title Regression: BigDecimal is broken on 2.13, works on 2.12.8 Regression: BigDecimal is broken on 2.13.0, works on 2.12.8 Jun 25, 2019
@dwijnand dwijnand added the library label Jun 25, 2019
@dwijnand dwijnand added this to the 2.13.1 milestone Jun 25, 2019
@dwijnand dwijnand added the regression label Jun 25, 2019
@plokhotnyuk

This comment has been minimized.

Copy link

@plokhotnyuk plokhotnyuk commented Jun 25, 2019

I would say that it is broken in 2.12.8 because it doesn't take in account a math context with rounding precision and rules, which is MathContext.DECIMAL128 by default.

@35VLG84

This comment has been minimized.

Copy link
Author

@35VLG84 35VLG84 commented Jun 25, 2019

Test case works also with 2.11.12. So even if 2.11 and 2.12 are broken in sense of math context, this is change in default behaviour and could break subtle way code with numerical calculations.

35VLG84 added a commit to e257-fi/tackler that referenced this issue Jun 25, 2019
There is behaviour change with Scala 2.13.0:
scala/bug#11590

Signed-off-by: 35V LG84 <35vlg84-x4e6b92@e257.fi>
@NthPortal

This comment has been minimized.

Copy link

@NthPortal NthPortal commented Jun 25, 2019

possibly related: scala/scala#6884, scala/scala#7670

@plokhotnyuk

This comment has been minimized.

Copy link

@plokhotnyuk plokhotnyuk commented Jun 25, 2019

@ 35VLG84 it is broken behaviour (in both 2.11 and 2.12 versions) that was ignored until 2.13...

@dwijnand

This comment has been minimized.

Copy link
Member

@dwijnand dwijnand commented Jun 25, 2019

@plokhotnyuk What changes can a user do to their code to maintain the behaviour in 2.11/2.12 (even if it's technically broken)? Is there some import or something they can add at all use-sites of BigDecimal?

@plokhotnyuk

This comment has been minimized.

Copy link

@plokhotnyuk plokhotnyuk commented Jun 25, 2019

Need just to use MathContext.UNLIMITED for operations that were fixed in mentioned bug fixes above.

@dwijnand

This comment has been minimized.

Copy link
Member

@dwijnand dwijnand commented Jun 25, 2019

For clarity: that's an optional constructor parameter of BigDecimal.

I'm happy to close this issue as "not a bug", having provided that detail for any users needing to mitigate the fix in 2.13.0.

@NthPortal

This comment has been minimized.

Copy link

@NthPortal NthPortal commented Jun 25, 2019

There has been talk of redesigning it so that the MathContext is an implicit passed to operations rather than a field of the class, but that's not guaranteed to ever happen, and if it does it will not happen before 2.14

@Ichoran

This comment has been minimized.

Copy link

@Ichoran Ichoran commented Jun 25, 2019

BigDecimal is conceptually broken as it carries along a MathContext which isn't used symmetrically for symmetric operations (i.e. a + b can be different from b + a due to the (hidden) context of a and b). I think for 2.14 we should just throw it out entirely and rework it from scratch, paying careful attention to (1) mathematical correctness; (2) flexibility in cases where there is more than one sensible choice; and (3) performance.

If we can find a set of symmetrizing operations for MathContext that don't have disastrous behavior (e.g. requires 2^n space after n operations), maybe we can leave it with the same name. Otherwise we should choose something else to make it clear it works differently (e.g. BigReal).

So we can label this as "not a bug", I guess, but the entire design is kind of a bug. Everything relating to the current design forces tradeoffs that result in surprising behavior like the above.

@dwijnand

This comment has been minimized.

Copy link
Member

@dwijnand dwijnand commented Jun 25, 2019

Sounds like a good plan to me.

Closing off this issue, meanwhile.

Do you want to straight open that as an issue here, or go via contributors.scala-lang.org first?

@dwijnand dwijnand closed this Jun 25, 2019
@35VLG84

This comment has been minimized.

Copy link
Author

@35VLG84 35VLG84 commented Jun 25, 2019

There is second problem with sum:

Welcome to Scala 2.13.0 (OpenJDK 64-Bit Server VM, Java 1.8.0_212).
Type in expressions for evaluation. Or try :help.

scala> :paste
// Entering paste mode (ctrl-D to finish)

import java.math.MathContext

object BigDecimalTest {
  def main(args: Array[String]): Unit = {
    test()
  }

  def test(): Unit = {

     val bds = List(
     scala.math.BigDecimal("1000000000000000000000000.1", MathContext.UNLIMITED),
     scala.math.BigDecimal("9.0000000000000000000000009", MathContext.UNLIMITED))

     assert(bds.sum == scala.math.BigDecimal("1000000000000000000000009.1000000000000000000000009", MathContext.UNLIMITED))

     // Below line works with scala 2.13.0
     //assert(bds.foldLeft(BigDecimal(0, MathContext.UNLIMITED))(_ + _) == scala.math.BigDecimal("1000000000000000000000009.1000000000000000000000009", MathContext.UNLIMITED))
     println("BigDecimal works")
  }
}

// Exiting paste mode, now interpreting.

import java.math.MathContext
defined object BigDecimalTest

scala> BigDecimalTest.test
java.lang.AssertionError: assertion failed
  at scala.Predef$.assert(Predef.scala:267)
  at BigDecimalTest$.test(<console>:14)
  ... 28 elided

Which is probably caused by wrong math context with num.zero in here: https://github.com/scala/scala/blob/6b4d32c3f518d21a798e8d3cf4a8c35866afa8e2/src/library/scala/collection/IterableOnce.scala#L915

I can't stress enough that this change with 2.13 is highly surprising, even though orinal implementation with 2.11 and 2.12 has been broken.

P.S. Above code works with 2.12.8

@Ichoran

This comment has been minimized.

Copy link

@Ichoran Ichoran commented Jun 25, 2019

Yeah, arguably the sum implementation should be reduceOption(_ + _).getOrElse(zero) (and similarly for product). This is something we can fix now.

@dwijnand

This comment has been minimized.

Copy link
Member

@dwijnand dwijnand commented Jun 26, 2019

There is second problem with sum:

Created #11592 for that. Thanks for the report, @35VLG84

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.