stevedlawrence commented on code in PR #1685:
URL: https://github.com/apache/daffodil/pull/1685#discussion_r3428121754
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/PrimitivesDateTime.scala:
##########
@@ -288,21 +284,21 @@ case class ConvertBinaryDateTimeSecMilliPrim(e:
ElementBase, lengthInBits: Long)
cal.clear()
cal.setLenient(false)
- val sdfWithTZ = new SimpleDateFormat("uuuu-MM-dd'T'HH:mm:ssZZZZ",
ULocale.ENGLISH)
+ val sdfWithTZ = new SimpleDateFormat("yyyy-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)
+ val sdf = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss", ULocale.ENGLISH)
cal.setTimeZone(TimeZone.UNKNOWN_ZONE)
sdf.parse(e.binaryCalendarEpoch, cal, pos)
Review Comment:
According to the spec `dfdl:binaryCalendarEpoch` is a date time, so I think
it does want to support negative years. I can't imagine anyone wanting a BC
epoch, but who knows. And switching to DFDLCalendarConversions would make
everything consistent it's write that we should use that instead of patterns.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/calendar/DFDLCalendar.scala:
##########
@@ -22,14 +22,15 @@ import java.math.BigDecimal as JBigDecimal
import org.apache.daffodil.lib.exceptions.Assert
import com.ibm.icu.util.Calendar
+import com.ibm.icu.util.GregorianCalendar
import com.ibm.icu.util.TimeZone
object DFDLCalendarOrder extends Enumeration {
type DFDLCalendarOrder = Value
val P_LESS_THAN_Q, P_GREATER_THAN_Q, P_EQUAL_Q, P_NOT_EQUAL_Q = Value
val fieldsForComparison = Array(
- Calendar.EXTENDED_YEAR,
+ Calendar.YEAR,
Review Comment:
I think we need to add Calendar.ERA before this so that when we compare
calendars BC dates are considered less than AD dates.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Facets.scala:
##########
@@ -487,12 +487,12 @@ trait Facets { self: Restriction =>
case PrimType.DateTime =>
dateToBigDecimal(
facet,
- "uuuu-MM-dd'T'HH:mm:ss.SSSSSSxxx",
+ "yyyy-MM-dd'T'HH:mm:ss.SSSSSSxxx",
Review Comment:
I believe the input to this `convertFacetToBigDecimal` is the date/dateTime
XSD 1.0 format. That means the year component could be negative, which `yyyy`
does not support.
I wonder if this should be using the DFDLCalendarConversion functions (which
are intended to efficiently parse XSD 1.0 date/dateTimes and should support
negative years) to convert `facet` to a calendar, and then we can convert that
calendar to a BigDecimal?
Extending that reasoning, it makes me think that if we are using a date/time
pattern string anywhere that might indicate we are doing the wrong thing? The
parsers/unparser will use whatever a schema provides via dfdl:calendarPattern,
and everything else (e.g. facet parsing, creating infoset values) probably
wants to use the DFDLCalendarConversions to covert to/from XSD 1.0 strings.
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml:
##########
@@ -2584,7 +2668,7 @@
<tdml:parserTestCase name="xfromdatetime_04" root="e_xfromdatetime3"
model="Functions.dfdl.xsd" description="Section 23 - Functions -
fn:year-from-dateTime - DFDL-23-109R, fn:month-from-dateTime - DFDL-23-110R,
fn:day-from-dateTime - DFDL-23-111R, fn:hours-from-dateTime - DFDL-23-112R,
fn:minutes-from-dateTime - DFDL-23-113R, fn:seconds-from-dateTime -
DFDL-23-114R">
- <tdml:document>-1234-01-30T10:01:02</tdml:document>
+ <tdml:document>1234-01-30T10:01:02-08:00 BC</tdml:document>
Review Comment:
I'm surprise you needed the timezone component. I would expect it to get
that from dfdl:calendarTimeZone if it was missing, which is what I assumed it
did before?
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/calendar/DFDLCalendarConversion.scala:
##########
@@ -298,12 +310,12 @@ object DFDLDateTimeConversion extends
DFDLCalendarConversion {
/**
* Supported patterns:
- * uuuu-MM-dd'T'HH:mm:ss.SSSSSSxxxxx
- * uuuu-MM-dd'T'HH:mm:ss.SSSSSS
- * uuuu-MM-dd'T'HH:mm:ssxxxxx
- * uuuu-MM-dd'T'HH:mm:ss
- * uuuu-MM-ddxxxxx
- * uuuu-MM-dd
+ * yyyy-MM-dd'T'HH:mm:ss.SSSSSSxxxxx
+ * yyyy-MM-dd'T'HH:mm:ss.SSSSSS
+ * yyyy-MM-dd'T'HH:mm:ssxxxxx
+ * yyyy-MM-dd'T'HH:mm:ss
+ * yyyy-MM-ddxxxxx
+ * yyyy-MM-dd
*/
Review Comment:
Might be worth adding a comment that negative years are supported, since
technically yyyy does not allow them.
##########
daffodil-test/src/test/resources/org/apache/daffodil/section05/simple_types/SimpleTypes.tdml:
##########
@@ -2652,7 +2652,7 @@
<tdml:documentPart type="bits">00000000 00000000 00000000
00111101</tdml:documentPart>
</tdml:document>
<tdml:errors>
- <tdml:error>Schema Definition Error: Failed to parse binaryCalendarEpoch
- Format must match the pattern 'uuuu-MM-dd'T'HH:mm:ss' or
'uuuu-MM-dd'T'HH:mm:ssZZZZ'</tdml:error>
+ <tdml:error>Schema Definition Error: Failed to parse binaryCalendarEpoch
- Format must match the pattern 'yyyy-MM-dd'T'HH:mm:ss' or
'yyyy-MM-dd'T'HH:mm:ssZZZZ'</tdml:error>
Review Comment:
I think technically this is allowed
##########
daffodil-test/src/test/resources/org/apache/daffodil/section05/simple_types/SimpleTypes.tdml:
##########
@@ -5673,7 +5673,7 @@
<tdml:document><![CDATA[02-13-2013 BC]]></tdml:document>
<tdml:infoset>
<tdml:dfdlInfoset>
- <date13>-2012-02-13</date13>
+ <date13>-2013-02-13</date13>
Review Comment:
Yep, this change is definitely correct. Our BC years were all off by one.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/PrimitivesDateTime.scala:
##########
@@ -268,8 +265,7 @@ case class ConvertTextDateTimePrim(e: ElementBase)
extends ConvertTextCalendarPrimBase(e, true) {
protected override val xsdType = "dateTime"
protected override val prettyType = "DateTime"
- protected override val infosetPattern = "uuuu-MM-dd'T'HH:mm:ss.SSSSSSxxx"
- protected override val implicitPattern = "uuuu-MM-dd'T'HH:mm:ss"
+ protected override val implicitPattern = "yyyy-MM-dd'T'HH:mm:ss"
Review Comment:
Note that these implicitPatterns do want to stay. I think they probably want
to be the only place where we have patterns in code.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/calendar/DFDLCalendarConversion.scala:
##########
@@ -95,7 +98,16 @@ object DFDLCalendarConversion {
val d = string.substring(endYear + 4, endYear + 6)
try {
- calendar.set(Calendar.EXTENDED_YEAR, Integer.parseInt(y))
+ // Use YEAR + ERA rather than EXTENDED_YEAR to match XSD 1.0 numbering:
+ // a negative lexical year is BCE, stored as ERA=BC with positive YEAR.
+ val yInt = Integer.parseInt(y)
+ if (yInt < 0) {
Review Comment:
Do we throw an error if yInt is 0? These strings are XSD 1.0 date/times
which do not support year 0.
--
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]
