stevedlawrence commented on code in PR #1677:
URL: https://github.com/apache/daffodil/pull/1677#discussion_r3362277620
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -1326,20 +1350,9 @@ Differences were (path, expected, actual):
maybeType match {
case Some("xs:hexBinary") => dataA.equalsIgnoreCase(dataB)
- case Some("xs:date") => {
- val a = DFDLDateConversion.fromXMLString(dataA)
- val b = DFDLDateConversion.fromXMLString(dataB)
- a == b
- }
- case Some("xs:time") => {
- val a = DFDLTimeConversion.fromXMLString(dataA)
- val b = DFDLTimeConversion.fromXMLString(dataB)
- a == b
- }
- case Some("xs:dateTime") => {
- val a = DFDLDateTimeConversion.fromXMLString(dataA)
- val b = DFDLDateTimeConversion.fromXMLString(dataB)
- a == b
+ case Some("xs:date") | Some("xs:time") | Some("xs:dateTime") => {
+ // braces are redundant in case clauses, but kept here for consistency.
Review Comment:
We don't need this comment about braces
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -54,6 +53,9 @@ import org.xml.sax.XMLReader
object XMLUtils {
+ // DatatypeFactory creation is relatively expensive, so create it once and
reuse.
+ private val datatypeFactory = DatatypeFactory.newInstance()
Review Comment:
Let's make this lazy. This factory is unlikely to be used in most Daffodil
usages, it's only tdml tests where it's needed, so we should avoid creating if
it's unlikely to be be used.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -1300,6 +1302,28 @@ Differences were (path, expected, actual):
}
}
+ /**
+ * Compares two XSD date/time lexical strings (`xs:date`, `xs:time`, or
+ * `xs:dateTime`) for value equality by parsing both into
`XMLGregorianCalendar`
+ * and comparing via the XSD `·order·` relation. This keeps ICU off the
+ * comparison path entirely, which is what allows the IBM DFDL cross tester
+ * (pinned to an older ICU version) to share this code without hitting
+ * newer-ICU-only methods (DAFFODIL-3077).
Review Comment:
I think it would be worth explicitly calling out us not using our custom
`DFDL*Conversion` classes. Maybe tweak it to say
> Note that we intentionally do not use Daffodil's
`DFDL*Conversion.fromXMLString` classes, which keeps ICU off the comparison
path entirely and allows the IBM DFDL cross tester (pinned to an older ICU
version) to share this code without hitting newer-ICU-only methods
(DAFFODIL-3077).
##########
daffodil-test/src/test/scala/org/apache/daffodil/section23/dfdl_expressions/TestDFDLExpressions.scala:
##########
@@ -766,7 +766,7 @@ class TestDFDLFunctions extends TdmlTests {
@Test def yearfromdatetime_01 = test
@Test def yearfromdatetime_02 = test
- @Test def yearfromdatetime_03 = test
+ @Ignore("year zero is invalid per XSD 1.0") @Test def yearfromdatetime_03 =
test
Review Comment:
Instead of adding an ignore message, our convention has been to create an
issue and add a comment, e.g.
```scala
// DAFFODIL-XYZ
@Ignore @Test def yearfromdatetime_03 = test
```
We may want to consider a convention where the message is the bug number,
i.e. `@Ignore("DAFFODIL-XYZ")`, but I would suggest we make that change
wholesale at once if we do want to do it. Probably not worth the effort for now
though.
--
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]
