Backport OffsetDateTime changes from 4.0 by xiazcy · Pull Request #3090 · apache/tinkerpop

xiazcy · 2025-04-04T19:09:37Z

OffsetDateTime is now the default Date type in Gremlin.
Changes were back-ported from Java/Python. For Go/JS/.NET, the OffsetDateTime serializers were added.
Made OffsetDateTime the default serializer for dates in GLVs, the Date type will only deserialize responses.

VOTE +1.

…ateTime is now the default Date type in Gremlin; added OffsetDateTime serializers to Go/JS/.NET. Made OffsetDateTime the default serializer for date types in GLVs (Date type will only deserialize).

...e/src/main/java/org/apache/tinkerpop/gremlin/language/translator/PythonTranslateVisitor.java

Cole-Greer · 2025-04-04T19:55:24Z

...-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/DateAddStep.java

+        OffsetDateTime date = (OffsetDateTime) object;
+        OffsetDateTime new_date;
+
+        switch (dateToken) {


nit: dateToken and value should arguably be converted to a Duration in the constructor for this step so this conversion does not need to happen each time the step is iterated.

...-core/src/main/java/org/apache/tinkerpop/gremlin/language/translator/GoTranslateVisitor.java

...rc/main/java/org/apache/tinkerpop/gremlin/process/traversal/translator/GolangTranslator.java

...ain/java/org/apache/tinkerpop/gremlin/process/traversal/translator/JavascriptTranslator.java

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/DatetimeHelper.java

...e/src/test/java/org/apache/tinkerpop/gremlin/language/grammar/GeneralLiteralVisitorTest.java

Cole-Greer · 2025-04-04T21:33:21Z

gremlin-go/driver/graphBinary.go

+	offset := readIntSafe(data, i)
+	// only way to pass offset info, timezone display is fixed to UTC as consequence (offset is calculated properly)
+	loc := time.FixedZone("UTC", int(offset))
+	datetime := time.Date(int(year), time.Month(month), int(day), 0, 0, 0, int(ns), loc)


int(ns) may be an issue for times which are late in the day. According to the io docs, the "time" value can be from 0 to 86,399,999,999,999. The max size of int is only guaranteed to be 2,147,483,647, although on most 64-bit machines it will be a 64 bit integer in practice.

Unfortunately the Date constructor requires int, it's more like a caveat of go, not sure if it's avoidable.

I think we need to break the 64bit ns into hours, minutes, seconds, and nanos to ensure none of them will overflow on systems with 32bit ints. We can't allow those datetimes to overload on deserialization.

kmcginnes · 2025-04-04T22:21:07Z

...c/main/java/org/apache/tinkerpop/gremlin/language/translator/JavascriptTranslateVisitor.java

+        final OffsetDateTime dt = DatetimeHelper.parse(removeFirstAndLastCharacters(dtString));
        sb.append("new Date(");
-        sb.append(dt.getTime());
+        sb.append(dt.toInstant().toEpochMilli());


This should work, and it seems equivalent to what was there before. As long as you've tested this with various time zones it should be fine.

However, I'd wager that if someone is using the translator to translate a query, I doubt they started with a date value as the millisecond since unix epoch. More likely they started with a string version of the date time.

Perhaps a better approach would be to use the ISO string value as the input to new Date(). The precision should be the same (milliseconds), and it supports time zone offsets or UTC. It's just a more human friendly translation.

kmcginnes · 2025-04-04T22:35:05Z

...-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/DateAddStep.java

-        cal.add(DTtoCalendar.get(dateToken), value);
-
-        return cal.getTime();
+        if (!(object instanceof OffsetDateTime)) throw new IllegalArgumentException("dateAdd accept only DateTime.");


Should the error be:

dateAdd accept only OffsetDateTime.

We changed it a general DateTime type in 4.0, but you are right it prob make more sense to be OffsetDateTime here. I will also make it accept Date as well for compatibility reasons.

kmcginnes · 2025-04-04T22:51:00Z

...-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/DateAddStep.java

Is it intentional for this step to only have second precision?

The original design of the step uses the dateToken enums, which only goes to second precision. I suppose we could add support for additional precisions, but that would be outside the scope of this PR.

I too find it strange that date add and date diff do not work with millisecond precision.

I agree that the available precision here is odd, although this is really out of scope for this PR and should be taken as a separate discussion. I think there should be come consideration of taking a Duration as input instead of an int and DT enum, although some consideration is needed around how to make Duration's constructable in gremlin-lang.

kmcginnes · 2025-04-04T22:53:47Z

...core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/DateDiffStep.java

+        final long otherDateMs = otherDate == null ? 0 : otherDate.toEpochSecond();

-        return (((Date) object).getTime() - otherDateMs) / 1000;
+        return (((OffsetDateTime) object).toEpochSecond() - otherDateMs);


Did you mean to use .toEpochMilli()?

If you did intend to use seconds instead of milliseconds, then you should change the variable name otherDateMs to be otherDateSeconds or something.

No this step meant to return epoch time in seconds, so I'm just directly using it.

kmcginnes · 2025-04-04T23:10:16Z

...e/src/test/java/org/apache/tinkerpop/gremlin/language/grammar/GeneralLiteralVisitorTest.java

-            final Date dt = (Date) new GenericLiteralVisitor(new GremlinAntlrToJava()).visitDateLiteral(ctx);
-            assertTrue(new Date().getTime() - dt.getTime() < 1000);
+            final OffsetDateTime dt = (OffsetDateTime) new GenericLiteralVisitor(new GremlinAntlrToJava()).visitDateLiteral(ctx);
+            assertTrue(OffsetDateTime.now(UTC).toEpochSecond() - dt.toEpochSecond() < 1000);


It looks like we are dropping from millisecond to second precision here.

kmcginnes · 2025-04-04T23:16:29Z

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Translator.java

                    return script.getBoundKeyOrAssign(withParameters, objectOrWrapper);
+                } else if (object instanceof OffsetDateTime) {
+                    final Object objectOrWrapper = withParameters ? object : getSyntax((OffsetDateTime) object);
+                    return script.getBoundKeyOrAssign(withParameters, objectOrWrapper);


Now that this exists, should the else if (object instanceof Date) above be removed?

I believe it should stay because both Date and OffsetDateTime types are still supported, just now the default is OffsetDateTime.

andreachild · 2025-04-07T16:42:32Z

.../main/java/org/apache/tinkerpop/gremlin/language/translator/AnonymizedTranslatorVisitor.java

 import java.math.BigDecimal;
 import java.math.BigInteger;
+import java.time.OffsetDateTime;
 import java.util.Date;


This import is now unused

andreachild · 2025-04-07T16:43:59Z

...e/src/main/java/org/apache/tinkerpop/gremlin/language/translator/DotNetTranslateVisitor.java

 import org.apache.tinkerpop.gremlin.util.DatetimeHelper;

+import java.time.OffsetDateTime;
 import java.util.Date;


This import is now unused

andreachild · 2025-04-07T18:16:01Z

...-core/src/main/java/org/apache/tinkerpop/gremlin/language/translator/GoTranslateVisitor.java

+import java.time.OffsetDateTime;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Date;


This import is now unused

andreachild · 2025-04-07T18:16:53Z

...ore/src/main/java/org/apache/tinkerpop/gremlin/language/translator/JavaTranslateVisitor.java

 import org.apache.tinkerpop.gremlin.util.DatetimeHelper;

+import java.time.OffsetDateTime;
 import java.util.Date;


This import is now unused

andreachild · 2025-04-07T18:30:47Z

...e/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java

 import org.apache.tinkerpop.gremlin.structure.VertexProperty;
 import org.apache.tinkerpop.gremlin.util.function.ConstantSupplier;

+import java.time.OffsetDateTime;


Date import is now unused

andreachild · 2025-04-07T18:32:27Z

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/__.java


+import java.time.OffsetDateTime;
 import java.util.Collection;
 import java.util.Date;


andreachild · 2025-04-07T18:50:07Z

...n-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AsDateStep.java

+        if (object instanceof OffsetDateTime)
+            return (OffsetDateTime) object;
        if (object instanceof Byte || object instanceof Short || object instanceof Integer || object instanceof Long)
            // numbers handled as milliseconds since January 1, 1970, 00:00:00 GMT.


Comment should be updated to UTC instead of GMT which are slightly different.

andreachild · 2025-04-07T18:53:16Z

...n-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AsDateStep.java

        final Object object = traverser.get();
        if (object == null)
-            throw new IllegalArgumentException("Can't parse null as Date.");
-        if (object instanceof Date)


Technically since Date types are supported isn't it possible to use the asDate step on a traversal that produces Date? In that case should the Date be converted to OffsetDateTime?

Yes, this is part of the change I'll revert along with other changes to make it compatible with Date as type.

andreachild · 2025-04-07T19:45:27Z

...core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/DateDiffStep.java


+import java.time.OffsetDateTime;
 import java.util.Collections;
 import java.util.Date;


andreachild · 2025-04-07T21:50:04Z

...core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/DateDiffStep.java

+        final long otherDateMs = otherDate == null ? 0 : otherDate.toEpochSecond();

-        return (((Date) object).getTime() - otherDateMs) / 1000;
+        return (((OffsetDateTime) object).toEpochSecond() - otherDateMs);


Suggestion:

return otherDate == null ? 0L : Duration.between(otherDate, (OffsetDateTime) object).getSeconds();

I like the simplification, not quite the right logic but I can use it, i.e. if otherDate is null we consider it as 0L and return the epoch seconds of date, instead of returning 0L.

andreachild · 2025-04-07T22:38:57Z

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/DatetimeHelper.java

+import java.time.Month;
+import java.time.OffsetDateTime;
+import java.time.Year;
 import java.time.YearMonth;


Unused imports

andreachild · 2025-04-07T23:27:11Z

gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/DataType.cs

        // TODO: Support metrics and traversal metrics
        public static readonly DataType Char = new DataType(0x80);
        public static readonly DataType Duration = new DataType(0x81);
+        public static readonly DataType OffsetDateTime = new DataType(0x88);


Just wondering why OffsetDateTime was not previously supported as a type for .NET? This PR does not add the type to TInkerPop, only changes the default date type.

Not just .NET, it actually wasn't supported in any of the GLVs because it's an extended serialization type.

andreachild · 2025-04-08T00:28:28Z

gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/Types/OffsetDateTimeSerializer.cs

+            var s = value.Second;
+            var ms = value.Millisecond; 
+            // Note there will be precision loss as microsecond and nanosecond access was added after .net 7
+            var ns = h * 60 * 60 * 1e9 + m * 60 * 1e9 + s * 1e9 + ms * 1e6;


Q says obtaining nanoseconds is possible using Ticks:

// Convert the time portion of DateTimeOffset to nanoseconds since midnight static long TimeToNanosecondsSinceMidnight(DateTimeOffset dto) { // Get the time of day as TimeSpan TimeSpan timeOfDay = dto.TimeOfDay; // Convert ticks to nanoseconds (1 tick = 100 nanoseconds in .NET) return timeOfDay.Ticks * 100; }

…e Date as parameter to dateDiff, update date precision in GLV translators.

kenhuuu · 2025-04-09T22:08:50Z

...re/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AsDateStepTest.java

-        assertEquals(new Date(1), __.__(1).asDate().next());
-        assertEquals(new Date(3), __.__(3L).asDate().next());
-        assertEquals(new Date(6), __.__(new BigInteger("6")).asDate().next());
-        assertEquals(testDate, __.__(testDate.getTime()).asDate().next());


You should add tests where Date is the input traverser. Same goes for the DateAddStepTest and DateDiffStepTest

Yup I can do that for asDate and dateAdd (tests in dateDiff were added already)

andreachild · 2025-04-09T22:57:57Z

...e/src/main/java/org/apache/tinkerpop/gremlin/language/translator/PythonTranslateVisitor.java

 import org.apache.tinkerpop.gremlin.util.DatetimeHelper;

 import java.math.BigInteger;
+import java.time.OffsetDateTime;


Unused imports

andreachild · 2025-04-09T23:21:39Z

...-core/src/main/java/org/apache/tinkerpop/gremlin/language/translator/GoTranslateVisitor.java

+                append(", ").append(dt.getMinute()).
+                append(", ").append(dt.getSecond()).
+                append(", ").append(dt.getNano()).
+                append(", time.FixedZone(\"UTC\", ").append(dt.getOffset().getTotalSeconds()).append(")").


Looking at documentation https://pkg.go.dev/time#FixedZone it looks like the zone name should include the offset, ie. UTC-8.

Also can a test be added to GremlinTranslatorTest.java for non zero offset scenario?

I can add the tests.

Technically the fixed zone name in go for this is a string only for display purposes...I.e. you can pass in any random string and it just appears at the end of your time display (doesn't affect the date calculation at all). For simplicity I just left the "UTC" string in, but can get the zoneId from OffsetDateTime instead.

…erializer to handle offset more properly.

andreachild · 2025-04-10T15:54:06Z

...e/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java

+    }
+
+    /**
+     * Returns the difference between two {@link OffsetDateTime} in epoch time.


Nit: could use a comment about the supported traversal types which has been changed from Date to ?. ie. explain why wasn't it just changed to OffsetDateTime?

andreachild · 2025-04-10T16:06:48Z

...-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/DateAddStep.java

+                newDate = date.plus(Duration.ofDays(value));
+                break;
+            default:
+                throw new IllegalArgumentException("DT tokens should only be second, minute, hour, or day.");


Should this validation be moved to the constructor instead?

andreachild · 2025-04-10T17:03:40Z

...core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/DateDiffStep.java

-    public DateDiffStep(final Traversal.Admin traversal, final Traversal<?, Date> dateTraversal) {
+    public DateDiffStep(final Traversal.Admin traversal, final Traversal<?, ?> dateTraversal) {
        super(traversal);
        this.dateTraversal = this.integrateChild(dateTraversal.asAdmin());


If the incoming Traversal is of type ? then shouldn't this.dateTraversal be of type ? as well since there's no guarantee the traversal resolves to an OffsetDateTime?

The current typing confuses IntelliJ:

Oh yes, that was a miss

andreachild · 2025-04-10T17:14:25Z

...core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/DateDiffStep.java

+                date = ((Date) object).toInstant().atOffset(ZoneOffset.UTC);
+            } else {
+                throw new IllegalArgumentException(
+                        String.format("DateDiff can only take OffsetDateTime or Date (deprecated) as argument, encountered %s", object.getClass()));


This can throw NullPointerException if object is null. Should the null logic handling be made consistent with the null logic handling for dateTraversal that assumes null means zero?

Yea this might need some handling, but I'd say out of scope for the PR. Logged in Jira: https://issues.apache.org/jira/browse/TINKERPOP-3152

andreachild · 2025-04-10T19:49:13Z

gremlin-go/driver/graphBinary.go

+		return nil, err
+	}
+	// construct time of day in nanoseconds
+	h := t.Hour()


Q recommends type conversion to int64 for these values to prevent issues with overflow on 32 bit systems.

Hmmm technically all date methods return/accept int, so that will remain an issue for 32 bit system regardless, but I can make it int64 to make the problem a bit less.

Cole-Greer · 2025-04-10T22:06:15Z

VOTE +1 pending fix to Go GraphBinary overflow issue and upgrade docs.

kenhuuu · 2025-04-10T23:54:34Z

VOTE +1

…ed upgrade docs.

Backport OffsetDateTime changes from Java/Python on 4.0 where OffsetD…

c515c6d

…ateTime is now the default Date type in Gremlin; added OffsetDateTime serializers to Go/JS/.NET. Made OffsetDateTime the default serializer for date types in GLVs (Date type will only deserialize).