[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #387: DAFFODIL-2349: fixed overflow ArithmeticException when totalDigits >= 10

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #387:
URL: https://github.com/apache/incubator-daffodil/pull/387#discussion_r442195870



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
##########
@@ -455,10 +455,9 @@ final class SimpleTypeRuntimeData(
     // Per http://www.w3.org/TR/xmlschema-2/#rf-totalDigits
     // |i| < 10^totalDigits
 
-    val number = new java.math.BigDecimal(scala.math.pow(10.0, digits.doubleValue()))
-    val biNumber = new java.math.BigInteger(number.intValueExact().toString())
+    val number = new java.math.BigDecimal(scala.math.pow(10, digits)).toBigIntegerExact
     val bdData = diNode.dataValueAsBigDecimal.unscaledValue()
-    val isDataLessThanNumber = bdData.compareTo(biNumber) < 0
+    val isDataLessThanNumber = bdData.compareTo(number) < 0
     isDataLessThanNumber

Review comment:
       This doesn't take into the account that ``bdData`` could be negative. This is an existing bug, but it would be nice to fix as part of this, and I think it just means adding an abs() somewhere.
   
   I'm also thinking that we need to change how we create ``number``. scala.math.pow returns a double, so there could be precision issues.  For example, if totalDigits were 30, this would be
   ```scala
   scala> val number = new java.math.BigDecimal(scala.math.pow(10.0, 30.0))
   res0: number: java.math.BigDecimal = 1000000000000000019884624838656
   ```
   Which is too big and would allow some values that are more than 30 digits. 30 is maybe pretty large for totalDigits and isn't too likely, but it's not out of the realm of possibility. And it looks like precision issues happen around 23. I think maybe just using BigInteger.pow should work e,g.
   ```scala
   val number = java.math.BigInteger.TEN.pow(totalDigits)
   ```
   That doesn't seem to have any precision issues.
   
   Also, I don't think this takes into account the scale of the BigDecimal, it only looks at the unscaledValue. For example,
   ```scala
   scala> (new java.math.BigDecimal("1E2")).unscaledValue
   res0: java.math.BigInteger = 1
   ```
   The total digits for 1E2 is three, but we are only comparing against the unscaledValue of 1, so this would not be caught if totalDigits were 2. I think we also need to take into account the scale or precision, though it's not immediately obvious to me the right way to do that.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org