Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-4104: fix issue with Instant, turn it into datetimestamp value #4145

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

GH-4104: fix issue with Instant, turn it into datetimestamp value #4145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions core/model-api/src/main/java/org/eclipse/rdf4j/model/base/AbstractLiteral.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import static java.lang.Math.abs;
import static java.time.temporal.ChronoField.DAY_OF_MONTH;
import static java.time.temporal.ChronoField.HOUR_OF_DAY;
import static java.time.temporal.ChronoField.INSTANT_SECONDS;
import static java.time.temporal.ChronoField.MINUTE_OF_HOUR;
import static java.time.temporal.ChronoField.MONTH_OF_YEAR;
import static java.time.temporal.ChronoField.NANO_OF_SECOND;
Expand Down Expand Up @@ -522,7 +523,7 @@ static class TemporalAccessorLiteral extends AbstractLiteral {
private static final ChronoField[] FIELDS = {
YEAR, MONTH_OF_YEAR, DAY_OF_MONTH,
HOUR_OF_DAY, MINUTE_OF_HOUR, SECOND_OF_MINUTE, NANO_OF_SECOND,
OFFSET_SECONDS
OFFSET_SECONDS, INSTANT_SECONDS
};

private static final DateTimeFormatter LOCAL_TIME_FORMATTER = new DateTimeFormatterBuilder()
Expand Down Expand Up @@ -584,6 +585,12 @@ static class TemporalAccessorLiteral extends AbstractLiteral {

.toFormatter();

private static final DateTimeFormatter DATETIMESTAMP_FORMATTER = new DateTimeFormatterBuilder()

.appendInstant()

.toFormatter();
abrokenjester marked this conversation as resolved.
Show resolved Hide resolved

private static final DateTimeFormatter DASH_FORMATTER = new DateTimeFormatterBuilder()

.appendLiteral("--")
Expand Down Expand Up @@ -620,13 +627,16 @@ private static Map<Integer, CoreDatatype.XSD> datatypes() {
int time = key(HOUR_OF_DAY, MINUTE_OF_HOUR, SECOND_OF_MINUTE);
int nano = key(NANO_OF_SECOND);
int zone = key(OFFSET_SECONDS);
int instant = key(INSTANT_SECONDS);

Map<Integer, CoreDatatype.XSD> datatypes = new HashMap<>();

datatypes.put(date + time, CoreDatatype.XSD.DATETIME);
datatypes.put(date + time + nano, CoreDatatype.XSD.DATETIME);
datatypes.put((date + time + zone), CoreDatatype.XSD.DATETIME);
datatypes.put((date + time + nano + zone), CoreDatatype.XSD.DATETIME);
datatypes.put((date + time + instant + zone), CoreDatatype.XSD.DATETIME);
datatypes.put((date + time + instant + nano + zone), CoreDatatype.XSD.DATETIME);
datatypes.put((instant + nano), CoreDatatype.XSD.DATETIMESTAMP);

datatypes.put(time, CoreDatatype.XSD.TIME);
datatypes.put(time + nano, CoreDatatype.XSD.TIME);
Expand All @@ -649,6 +659,7 @@ private static Map<CoreDatatype.XSD, DateTimeFormatter> formatters() {

final Map<CoreDatatype.XSD, DateTimeFormatter> formatters = new EnumMap<>(CoreDatatype.XSD.class);

formatters.put(CoreDatatype.XSD.DATETIMESTAMP, DATETIMESTAMP_FORMATTER);
formatters.put(CoreDatatype.XSD.DATETIME, DATETIME_FORMATTER);
formatters.put(CoreDatatype.XSD.TIME, OFFSET_TIME_FORMATTER);
formatters.put(CoreDatatype.XSD.DATE, OFFSET_DATE_FORMATTER);
Expand Down Expand Up @@ -705,18 +716,17 @@ private static int key(Predicate<ChronoField> include, ChronoField... fields) {
private final CoreDatatype.XSD datatype;

TemporalAccessorLiteral(TemporalAccessor value) {

this.value = value;

datatype = DATATYPES.get(key(value));
datatype = DATATYPES.get(key(this.value));

if (datatype == null) {
throw new IllegalArgumentException(String.format(
"value <%s> cannot be represented by an XML Schema date/time datatype", value
"value <%s> cannot be represented by an XML Schema date/time datatype", this.value
));
}

this.label = FORMATTERS.get(datatype).format(value);
this.label = FORMATTERS.get(datatype).format(this.value);
}

@Override
Expand Down
13 changes: 13 additions & 0 deletions core/model/src/test/java/org/eclipse/rdf4j/model/util/ValuesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

import java.math.BigDecimal;
import java.math.BigInteger;
import java.time.Instant;
import java.time.LocalDateTime;
import java.time.Period;
import java.util.Date;
Expand Down Expand Up @@ -508,6 +509,18 @@ public void testTemporalAccessorLiteralNull() {

}

@Test
public void testTemporalAccessorLiteralInstant() {
final Instant value = Instant.ofEpochSecond(1234567890, 12);
Literal literal = literal(value);

assertThat(literal).isNotNull();
assertThat(literal.temporalAccessorValue()).isEqualTo(value);
assertThat(literal.getLabel()).isEqualTo(value.toString());
assertThat(literal.getDatatype()).isEqualTo(XSD.DATETIMESTAMP);
assertThat(literal.getCoreDatatype()).isEqualTo(CoreDatatype.XSD.DATETIMESTAMP);
Comment on lines +520 to +521
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

}

@Test
public void testTriple() {
Triple triple = triple(RDF.ALT, RDF.TYPE, RDFS.CONTAINER);
Expand Down