SQL: Replace joda with java time by matriv · Pull Request #38437 · elastic/elasticsearch

matriv · 2019-02-05T15:30:18Z

Replace remaining usages of joda classes with java time.

Replace remaining usages of joda classes with java time. Fixes: elastic#37703

elasticmachine · 2019-02-05T15:30:24Z

Pinging @elastic/es-search

matriv · 2019-02-05T15:57:24Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

            if (values instanceof String) {
                return DateUtils.asDateTime(Long.parseLong(values.toString()));
            }
-            // returned by nested types...


@costin I'm not sure about this, but doesn't seem to break something. Should we check more in depth and add tests?

It can be removed now with Joda out. Likely this will eliminate asDateTime as well.

pgomulka

it looks like we are close to finish this, looks good but left some comments

pgomulka · 2019-02-05T15:38:55Z

x-pack/plugin/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/TypeConverterTests.java


    public void testTimestampAsNative() throws Exception {
-        DateTime now = DateTime.now();
+        ZonedDateTime now = ZonedDateTime.now(Clock.tick(Clock.system(ZoneId.of("Z")), Duration.ofMillis(1)));


why using Clock here?
maybe we could just use
ZonedDateTime.now(ZoneOffset.UTC)

If I recall correctly, it's because the clock approach had unpredictable behavior between jdk 8 and 11.
@astefan do you remember?

Right - you guys made an excellent fix here #38437 (comment)
I had similar problems this morning with watcher.

@costin yes. We had this issue in the past for the NOW() function - #36877. @matriv's approach is spot on.

pgomulka · 2019-02-05T15:49:13Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/util/DateUtils.java

-    private static final long DAY_IN_MILLIS = 60 * 60 * 24 * 1000;
-
-    // TODO: do we have a java.time based parser we can use instead?
-    private static final DateTimeFormatter UTC_DATE_FORMATTER = ISODateTimeFormat.dateOptionalTimeParser().withZoneUTC();


would it be the same if we use DateFormatters.forPattern("date_optional_time") ?

It might work (see the TODO above).

If I use the date_optional_time and try to parse a date only String like 2019-02-06 I get:

java.time.DateTimeException: Unable to obtain Instant from TemporalAccessor: {},ISO resolved to 1970-01-01 of type java.time.format.Parsed

So I need the same formatter but with:

.parseDefaulting(HOUR_OF_DAY, 0) .parseDefaulting(MINUTE_OF_HOUR, 0) .parseDefaulting(SECOND_OF_MINUTE, 0)

pgomulka · 2019-02-05T15:58:06Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/util/DateUtils.java

     */
    public static ZonedDateTime asDateTime(String dateFormat) {
-        return asDateTime(UTC_DATE_FORMATTER.parseDateTime(dateFormat));
+        return asDateTime(Instant.from(UTC_DATE_FORMATTER.parse(dateFormat)).toEpochMilli());


how about something like?

return DateFormatters.from(DATE_TIME_FORMATTER.parse(dateFormat)))

Actually I need return DateFormatters.from(UTC_DATE_FORMATTER.parse(dateFormat)).withZoneSameInstant(UTC);

pgomulka · 2019-02-05T15:59:18Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/util/DateUtils.java

    public static final ZoneId UTC = ZoneId.of("Z");
    public static final String DATE_PARSE_FORMAT = "epoch_millis";

+    private static final DateTimeFormatter UTC_DATE_FORMATTER = new DateTimeFormatterBuilder()


I am not sure we need this (looks like date_optional_time to me) but if so, maybe we could have it in DateFormatters?

Indeed, it looks to be date_optional_time

See my response here: #38437 (comment)

Is there an elegant way to overcome this when using the date_optional_time ?

Scratch that, I didn't check again after using DateFormatters.from.
This method has the logic to handle the date only part.

pgomulka · 2019-02-05T16:01:18Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java

 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static org.elasticsearch.xpack.sql.type.DataTypeConversion.conversionFor;
+import static org.elasticsearch.xpack.sql.util.DateUtils.UTC;


probably matter of aesthetics preference and aesthetics but wouldn't direct calls for DateUtils.UTC or DateUtils.asDateOnly be more obvious here?

+1 - in other places we call DateUtils explicit.

pgomulka · 2019-02-05T16:02:20Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java

-        DateTime dt = null;
+        ZonedDateTime zdt = null;
        try {
            DateTimeFormatter formatter = new DateTimeFormatterBuilder()


is there a Formatter in DateFormatters that we can use here?
if not, maybe we could make this static final field

+1 on making this a static field in DateUtils. I'm not aware of any pattern that matches this (it's basically ISO with ' ' instead of 'T'.

astefan

Looking good. Left few comments.

astefan · 2019-02-05T15:49:47Z

x-pack/plugin/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/TypeConverterTests.java


    public void testTimestampAsNative() throws Exception {
-        DateTime now = DateTime.now();
+        ZonedDateTime now = ZonedDateTime.now(Clock.tick(Clock.system(ZoneId.of("Z")), Duration.ofMillis(1)));


The same approach is used here maybe you can re-use that method.

Also, ZoneId.of("Z") seems to have a constant defined in DateUtils. Can you use that one or is not visible in this project?

jdbc module doesn't have access to DateUtils or TestUtils so both are not possible.

astefan · 2019-02-05T16:03:26Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java

        try {
-            dt = ISODateTimeFormat.hourMinuteSecond().parseDateTime(string);
-        } catch (IllegalArgumentException ex) {
+            lt = LocalTime.parse(string, ISO_LOCAL_TIME);


I see ISO_LOCAL_TIME deals, also, with format as HH:mm (not only HH:mm:ss) and the previous joda implementation seems to have looked at HH:mm:ss only... not sure if this is an issue or not, meaning if there are 0 seconds, does it return 12:13 only or 12:13:00?

Will dive into that when we implement the TIME data type, since now this method always throws an exception throw new SqlIllegalArgumentException("Time (only) literals are not supported; a date component is required as well");

I leave my comments, but don't want to block you tomorrow if others approve

costin

Left a number of comments.

costin

Looks good overall - left another round.

costin · 2019-02-05T19:19:01Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

            if (values instanceof String) {
                return DateUtils.asDateTime(Long.parseLong(values.toString()));
            }
-            // returned by nested types...


It can be removed now with Joda out. Likely this will eliminate asDateTime as well.

costin · 2019-02-05T19:19:41Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java

 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static org.elasticsearch.xpack.sql.type.DataTypeConversion.conversionFor;
+import static org.elasticsearch.xpack.sql.util.DateUtils.UTC;


+1 - in other places we call DateUtils explicit.

costin · 2019-02-05T19:29:57Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/util/DateUtils.java

    public static final ZoneId UTC = ZoneId.of("Z");
    public static final String DATE_PARSE_FORMAT = "epoch_millis";

+    private static final DateTimeFormatter UTC_DATE_FORMATTER = new DateTimeFormatterBuilder()


Indeed, it looks to be date_optional_time

matriv · 2019-02-06T11:25:39Z

@costin @astefan @pgomulka Thanks a lot for all the comments.
I've pushed a fixup and responded to the comments. Could you please check again?

matriv · 2019-02-06T12:44:26Z

By using DateFormatters.from() it's possible to use the date_optional_time.
@costin @pgomulka thanks a lot for checking that.

pgomulka

LGTM, thank you for looking into this!

costin

LGTM. Make sure to add the version tag as well.

astefan

LGTM

matriv · 2019-02-06T14:18:24Z

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/default-distro

matriv · 2019-02-06T14:40:12Z

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/default-distro

matriv · 2019-02-06T16:31:33Z

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/default-distro

Replace remaining usages of joda classes with java time. Fixes: #37703

matriv · 2019-02-08T21:24:12Z

Backported to 7.x with af8a444
to 7.0 with 6e769c1

* master: (1159 commits) Fix timezone fallback in ingest processor (elastic#38407) Avoid polluting download stats on builds (elastic#38660) SQL: Prevent grouping over grouping functions (elastic#38649) SQL: Relax StackOverflow circuit breaker for constants (elastic#38572) [DOCS] Fixes broken migration links (elastic#38655) Drop support for the low-level REST client on JDK 7 (elastic#38540) [DOCS] Adds placeholders for v8 highlights, breaking changes, release notes (elastic#38641) fix dissect doc "ip" --> "clientip" (elastic#38545) Concurrent file chunk fetching for CCR restore (elastic#38495) make DateMathIndexExpressionsIntegrationIT more resilient (elastic#38473) SQL: Replace joda with java time (elastic#38437) Add fuzziness example (elastic#37194) (elastic#38648) Mute AnalysisModuleTests#testStandardFilterBWC (elastic#38636) add geotile_grid ref to asciidoc (elastic#38632) Enable Dockerfile from artifacts.elastic.co (elastic#38552) Mute FollowerFailOverIT testFailOverOnFollower (elastic#38634) Account for a possible rolled over file while reading the audit log file (elastic#34909) Mute failure in InternalEngineTests (elastic#38622) Fix Issue with Concurrent Snapshot Init + Delete (elastic#38518) Refactor ZonedDateTime.now in millis resolution (elastic#38577) ...

* master: Fix timezone fallback in ingest processor (elastic#38407) Avoid polluting download stats on builds (elastic#38660) SQL: Prevent grouping over grouping functions (elastic#38649) SQL: Relax StackOverflow circuit breaker for constants (elastic#38572) [DOCS] Fixes broken migration links (elastic#38655) Drop support for the low-level REST client on JDK 7 (elastic#38540) [DOCS] Adds placeholders for v8 highlights, breaking changes, release notes (elastic#38641) fix dissect doc "ip" --> "clientip" (elastic#38545) Concurrent file chunk fetching for CCR restore (elastic#38495) make DateMathIndexExpressionsIntegrationIT more resilient (elastic#38473) SQL: Replace joda with java time (elastic#38437) Add fuzziness example (elastic#37194) (elastic#38648)

SQL: Replace joda with java time

af51ec6

Replace remaining usages of joda classes with java time. Fixes: elastic#37703

matriv added >non-issue :Analytics/SQL SQL querying labels Feb 5, 2019

matriv requested review from costin and pgomulka February 5, 2019 15:30

matriv commented Feb 5, 2019

View reviewed changes

matriv mentioned this pull request Feb 5, 2019

SQL: Fix issue with IN not resolving to underlying keyword field #38440

Merged

pgomulka previously requested changes Feb 5, 2019

View reviewed changes

pgomulka requested a review from spinscale February 5, 2019 16:11

astefan reviewed Feb 5, 2019

View reviewed changes

costin reviewed Feb 5, 2019

View reviewed changes

Address comments

bc77167

matriv added 2 commits February 6, 2019 14:27

Use the date_optional_time formatter instead of a custom one

88ef084

fix checkstyle

459c8db

pgomulka approved these changes Feb 6, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into mt/fix-37703

904c0c8

costin approved these changes Feb 6, 2019

View reviewed changes

matriv added the v7.0.0 label Feb 6, 2019

Merge remote-tracking branch 'upstream/master' into mt/fix-37703

8e06556

jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019

astefan approved these changes Feb 6, 2019

View reviewed changes

matriv added the v7.0.0 label Feb 6, 2019

matriv added the v7.2.0 label Feb 6, 2019

Merge remote-tracking branch 'upstream/master' into mt/fix-37703

cf9dc6b

matriv added 2 commits February 6, 2019 23:46

Merge remote-tracking branch 'upstream/master' into mt/fix-37703

e37ddc5

Merge remote-tracking branch 'upstream/master' into mt/fix-37703

153fa09

matriv merged commit d253fb3 into elastic:master Feb 8, 2019

matriv deleted the mt/fix-37703 branch February 8, 2019 20:55

matriv added a commit that referenced this pull request Feb 8, 2019

SQL: Replace joda with java time (#38437)

af8a444

Replace remaining usages of joda classes with java time. Fixes: #37703

matriv added a commit that referenced this pull request Feb 8, 2019

SQL: Replace joda with java time (#38437)

6e769c1

Replace remaining usages of joda classes with java time. Fixes: #37703

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 12, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

matriv commented Feb 5, 2019

Uh oh!

elasticmachine commented Feb 5, 2019

Uh oh!

matriv Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv commented Feb 6, 2019

matriv Feb 5, 2019 •

edited

Loading

matriv Feb 6, 2019 •

edited

Loading

matriv Feb 6, 2019 •

edited

Loading