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 2025/11/07 11:31:12 UTC
[PR] Remove workaround for ICU-22303, fixed in ICU 78.1 [daffodil]
stevedlawrence opened a new pull request, #1587:
URL: https://github.com/apache/daffodil/pull/1587
ICU 77.1 and older had a bug where it would not correctly parse infinity or NaN if the text pattern had a decimal and it was required to match a decimal point (which is enabled with strict parse policy). To workaround this bug, we added logic to parse a number second time if the first failed, but with different settings that did not require the decimal point, which allows it to detect infinity/NaN. But this adds extra overhead and complexity.
ICU 78.1 fixed this bug, so we can now remove this workaround and avoid the overhead related to a second parse. This also updates the test that was added to detect if this bug was fixed, which now detects if ICU has a regression and breaks the fix.
DAFFODIL-2985
--
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
Re: [PR] Remove workaround for ICU-22303, fixed in ICU 78.1 [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence merged PR #1587:
URL: https://github.com/apache/daffodil/pull/1587
--
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
Re: [PR] Remove workaround for ICU-22303, fixed in ICU 78.1 [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1587:
URL: https://github.com/apache/daffodil/pull/1587#discussion_r2503667188
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/ConvertTextStandardNumberParser.scala:
##########
@@ -192,43 +192,13 @@ case class ConvertTextStandardNumberParser(
val pos = new ParsePosition(0)
val icuNum: JNumber = df.parse(strToParse, pos) match {
case null => {
- val infNaN: JDouble =
- if (df.isDecimalPatternMatchRequired) {
- // ICU failed to parse. But there is a bug in ICU4J (ICU-22303) that if there is
- // a decimal in the pattern and we've set that decimal to be required (due to
- // strict mode), then it will fail to parse Inf/NaN representations. As a
- // workaround, we clone the DecimalFormat, disable requiring the decimal, and
- // reparse. We only accept successful Inf/NaN parses though--everything else is
- // considered a parse error since it meant the decimal point was missing or
- // wasn't either inf/nan or a valid number. If ICU fixes this bug, we should
- // remove this infNan variable and its use, as it is likely pretty expensive to
- // clone, change a setting, and reparse. Fortunately, it is only in the error
- // case of strict parsing so should be rare.
- pos.setIndex(0)
- val newDF = df.clone().asInstanceOf[DecimalFormat]
- newDF.setDecimalPatternMatchRequired(false)
- newDF.parse(strToParse, pos) match {
- case d: JDouble => {
- Assert.invariant(d.isNaN || d.isInfinite)
- d
- }
- case _ => null
- }
- } else {
- null
- }
-
- if (infNaN != null) {
- infNaN
- } else {
- PE(
- start,
- "Unable to parse %s from text: %s",
- context.optPrimType.get.globalQName,
- str
- )
- return
- }
+ PE(
Review Comment:
ICU DecimalFormatSymbols has support for infinity and Nan.
[DecimalFormatSymbols.setInfinity](https://unicode-org.github.io/icu-docs/apidoc/dev/icu4j/com/ibm/icu/text/DecimalFormatSymbols.html#setInfinity(java.lang.String))
[DecimalFormatSymbols.setNaN](https://unicode-org.github.io/icu-docs/apidoc/dev/icu4j/com/ibm/icu/text/DecimalFormatSymbols.html#setNaN(java.lang.String))
We call those when we create the DecimalFormat. I'll add a comment to make this clear.
--
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
Re: [PR] Remove workaround for ICU-22303, fixed in ICU 78.1 [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on PR #1587:
URL: https://github.com/apache/daffodil/pull/1587#issuecomment-3502089902
Here is the dependency upgrade checklist:
[ICU 78 release notes](https://unicode-org.github.io/icu/download/78.html)
[ICU 77 diff](https://github.com/unicode-org/icu/compare/release-77-1...release-78.1)
- [X] **Do all automated continuous integration checks pass?**
Yes, with one test updated due to ICU bug fix
- [X] **Is the update a patch, minor, or major update?**
Major release, updates to Unicode and CLDR, plus bug fixes/refinements. Drops Java 8 support, but we now already require Java 17+
- [X] **Is the license still compatible with ASF License Policy?**
Yes, still Unicode License v3
- [X] **Have any changes been made to LICENSE/NOTICE files that need to be incorporated?**
No
- [X] **Have any transitive dependencies been added or changed?**
No transitive dependencies
--
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
Re: [PR] Remove workaround for ICU-22303, fixed in ICU 78.1 [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1587:
URL: https://github.com/apache/daffodil/pull/1587#discussion_r2503627644
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/ConvertTextStandardNumberParser.scala:
##########
@@ -192,43 +192,13 @@ case class ConvertTextStandardNumberParser(
val pos = new ParsePosition(0)
val icuNum: JNumber = df.parse(strToParse, pos) match {
case null => {
- val infNaN: JDouble =
- if (df.isDecimalPatternMatchRequired) {
- // ICU failed to parse. But there is a bug in ICU4J (ICU-22303) that if there is
- // a decimal in the pattern and we've set that decimal to be required (due to
- // strict mode), then it will fail to parse Inf/NaN representations. As a
- // workaround, we clone the DecimalFormat, disable requiring the decimal, and
- // reparse. We only accept successful Inf/NaN parses though--everything else is
- // considered a parse error since it meant the decimal point was missing or
- // wasn't either inf/nan or a valid number. If ICU fixes this bug, we should
- // remove this infNan variable and its use, as it is likely pretty expensive to
- // clone, change a setting, and reparse. Fortunately, it is only in the error
- // case of strict parsing so should be rare.
- pos.setIndex(0)
- val newDF = df.clone().asInstanceOf[DecimalFormat]
- newDF.setDecimalPatternMatchRequired(false)
- newDF.parse(strToParse, pos) match {
- case d: JDouble => {
- Assert.invariant(d.isNaN || d.isInfinite)
- d
- }
- case _ => null
- }
- } else {
- null
- }
-
- if (infNaN != null) {
- infNaN
- } else {
- PE(
- start,
- "Unable to parse %s from text: %s",
- context.optPrimType.get.globalQName,
- str
- )
- return
- }
+ PE(
Review Comment:
I was surprised to see that this code doesn't have to deal with textStandardInfinityRep nor textStandardNaNRep.
This code needs a one-line comment as to where those are handled.
--
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