mbeckerle commented on code in PR #1133:
URL: https://github.com/apache/daffodil/pull/1133#discussion_r1440823520
##########
daffodil-test/src/test/resources/org/apache/daffodil/section05/simple_types/SimpleTypes.tdml:
##########
@@ -3722,10 +3721,10 @@
</tdml:infoset>
</tdml:parserTestCase>
- <tdml:parserTestCase name="float_text_delim" root="floatTextDelim"
+ <tdml:parserTestCase name="float_text_max" root="floatTextDelim"
model="SimpleTypes-Embedded.dfdl.xsd"
description="Section 5 Simple type-float - DFDL-5-008R">
- <tdml:document><![CDATA[3.4028235E38]]></tdml:document>
+
<tdml:document><![CDATA[340,282,350,000,000,000,000,000,000,000,000,000,000]]></tdml:document>
Review Comment:
I had to stare at this a while and go look at GeneralFormatBase to decide
this is ok.
Insert a comment that this is 39 digits long because the textNumberPattern
is "#,###.###" which doesn't allow for use of an exponent, so it has to output
all the integer digits.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -681,6 +687,8 @@ object NodeInfo extends Enum {
protected override def fromNumberNoCheck(n: Number): DataValueFloat =
n.floatValue
override val min = -JFloat.MAX_VALUE.doubleValue
override val max = JFloat.MAX_VALUE.doubleValue
+ override val minStr = "-" + JFloat.MAX_VALUE.toString
Review Comment:
See comment below about minStr
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/ConvertTextStandardNumberUnparser.scala:
##########
@@ -86,7 +89,22 @@ case class ConvertTextNumberUnparser(
case n: JNumber if zeroRep.isDefined && isZero(n) => zeroRep.get
case _ =>
try {
- df.format(scaledValue)
+ if (
+ (context.optPrimType.get eq NodeInfo.Float) &&
+ JFloat.isFinite(scaledValue.asInstanceOf[Float])
+ ) {
+ // ICU4J has has no format() function that handles a float.
Instead, ICU4J converts
+ // the float to a double, and formats that value. However,
formatting a double as a
+ // string has different precision than formatting a float as a
string, resulting in
+ // extra precision that implies more accuracy than actually exists
for floats, and
+ // can also lead to failures to exactly round. To solve this, we
convert the float
+ // to a String, convert that String to a BigDecimal, and then
format that. This is
+ // more expensive, but ensures we unparse the exact same precision
as represented by
+ // the float.
Review Comment:
I don't get it. Does constructing BigDecimal(string) work if the string uses
exponential notation as in "3.4E38" ?
If so why didn't they provide a constructor that takes a float?
I guess it has to work, or your tests would be failing.
We could bypass all of this if the float is an integer because none of this
is required if it's an integer. The issue with double conversion only matters
if there are real non-zero fraction digits. That said, I think the only way to
tell if a float is an integer is to round it to one and see if it is equal.
Nah. Not worth it. Plus I am not sure the interactions with the number
pattern "@" character stuff, etc.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -698,6 +706,8 @@ object NodeInfo extends Enum {
protected override def fromNumberNoCheck(n: Number): DataValueDouble =
n.doubleValue
override val min = -JDouble.MAX_VALUE
override val max = JDouble.MAX_VALUE
+ override val minStr = "-" + JDouble.MAX_VALUE.toString
Review Comment:
Is there a reason these aren't just min.toString and max.toString? The fact
that they are not suggests to me something funny is going on that requires us
not to call toString on the min because it's negated or something.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
