Convert BigInt/Decimal to expected types for packed numbers #1391
Conversation
daffodil-test/src/test/resources/org/apache/daffodil/section13/packed/packed.tdml
Outdated
Show resolved
Hide resolved
...dil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala
Show resolved
Hide resolved
...dil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala
Outdated
Show resolved
Hide resolved
...dil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala
Show resolved
Hide resolved
...dil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala
Show resolved
Hide resolved
...dil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala
Show resolved
Hide resolved
daffodil-test/src/test/scala/org/apache/daffodil/section13/packed/TestPacked.scala
Outdated
Show resolved
Hide resolved
daffodil-test/src/test/resources/org/apache/daffodil/section13/packed/packed.tdml
Outdated
Show resolved
Hide resolved
26912c9 to
416bf4f
Compare
8b0b392 to
9b0b50d
Compare
|
I think I've addressed all the comments from review now. I have now rebased onto main, which now includes the fixes from #1337. Definitely was a mistake to rebase onto that earlier before it was merged. |
...dil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/PackedBinaryTraits.scala
Outdated
Show resolved
Hide resolved
Packed numbers had been represented internally as BigInt/Decimal. This could cause issues when the values were referenced as part of an expression and would lead to class conversion errors. This commit parses the packed numbers into their expected numeric type. DAFFODIL-2961
9b0b50d to
678d16b
Compare
| override def toBigInteger(num: Array[Byte]): JBigInteger = DecimalUtils.bcdToBigInteger(num) | ||
| override def toBigDecimal(num: Array[Byte], scale: Int): JBigDecimal = | ||
| DecimalUtils.bcdToBigDecimal(num, scale) | ||
| override def toNumber(num: Array[Byte]): JBigInteger = DecimalUtils.bcdToBigInteger(num) |
There was a problem hiding this comment.
Here the return type is different. toNumber returns a JBigInteger.
I'm surprised this even works. JBigInteger must be a sub-type of JBigDecimal.
| override def toBigInteger(num: Array[Byte]): JBigInteger = DecimalUtils.bcdToBigInteger(num) | ||
| override def toBigDecimal(num: Array[Byte], scale: Int): JBigDecimal = | ||
| DecimalUtils.bcdToBigDecimal(num, scale) | ||
| override def toNumber(num: Array[Byte]): JBigDecimal = |
There was a problem hiding this comment.
Why change this to toNumber? The return type is not JNumber.
Is there scaladoc on the toNumber method? There needs to be to explain this naming convention. I don't really understand why this got renamed.
There was a problem hiding this comment.
The trait these classes mix in requires an implementation of def toNumber(num: Array[Byte]): A, where A <: Number. We could remove the A type parameter and just make it so it always returns a JNumber, but then implementations have to be a bit more careful to return the right thing.
As it is right now, the type parameter ensures the implementations of toNumber return the right kind of JNumber (some should always return a BigInteger, some should always return a BigDecimal). I'm not sure the type parameter is used anywhere else though, so maybe that's overkill and just adds unnecessary complication?
There was a problem hiding this comment.
I think the type parameter is almost certainly the right thing, I just don't see a change that adds this toNumber super method declaration, yet it's not in the current Daffodil main branch, so when did this type parameter get added?
But what is the purpose of the toNumber method? I think it is "convert to a number type we can store in the Infoset object. This should be the most specific meaningful subtype of JNumber" that is used in the Infoset object representation. Or something like that.
There was a problem hiding this comment.
The top level declaration of toNumber is in PackedBinaryTraits.scala where it is simply declared as
trait PackedBinaryConversion[A <: Number] {
def toNumber(num: Array[Byte]): A
....
The actual implementations are in the classes that extend the PackedBinaryConversion trait, like BCDIntegerKnownLengthParser.
The function will only generate either BigInteger's or BigDecimal's depending on the calling classes type, so perhaps a better name for the function would be "toBigNumber". The BigNumber then gets converted to the appropriate PrimType by calling PrimNumberic.fromNumber(bigNumber). This call is made by the toPrimType function of the same trait.
|
I've made another small change that I had forgotten about. We don't want to check the bit length of binary numbers during schema compilation if the binaryNumberRep is not "binary" as it can take more than 4 bytes to represent an xs:int for example. Please double check that this simple extra conditional is a reasonable solution to this. |
| <tdml:error>Error in packed data</tdml:error> | ||
| <tdml:error>out of range for type</tdml:error> | ||
| </tdml:errors> | ||
| </tdml:parserTestCase> |
There was a problem hiding this comment.
Can we add a positive test case for this? To show that you can have a large amount of bytes for packed values as long as the result is within range of the type.
Still have an edge case for packed xs:date/Time values require an actual BigInt, but this fixes DAFFODIL-2961