Convert BigInt/Decimal to expected types for packed numbers

[jadams-tresys]

Still have an edge case for packed xs:date/Time values require an actual BigInt, but this fixes DAFFODIL-2961

[stevedlawrence]

Can you remove "error"/"err" from the schema and test name. I think in general that implies it is a negative test case. Maybe use "expr" instead to make it clear that this is about using packed values in expressions?

[stevedlawrence]

It looks like when we always use the packed integer parsers for dealing with date/times, so should this be an Assert.impossible() case?

[jadams-tresys]

I believe that is correct. Will change.

[stevedlawrence]

I wonder if it's worth doing a little more refactoring. I say this because something that implements PackedBinaryConversion only ever uses one of toBigInteger/toInteger or toBigDecimal/toDecimal. So it feels a bit odd to require both be implemented. And potentially unsafe--an implementation could accidentally use the wrong function.

What if instead we remove PackedBinaryConversion altogether, and PackedBinaryDecimalBaseParser has the abstract definition of toBigDecimal, and PackedBinaryIntegerBaseParser has the abstract definition for toBigInteger. Then these new toInteger and toDecimal functions go and the logic just moved into the the appropriate parser trait.

[jadams-tresys]

The issue here is that we would still need to duplicate everything in the PackedBinaryDecimal/IntegerDelimitedBaseParsers as well. I think that was the reason for the conversion trait to begin with. The DelimitedBaseParsers do not share anything with the standard BaseParsers.

[stevedlawrence]

PackedIntegerDelimitedParser only uses toBigInteger and PackedDecimalDelimitedParser only uses toBigDecimal, so these two classes do need an abstract toBigInteger/toBigDecimal respectively, but they shouldn't need both that the PackedBinaryConversion trait requires.

[jadams-tresys]

Wouldn't the delimited parsers need the same conversion code to avoid the error this bug was fixing where a value gets used in an expression?

[stevedlawrence]

Ah right, maybe right refactoring is more like this?

 // returns either BigDecimal or BigInteger depending on the implementation
def toBigNumber(num: Array[Byte]): Number 

def toPrimType(...) : DataValueNumber = {
  val bigNumber = toBigNumber
  context.optPrimType.get match {
    case pn: PrimType.PrimNumeric => pn.fromNumber(bigNmber)
    case => bigNumber
  }
}

Then all the individual parses just have to implement toBigNumber correctly, and the abstract base classes just call toPrimType. Might even be renaming it and changing it to also handle exceptions and call setDataValue to avoid more duplication.

[jadams-tresys]

Just making sure that I'm on the same page. Is that not what I did below (although they are called toInter/Decimal instead of toPrimType)? I recognize that I need to add some exception handling for potential InvalidPrimitiveDataExceptions. I think it also makes more sense to me to keep the calls to actually set the value inside the parse functions instead of moving them inside the toPrimType trait function.

[stevedlawrence]

I think the main difference I'm suggesting is making the trait more generic so implementations don't have to implement both toBigInteger and toBigDecimal, they just implement a single toBigNumber and that will return the BigDecimal or BigInteger depending on if it's an decimal or integer type of parser. To ensure correct types, we could define it something like this:

trait PackedBinaryConversion[A <: Number] {
  def toNumber(num: Array[Byte]): A

  def toPrimType(...) = { ... toNumber(...)  }
}

And then usages would provide the right type parameter, for example:

abstract class PackedBinaryIntegerDelimitedBaseParser(...)
  ...
  with PackedBinaryConversion[JBigInteger]

...

abstract class PackedBinaryIntegerDelimitedDecimalParser(...)
  ...
  with PackedBinaryConversion[JBigDecimal]

Avoids duplicating the decimal/integer logic in the PackedBinaryConversion trait.

I think it also makes more sense to me to keep the calls to actually set the value inside the parse functions instead of moving them inside the toPrimType trait function.

I have no strong preference either way. You just have to duplicate error handling logic if it's not done in the shared trait.

[jadams-tresys]

Is there a clean way to have a single toNumber function while also dealing with the scale parameter that is needed for Decimal? I thought about defaulting scale to 0 and treating scale=0 as a BigInt, but scale=0 is valid for BigDecimal as well.

You just have to duplicate error handling logic if it's not done in the shared trait.
If I have it in the trait I would then need to pass that current state in as well to create the Parse Error. Since the PackedBinaryConversion trait I wasn't sure if there was a way to pass in either a Parse state or an Unparse state that could create errors appropriately.

I'm pushing up some changes now that I think cover everything else pretty well.

[stevedlawrence]

Could you drop the scale parameter and get it from the class member, e.g.

class BCDDecimalKnownLengthParser(..., binaryDecimalVirtualPoint: Int)
  extends ... {

  override def toNumber(num: Array[Byte]): JBigDecimal =
    DecimalUtils.bcdToBigDecimal(num, binaryDecimalVirtualPoint)
}

I think this actually makes is actually an improvement over what we have now, since the abstract class shouldn't need to know anything about how the implementations converts a byte array to a Number, all the information like scale, sign codes, etc. is private to the specific packed binary number implementation. And if we ever need some new parameter on how a byte array is converted to a Number it can be done specifically in the implementation and the base class doesn't have to change at all.

[jadams-tresys]

I've implemented it as you described now. It is much cleaner now I think. Unfortunately rebasing onto Lola's branch has really muddied up the diff when it comes to review.

[stevedlawrence]

We should probably hold off until #1337 is merged, since that touches some of this code and is the bigger commit, easier to deal with conflicts with the smaller commit.

[stevedlawrence]

Can you add some out of range tests as well? That's a new limitation that this will enforce that I think wasn't previously enforced.

[mbeckerle]

+1
I don't have much to add beyond finding one typo.

[mbeckerle]

usign => unsigned