-
Notifications
You must be signed in to change notification settings - Fork 72
Correct numeric operations #1571
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
Correct numeric operations #1571
Conversation
mbeckerle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 This simplifies things and is well commented. I suggested one comment mod where it didn't read right to me.
| * that leads to this error. For example, the expression "xs:int(1) div xs:double(.75)" | ||
| * promotes both args to xs:decimal, and that division leads to non-terminating | ||
| * decimal expansions. It is likely too difficult to require users to round these | ||
| * basic expressions, so we should make a decision on how to handle this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment wording suggests an open TODO, ie., thta this code does not make a decision about this.
I think the code does have a specific decision below (34 digits), so reword to reference that.
olabusayoT
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
The current logic of numeric operations using in DFDL expressions is not correct in a couple of ways First, numeric operations using shorts and bytes are just broken and can least to ClassCastExceptions. The main reason is that Java promotes short and byte operations to ints but our logic did not account for that, so data values had ints when we expected short/byte. Second, the currently logic does not correctly implement the XPath 2.0 specification for numeric operations. For example, if two arguments have a least upper bound of SignedNumeric we cast them to Double. An example where this breaks if xs:double and xs:decimal, which has a least upper bound of SignedNumeric. These should should be promoted to Decimal instead of Double. We also did not handle underflow/overflow correctly. For example, xs:long(xs:byte(255) + xs:byte(1)) results in an out of range error rather than correctly returning xs:long(256). This changes our numeric operations fix these problems and match the XPath 2.0 spec. To achieve this we first promote numeric operands to one of decimal, integer, double, float, or long, based on the least upper bound of the operand types. Java does not perform any type promotion with these, so it avoids accidental changes of expected types. Note that we long where possible to maintain performant operations, rather than simply promoting everything to xs:integer as implied by the XPath 2.0 specification. This does mean underflow/overflow is possible, but this is much less likely. Note that such overflow will not be detected in intermediate operations, but will be detect if the result does not fit withing the range of the target type. This also changes the results of div and idv operations to return Decimal/Float/Double or Integer/Long for similar reasons. This means many of the NumericOps can not be removed (e.g. PlusShort) since the set of types we perform operations on is much smaller. We also update the NumericOps implementations to use DataValue's, since we know exactly what the types should be, rather than relying on asFoo to convert the operand to the expected type. This also changes the resulting type of numeric if-branches. This are only promoted to the least upper bound rather than also doing the numeric operation conversions. The numeric results can will be promoted if then used in a numeric operation. DAFFODIL-2574
ead205d to
0b147c0
Compare
The current logic of numeric operations using in DFDL expressions is not correct in a couple of ways
First, numeric operations using shorts and bytes are just broken and can least to ClassCastExceptions. The main reason is that Java promotes short and byte operations to ints but our logic did not account for that, so data values had ints when we expected short/byte.
Second, the currently logic does not correctly implement the XPath 2.0 specification for numeric operations. For example, if two arguments have a least upper bound of SignedNumeric we cast them to Double. An example where this breaks if xs:double and xs:decimal, which has a least upper bound of SignedNumeric. These should should be promoted to Decimal instead of Double. We also did not handle underflow/overflow correctly. For example, xs:long(xs:byte(255) + xs:byte(1)) results in an out of range error rather than correctly returning xs:long(256).
This changes our numeric operations fix these problems and match the XPath 2.0 spec.
To achieve this we first promote numeric operands to one of decimal, integer, double, float, or long, based on the least upper bound of the operand types. Java does not perform any type promotion with these, so it avoids accidental changes of expected types. Note that we long where possible to maintain performant operations, rather than simply promoting everything to xs:integer as implied by the XPath 2.0 specification. This does mean underflow/overflow is possible, but this is much less likely. Note that such overflow will not be detected in intermediate operations, but will be detect if the result does not fit withing the range of the target type.
This also changes the results of div and idv operations to return Decimal/Float/Double or Integer/Long for similar reasons.
This means many of the NumericOps can not be removed (e.g. PlusShort) since the set of types we perform operations on is much smaller. We also update the NumericOps implementations to use DataValue's, since we know exactly what the types should be, rather than relying on asFoo to convert the operand to the expected type.
This also changes the resulting type of numeric if-branches. This are only promoted to the least upper bound rather than also doing the numeric operation conversions. The numeric results can will be promoted if then used in a numeric operation.
DAFFODIL-2574