GH-4104: fix issue with Instant, turn it into datetimestamp value #4145
Conversation
…amp value Signed-off-by:Bart Hanssens <[email protected]>
barthanssens
force-pushed
the
GH-4104-values-instant-xsdtimestamp
branch
from
9e49002 to
bf6452a
Compare
core/model-api/src/main/java/org/eclipse/rdf4j/model/base/AbstractLiteral.java
Show resolved
Hide resolved
| assertThat(literal.getDatatype()).isEqualTo(XSD.DATETIMESTAMP); | ||
| assertThat(literal.getCoreDatatype()).isEqualTo(CoreDatatype.XSD.DATETIMESTAMP); |
There was a problem hiding this comment.
What's the reasoning for using dateTimeStamp instead of just dateTime? As far as I'm aware the only difference is that the time zone is not optional in dateTimeStamp, but dateTime seems to be more far more commonly used.
There was a problem hiding this comment.
Unfortunately a bit of a hack... currently the DateTimeFormatter is selected based on the XSD type.
So for xsd:dateTime a DateTimeFormatter is selected that uses ChronoField.YEAR field etc, which is not supported by Instant...
Giving it a second thought.... Perhaps it could be implemented the other way round (always use a formatter using Chronofield.INSTANT_SECONDS for xsd:dateTime), though that would break if a Temporal would not support that field
There was a problem hiding this comment.
@barthanssens but regardless of which formatter you use, you could use the xsd:dateTime datatype URI for the resulting literal I think? Every dateTimeStamp is also a valid dateTime...
There was a problem hiding this comment.
I have to re-check the code, but I was under the impression that the current code first selects the xsd type on the chronofields of the java class, (where Instant does not support e.g. Year)
and then that xsd type is used to select the dateformatter
Instead of first do the formatting and then set the xsd datatype.
I'll look into it, I do agree that using dateTime for all cases is a better solution...
Signed-off-by:Bart Hanssens [email protected]
GitHub issue resolved: #4104
Briefly describe the changes proposed in this PR:
PR Author Checklist (see the contributor guidelines for more details):
mvn process-resourcesto format from the command line)