You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "stevedlawrence (via GitHub)" <gi...@apache.org> on 2023/01/25 17:44:31 UTC
[GitHub] [daffodil] stevedlawrence opened a new pull request, #934: Fix handling of NaN/Infinifty when parsing text integers
stevedlawrence opened a new pull request, #934:
URL: https://github.com/apache/daffodil/pull/934
When parsing text integers, ICU still attempts to parse NaN and infinty representations and returns Double.NaN, Double.POSITIVE_INFINITY, or Double.NEGATIVE_INFINITY if matched. There is no way to disable this behavior in ICU.
So this modifies the ConvertTextStandardNumberParser to throw a PE when the prim type is an integer and ICU returns a Double, which means it must have parsed an NaN/Inf which is not considered valid by DFDL.
This also adds a new isInteger method to PrimNumeric to more easily determine if a numeric primitive type is an integer without having to do a match-case on types, and uses this new method in a few places.
Also modifies PrimNumericInteger so that doubles and floats that are NaN or infinite are not considered valid for integer types. This ensures we error instead of converting to zero, such as when used in expressions or reading values from an infoset.
Also adds an ICU specific test to detect if ICU ever changes this behavior.
Note that the original bug expects an SDE, but this is not correct. When and integer fails to parse because NaN or INF is the value, it should be a PE. It is also a PE if a length expression tries to convet a double NaN/INF to a long. It is only an SDE if the length value returned is negative. Tests are aded to verify this behavior.
[DAFFODIL-908](https://issues.apache.org/jira/browse/DAFFODIL-908)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on a diff in pull request #934: Fix handling of NaN/Infinifty when parsing text integers
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #934:
URL: https://github.com/apache/daffodil/pull/934#discussion_r1087012684
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/ConvertTextStandardNumberParser.scala:
##########
@@ -173,27 +173,32 @@ case class ConvertTextStandardNumberParser(
val df = textNumberFormatEv.evaluate(start).get
val strCheckPolicy = if (df.isParseStrict) str else str.trim
val pos = new ParsePosition(0)
- val icuNum: Number = df.parse(strCheckPolicy, pos)
-
-
- if (icuNum == null) {
- PE(start, "Unable to parse %s from text: %s",
- context.optPrimType.get.globalQName, str)
- return
- }
-
- // sometimes ICU will return their own custom BigDecimal, even if the
- // value could be represented as a BigInteger. We only want Java types,
- // so detect this and convert it to the appropriate type
- val num1 = icuNum match {
+ val icuNum: Number = df.parse(strCheckPolicy, pos) match {
+ case null => {
+ PE(start, "Unable to parse %s from text: %s", context.optPrimType.get.globalQName, str)
+ return
+ }
+ case d: JDouble if primNumeric.isInteger => {
+ // If ICU returns a Double when only integers are expected, it means the
+ // string must have been NaN, -Infinity, or Infinity using the locales
+ // default symbols. There does not seem to be a way to disable this even
+ // with setParseBigDecimal to false and setParseIntegerOnly set to true.
+ // So just create the same PE as if it failed to parse it, which is what
+ // we really want ICU to do
+ Assert.invariant(d.isNaN || d.isInfinite)
+ PE(start, "Unable to parse %s from text: %s", context.optPrimType.get.globalQName, str)
+ return
+ }
case bd: ICUBigDecimal => {
+ // sometimes ICU will return their own custom BigDecimal, even if the
+ // value could be represented as a BigInteger. We only want Java types,
+ // so detect this and convert it to the appropriate type
if (bd.scale == 0) bd.unscaledValue
else bd.toBigDecimal
}
- case _ => icuNum
+ case num => num
Review Comment:
Can we use `case num: Number => num` to be clearer or would that cause Scala to complain that a default case is still missing?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #934: Fix handling of NaN/Infinifty when parsing text integers
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #934:
URL: https://github.com/apache/daffodil/pull/934#discussion_r1087024338
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/ConvertTextStandardNumberParser.scala:
##########
@@ -173,27 +173,32 @@ case class ConvertTextStandardNumberParser(
val df = textNumberFormatEv.evaluate(start).get
val strCheckPolicy = if (df.isParseStrict) str else str.trim
val pos = new ParsePosition(0)
- val icuNum: Number = df.parse(strCheckPolicy, pos)
-
-
- if (icuNum == null) {
- PE(start, "Unable to parse %s from text: %s",
- context.optPrimType.get.globalQName, str)
- return
- }
-
- // sometimes ICU will return their own custom BigDecimal, even if the
- // value could be represented as a BigInteger. We only want Java types,
- // so detect this and convert it to the appropriate type
- val num1 = icuNum match {
+ val icuNum: Number = df.parse(strCheckPolicy, pos) match {
+ case null => {
+ PE(start, "Unable to parse %s from text: %s", context.optPrimType.get.globalQName, str)
+ return
+ }
+ case d: JDouble if primNumeric.isInteger => {
+ // If ICU returns a Double when only integers are expected, it means the
+ // string must have been NaN, -Infinity, or Infinity using the locales
+ // default symbols. There does not seem to be a way to disable this even
+ // with setParseBigDecimal to false and setParseIntegerOnly set to true.
+ // So just create the same PE as if it failed to parse it, which is what
+ // we really want ICU to do
+ Assert.invariant(d.isNaN || d.isInfinite)
+ PE(start, "Unable to parse %s from text: %s", context.optPrimType.get.globalQName, str)
+ return
+ }
case bd: ICUBigDecimal => {
+ // sometimes ICU will return their own custom BigDecimal, even if the
+ // value could be represented as a BigInteger. We only want Java types,
+ // so detect this and convert it to the appropriate type
if (bd.scale == 0) bd.unscaledValue
else bd.toBigDecimal
}
- case _ => icuNum
+ case num => num
Review Comment:
Scala has no problem with it and I guess is able to detect all cases are covered. Will add.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence merged pull request #934: Fix handling of NaN/Infinifty when parsing text integers
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence merged PR #934:
URL: https://github.com/apache/daffodil/pull/934
--
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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org