Support date/time compute fields #25

aymkhalil · 2022-10-04T00:56:59Z

Fixes: #20

This patch adds support for the following date/time types:

Date: Contains date info only, represented as ISO 8601 date like '2022-10-02'
Time: Contains time info only, represented as ISO 8601 time like '23:00:00'
OffsetDateTime: Full date time info, represented as UTC ISO 3339 like '2011-12-03T10:15:30Z' or '2011-12-03T10:15:30+00:00' or '2011-12-03T10:15:30+02:00'
Added the following utility date functions:
- now() return the epoch millis as of now
- dateadd(input , delta, unit) where
  - input: Is either a Long representing epoch millis or a string representing ISO date like '2011-12-03T10:15:30'
  - delta: As int representing how many units to add or subtract form the input date.
  - unit: could be one of [years, months, days, hours, minutes, seconds, millis]

Implementation note:

The JSTL evaluator maps the types Date, Time, OffsetDateTime to java.time.LocalDate, java.time.LocalTime, java.time.Instant respectively.
In Avro, the types Date, Time, OffsetDateTime are converted to logical times date, time-millis, timestamp-millis respectively

eolivelli

The approach LGTM

how does this patch play with the Cassandra Sink, the ElasticSearch Sink (or the other Sinks) ?

aymkhalil · 2022-10-04T15:53:21Z

The approach LGTM

how does this patch play with the Cassandra Sink, the ElasticSearch Sink (or the other Sinks) ?

It works with with ES sink but not with Cassandra Sink (because the logical types that are supported are the CDC custom ones, not standard Avro). I created an issue to support that.

As far as this patch, it makes sense to stick only to the standard avro types.

eolivelli

LGTM
great stuff

...ons/src/main/java/com/datastax/oss/pulsar/functions/transforms/jstl/CustomTypeConverter.java

cbornet · 2022-10-05T15:53:16Z

...sformations/src/main/java/com/datastax/oss/pulsar/functions/transforms/ComputeFieldStep.java

+        LocalTime localTime = (LocalTime) value;
+        return (int) (localTime.toNanoOfDay() / 1000000);
+      case "timestamp-millis":
+        validateLogicalType(value, schema.getLogicalType(), LocalDateTime.class);


Shouldn't it be mapped to a j.u.Instant ?

This is driven by the type generated from the JSTL evaluator. Now the same question applies (why not use Instant with ejstl?)

We could, but I found it:

More consistent with the TIME and DATE types (because for those the better option is LocalTime & LocalDate as those parse the provided input in ISO format parts out of the box - things like 2017-02-10 or 22:30:00

Instant iso format require the user to pass empty Z in the format (like 2017-02-10T22:30:00Z, but local date time accepts 2017-02-10T22:30:00. As far as user is concerned, all dates in their avro output will be in reference to epoch

LocalDateTime also provides plus operation with Units of years and months which are utilized with the dateadd method. Instant only accepts those values: https://docs.oracle.com/javase/8/docs/api/java/time/Instant.html#plus-long-java.time.temporal.TemporalUnit-

It seems to me that if the datetime is referenced to epoch, Instant is the type to use whereas LocalDatetime is not bound to a timezone (it's the time you see on the clock on the wall without knowing where you are). But probably you transparently assume UTC timezone and the user doesn't see the LocalDatetime type anyway.

that is fair, let me switch to time instant. And then we can decide separately if we want the user format to be:
'2007-12-03T10:15:30 vs 2007-12-03T10:15:30.00Z (the latter is the Instant requirement, empty Z must be included indicating UTC.

Either way, the user is not aware of the underlying date object - it is just easier for us to use the underlying parse method as is without pre-processing

OK I see. What I would suggest is that we support RFC3339 dates. This maps well to j.u.t.OffsetDateTime.
With maybe some tolerance on the input string where if the time offset is not there, we assume UTC. I think Jackson does it (not sure).

Also OffsetDateTime has plusMonths and plusYears

I think Jackson does it (not sure).

It does not.

+1 to ISO 3330. to make documentation easier, we can stick to its format and refer user to the RFC. (for UTC, they could write either 2022-07-02T01:02:00Z OR 2022-07-02T01:02:00+00). If the provide millis (which I think will be common case) we assume epoch millis. Another common use case that won't include the string is fn:now()).

we can always add more permissive format (like 2022-07-02T01:02:00) without breaking existing users...

cbornet · 2022-10-05T16:06:55Z

...ations/src/test/java/com/datastax/oss/pulsar/functions/transforms/jstl/JstEvaluatorTest.java

+    long actualNow =
+        new JstlEvaluator<Long>("${fn:now()}", long.class).evaluate(primitiveStringContext);
+
+    assertTrue(actualNow >= expectedNow);


Maybe we could have the possibility to pass a custom Clock to the class under test.
This way it's easier to control time.

+1. will do.

JUEL only supports binding of static method, so I have to do it the suboptimal way by introducing a clock on the static class and modify it by tests (as opposed to dependency injection)

cbornet · 2022-10-05T16:16:51Z

...ations/src/test/java/com/datastax/oss/pulsar/functions/transforms/jstl/JstEvaluatorTest.java

+
+    long expected = Instant.now().plusSeconds(-3333).toEpochMilli();
+    long actual =
+        new JstlEvaluator<Long>("${fn:dateadd(fn:now(), -3333, 'seconds')}", long.class)


I think it would be nice to be able to do pass an ISO8601 duration: https://en.m.wikipedia.org/wiki/ISO_8601#Durations.
Jackson has good support for deserializing Durations.

Jackson may not even be needed. It's probably as simple as Duration.parse() : https://docs.oracle.com/javase/8/docs/api/java/time/Duration.html#parse-java.lang.CharSequence-

+1, will add in a separate PR: #28

btw the Duration in java.time supports only this format (still ISO compliant): PnDTnHnMn.nS so user cannot include years or months. Joda period maybe a solution. Check #28 for more info.

cbornet · 2022-10-11T10:08:22Z

...ormations/src/main/java/com/datastax/oss/pulsar/functions/transforms/jstl/JstlFunctions.java

+  }
+
+  private static long dateadd(LocalDateTime localDateTime, long delta, String unit) {
+    ChronoUnit chronoUnit = dateAddUnits.get(unit);


We could use ChronoUnit.valueOf() and remove the dateAddUnits map ?

public static long dateadd(Object input, long delta, String unit) { if (input instanceof String) { return dateadd((String) input, delta, unit); } else if (input instanceof Long) { return dateadd((long) input, delta, unit); } throw new IllegalArgumentException( "Invalid input type: " + input.getClass().getSimpleName() + ". Should either be an RFC3339 datetime string like '2007-12-01T12:30:00Z' or epoch millis"); } private static long dateadd(String isoDateTime, long delta, String unit) { OffsetDateTime offsetDateTime = OffsetDateTime.parse(isoDateTime); return dateadd(offsetDateTime, delta, unit); } private static long dateadd(long epochMillis, long delta, String unit) { Instant instant = Instant.ofEpochMilli(epochMillis); OffsetDateTime offsetDateTime = OffsetDateTime.ofInstant(instant, ZoneOffset.UTC); return dateadd(offsetDateTime, delta, unit); } private static long dateadd(OffsetDateTime offsetDateTime, long delta, String unit) { try { ChronoUnit chronoUnit = ChronoUnit.valueOf(unit.toUpperCase()); return offsetDateTime.plus(delta, chronoUnit).toInstant().toEpochMilli(); } catch (IllegalArgumentException e) { throw new IllegalArgumentException( "Invalid unit: " + unit + ". Should be one of " + Arrays.stream(ChronoUnit.values()) .map(s -> s.name().toLowerCase()) .collect(Collectors.toList()), e); } }

The unit does not map one to one with the Chrono units (it is a subset):
[nanos, micros, millis, seconds, minutes, hours, half_days, days, weeks, months, years, decades, centuries, millennia, eras, forever] vs years, months, days,hours,minutes,millis - also just preder to have them decoupled in case we use different names (day vs days)

You're right. It's better to keep it decoupled.
Maybe use a switch instead of a Map ? (no strong opinion here)
If we keep the Map, we should initialize it with Map.of() (min JDK is 11) instead of a static initializer.

+1. Switch fits better, I wanted to cache them for some reason but there is no need to.

cbornet · 2022-10-11T10:20:10Z

...src/test/java/com/datastax/oss/pulsar/functions/transforms/jstl/CustomTypeConverterTest.java

    assertNull(converter.convert(null, String.class));
    assertNull(converter.convert(null, int.class));
    assertNull(converter.convert(null, long.class));
    assertNull(converter.convert(null, float.class));
    assertNull(converter.convert(null, double.class));
    assertNull(converter.convert(null, boolean.class));
+    assertNull(converter.convert(null, LocalDate.class));
+    assertNull(converter.convert(null, LocalTime.class));
+    assertNull(converter.convert(null, LocalDateTime.class));


Still LocalDateTime ?

cbornet · 2022-10-11T10:54:41Z

...ormations/src/main/java/com/datastax/oss/pulsar/functions/transforms/jstl/JstlFunctions.java

+    return clock.millis();
+  }
+
+  private static final ZoneId UTC = ZoneId.of("UTC");


Already exists as ZoneOffset.UTC

cbornet · 2022-10-12T22:23:17Z

...sformations/src/main/java/com/datastax/oss/pulsar/functions/transforms/ComputeFieldStep.java

+        .getTypes()
+        .stream()
+        .filter(subSchema -> subSchema.getLogicalType() != null)
+        .map(Schema::getLogicalType)


The filter and map can be switched. And filter is then just Objects::nonnull

aymkhalil requested review from eolivelli and cbornet October 4, 2022 00:57

aymkhalil marked this pull request as draft October 4, 2022 00:59

eolivelli approved these changes Oct 4, 2022

View reviewed changes

eolivelli self-requested a review October 4, 2022 06:28

aymkhalil force-pushed the fn-date-time branch from 930a20d to d3d14ce Compare October 4, 2022 18:43

aymkhalil marked this pull request as ready for review October 4, 2022 18:50

eolivelli approved these changes Oct 5, 2022

View reviewed changes

...ons/src/main/java/com/datastax/oss/pulsar/functions/transforms/jstl/CustomTypeConverter.java Outdated Show resolved Hide resolved

cbornet reviewed Oct 5, 2022

View reviewed changes

aymkhalil requested a review from cbornet October 5, 2022 19:35

aymkhalil added 3 commits October 6, 2022 09:44

Support date/time compute fields

ddd2112

Use custom clock for now() function

fe181a2

Back ISO datetime strings by java.time.Instant

b9a694d

aymkhalil force-pushed the fn-date-time branch from a1705cf to b9a694d Compare October 6, 2022 16:44

cbornet reviewed Oct 11, 2022

View reviewed changes

Use rfc 3339 instead of 8601 for date time type

6d8a229

aymkhalil requested a review from cbornet October 12, 2022 19:12

cbornet reviewed Oct 12, 2022

View reviewed changes

Address minor comments

2b8ba56

aymkhalil requested a review from cbornet October 12, 2022 23:59

cbornet approved these changes Oct 13, 2022

View reviewed changes

cbornet merged commit 5d99244 into master Oct 13, 2022

cbornet deleted the fn-date-time branch October 13, 2022 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support date/time compute fields #25

Support date/time compute fields #25

aymkhalil commented Oct 4, 2022 •

edited

Loading

eolivelli left a comment •

edited

Loading

aymkhalil commented Oct 4, 2022

eolivelli left a comment

cbornet Oct 5, 2022

aymkhalil Oct 5, 2022 •

edited

Loading

cbornet Oct 6, 2022

aymkhalil Oct 6, 2022

cbornet Oct 11, 2022

cbornet Oct 11, 2022

cbornet Oct 11, 2022

aymkhalil Oct 12, 2022

cbornet Oct 5, 2022

aymkhalil Oct 5, 2022

aymkhalil Oct 5, 2022

cbornet Oct 5, 2022

cbornet Oct 5, 2022

aymkhalil Oct 5, 2022

aymkhalil Oct 6, 2022 •

edited

Loading

cbornet Oct 11, 2022 •

edited

Loading

aymkhalil Oct 12, 2022

cbornet Oct 12, 2022

aymkhalil Oct 12, 2022

cbornet Oct 11, 2022

cbornet Oct 11, 2022

cbornet Oct 12, 2022

aymkhalil Oct 12, 2022

Support date/time compute fields #25

Support date/time compute fields #25

Conversation

aymkhalil commented Oct 4, 2022 • edited Loading

eolivelli left a comment • edited Loading

Choose a reason for hiding this comment

aymkhalil commented Oct 4, 2022

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aymkhalil Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aymkhalil Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

cbornet Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aymkhalil commented Oct 4, 2022 •

edited

Loading

eolivelli left a comment •

edited

Loading

aymkhalil Oct 5, 2022 •

edited

Loading

aymkhalil Oct 6, 2022 •

edited

Loading

cbornet Oct 11, 2022 •

edited

Loading