stevedlawrence commented on code in PR #1685:
URL: https://github.com/apache/daffodil/pull/1685#discussion_r3435569189
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Facets.scala:
##########
@@ -482,20 +482,21 @@ trait Facets { self: Restriction =>
localFacet
}
- private def convertFacetToBigDecimal(facet: String): java.math.BigDecimal = {
- self.primType match {
- case PrimType.DateTime =>
- dateToBigDecimal(
- facet,
- "uuuu-MM-dd'T'HH:mm:ss.SSSSSSxxx",
- PrimType.DateTime.toString(),
- this
- )
- case PrimType.Date =>
- dateToBigDecimal(facet, "uuuu-MM-ddxxx", PrimType.Date.toString(),
this)
- case PrimType.Time =>
- dateToBigDecimal(facet, "HH:mm:ss.SSSSSSxxx",
PrimType.Time.toString(), this)
- case _ => new java.math.BigDecimal(facet)
+ private def convertFacetToBigDecimal(
+ facet: String,
+ facetType: Facet.Type
+ ): java.math.BigDecimal = {
+ try {
+ if (Facet.isLengthFamilyFacet(facetType)) {
+ new java.math.BigDecimal(facet)
+ } else {
+ // value-space facets (min/max Inclusive/Exclusive, enumeration):
+ // parse in the element's own value space
+ calendarStringToBigDecimal(facet, primType, this)
+ }
+ } catch {
+ case e: IllegalArgumentException =>
+ SDE("invalid facet restriction: %s", e.getMessage)
Review Comment:
We should include the facet name in this SDE
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Facets.scala:
##########
@@ -482,20 +482,21 @@ trait Facets { self: Restriction =>
localFacet
}
- private def convertFacetToBigDecimal(facet: String): java.math.BigDecimal = {
- self.primType match {
- case PrimType.DateTime =>
- dateToBigDecimal(
- facet,
- "uuuu-MM-dd'T'HH:mm:ss.SSSSSSxxx",
- PrimType.DateTime.toString(),
- this
- )
- case PrimType.Date =>
- dateToBigDecimal(facet, "uuuu-MM-ddxxx", PrimType.Date.toString(),
this)
- case PrimType.Time =>
- dateToBigDecimal(facet, "HH:mm:ss.SSSSSSxxx",
PrimType.Time.toString(), this)
- case _ => new java.math.BigDecimal(facet)
+ private def convertFacetToBigDecimal(
+ facet: String,
+ facetType: Facet.Type
+ ): java.math.BigDecimal = {
+ try {
+ if (Facet.isLengthFamilyFacet(facetType)) {
+ new java.math.BigDecimal(facet)
+ } else {
+ // value-space facets (min/max Inclusive/Exclusive, enumeration):
+ // parse in the element's own value space
+ calendarStringToBigDecimal(facet, primType, this)
Review Comment:
This needs to take into account the prim type. For example, if the prim Type
was xs:int then this is would try to convert the value to a calendar which will
fail.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/RestrictionUnion.scala:
##########
@@ -394,7 +372,7 @@ sealed trait TypeChecks { self: Restriction =>
theContext.SDE("%s is not a valid Boolean value. Expected 'true' or
'false'.", x)
case (_, _) => {
// Perform conversions once
- val theValue = convertStringToBigDecimal(value, primType, theContext)
+ val theValue = calendarStringToBigDecimal(value, primType, theContext)
Review Comment:
This probably wants to call convertFacetToBigDecimal, this part of the code
can get more than just calendars types.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/PrimitivesDateTime.scala:
##########
@@ -284,36 +278,18 @@ case class ConvertBinaryDateTimeSecMilliPrim(e:
ElementBase, lengthInBits: Long)
protected override val prettyType = "DateTime"
lazy val epochCalendar: Calendar = {
- val cal = Calendar.getInstance
- cal.clear()
- cal.setLenient(false)
-
- val sdfWithTZ = new SimpleDateFormat("uuuu-MM-dd'T'HH:mm:ssZZZZ",
ULocale.ENGLISH)
- var pos = new ParsePosition(0)
- sdfWithTZ.parse(e.binaryCalendarEpoch, cal, pos)
-
- if (pos.getIndex != e.binaryCalendarEpoch.length || pos.getErrorIndex >=
0) {
- // binaryCalendarEpoch didn't match the first format with timezone, so
try without
- cal.clear()
- pos = new ParsePosition(0)
- val sdf = new SimpleDateFormat("uuuu-MM-dd'T'HH:mm:ss", ULocale.ENGLISH)
- cal.setTimeZone(TimeZone.UNKNOWN_ZONE)
- sdf.parse(e.binaryCalendarEpoch, cal, pos)
-
- if (pos.getIndex != e.binaryCalendarEpoch.length || pos.getErrorIndex >=
0) {
- SDE(
- "Failed to parse binaryCalendarEpoch - Format must match the pattern
'uuuu-MM-dd'T'HH:mm:ss' or 'uuuu-MM-dd'T'HH:mm:ssZZZZ'"
- )
- }
- }
-
- try {
- cal.getTime
- } catch {
- case e: IllegalArgumentException => {
- SDE("Failed to parse binaryCalendarEpoch: %s.", e.getMessage())
+ val dfdlDateTime =
+ try {
+ DFDLDateTimeConversion.fromXMLString(e.binaryCalendarEpoch)
+ } catch {
+ case exception: IllegalArgumentException =>
Review Comment:
I think we should probably avoid directly calling DFDL*Conversion functions.
Instead, we can use `NodeInfo.DateTime.fromXMLString(...).getCalendar` and
catch `InvalidPrimitiveDataException`. That way NodeInfo.fromXMLString becomes
the one source of truth for how to convert between XSD primitives and our
internal primities.
Then our DFDLConversion functions just become an implementation detail of
our NodeInfos, and we can easily change how NodeInfo does conversions in the
future without affecting other code.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/RestrictionUnion.scala:
##########
@@ -306,61 +302,43 @@ final class Union private (val xmlArg: Node,
simpleTypeDef: SimpleTypeDefBase)
}
sealed trait TypeChecks { self: Restriction =>
- protected def dateToBigDecimal(
- date: String,
- format: String,
- dateType: String,
- context: ThrowsSDE
- ): JBigDecimal = {
- val df = new SimpleDateFormat(format)
- df.setCalendar(new GregorianCalendar())
- df.setTimeZone(TimeZone.GMT_ZONE)
- val bd =
- try {
- val dt = df.parse(date)
- new JBigDecimal(dt.getTime())
- } catch {
- case s: scala.util.control.ControlThrowable => throw s
- case u: UnsuppressableException => throw u
- case e1: Exception => {
- try {
- // Could already be a BigDecimal
- new JBigDecimal(date)
- } catch {
- case s: scala.util.control.ControlThrowable => throw s
- case u: UnsuppressableException => throw u
- case e2: Exception =>
- context.SDE(
- "Failed to parse (%s) to %s (%s) due to %s (after %s).",
- date,
- dateType,
- format,
- e2.getMessage(),
- e1.getMessage()
- )
- }
- }
- }
- bd
- }
- private def convertStringToBigDecimal(
+ /**
+ * Converts a lexical date/time/dateTime string to its numeric (epoch millis)
+ * BigDecimal, parsing via DFDLCalendarConversion (XSD 1.0, YEAR+ERA) rather
+ * than calendar patterns. Non-calendar types are parsed as plain
BigDecimals.
+ *
+ * If the calendar parse fails, the value is retried as a plain BigDecimal
+ * before giving up with an SDE. This handles the narrowing round-trip where
a
+ * date facet has already been converted to its numeric (millis) form and
+ * stored back as a string, but it will also accept any other numeric string.
+ * UnsuppressableException always propagates.
+ *
+ * NOTE: the retry exists because converted facet values are stored back as
+ * strings rather than original lexical values, so conversion is not
idempotent
+ * (see the related performance TODO on checkValueSpaceFacetRange). Storing
the
+ * original lexical strings (or the evaluated values) would let the retry be
+ * removed.
+ */
+ protected def calendarStringToBigDecimal(
value: String,
primType: PrimType,
context: ThrowsSDE
): JBigDecimal = {
primType match {
- case PrimType.DateTime =>
- dateToBigDecimal(
- value,
- "uuuu-MM-dd'T'HH:mm:ss.SSSSSSxxx",
- PrimType.DateTime.toString(),
- context
- )
- case PrimType.Date =>
- dateToBigDecimal(value, "uuuu-MM-ddxxx", PrimType.Date.toString(),
context)
- case PrimType.Time =>
- dateToBigDecimal(value, "HH:mm:ss.SSSSSSxxx",
PrimType.Time.toString(), context)
+ case PrimType.DateTime | PrimType.Date | PrimType.Time =>
+ try {
+ primType.fromXMLString(value).getCalendar.toJBigDecimal
+ } catch {
+ case u: UnsuppressableException => throw u
+ case _: Exception =>
+ try {
+ new JBigDecimal(value)
Review Comment:
I'm not sure it's a good idea to to fall back to converting to a
JBigDeicmal. This function is just about calendar strings, so if something
passes in an integer string then it should not succeed, which is what this
does. We did that before, but that probably wasn't the right thing.
I would also suggestw e dont'want to catch unsuppressable exception or
exception. The only exceptino that should be thrown here that we don't want to
bubble up is the InvalidPrimitiveDataException thrown by fromXMLString. I don't
think getCalendar or toJBigDecimal can throw exceptions.
I would also suggest we remove this function and just move calendar
conversion logic into convertFacetToBigDecimal, that way that is the one
function that handles converting a facet to big decimal
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/calendar/DFDLCalendar.scala:
##########
@@ -38,6 +40,21 @@ object DFDLCalendarOrder extends Enumeration {
)
}
+object DateTimeUtil {
+
+ /**
+ * Returns the XSD 1.0 signed year of a calendar. YEAR is the positive
+ * year-of-era; a BCE year is returned negated.
+ *
+ * @param calendar the ICU calendar instance
+ * @return signed year (e.g. 1 BCE -> -1, 1 CE -> 1)
+ */
+ def signedYear(calendar: Calendar): Int = {
Review Comment:
Thoughts on making this a function on DFDLCalendar, similar to hasTimeZone
and toJBigDecimal, so we could do something like
```scala
val xsdYear = dfdlDate.signedYear
```
It would have the same logic, but we don't have to pass in a calendar and it
makes signedYear a bit more visible in case we need it for other things.
It looks like everywhere we call `signedYear` we are calling it on a
DFDLCalendar so it shoudl be a straightforward change.
--
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]
