-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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.
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
SQL: Replace joda with java time #38437
Conversation
Replace remaining usages of joda classes with java time. Fixes: elastic#37703
Pinging @elastic/es-search |
@@ -132,10 +131,6 @@ private Object unwrapMultiValue(Object values) { | |||
if (values instanceof String) { | |||
return DateUtils.asDateTime(Long.parseLong(values.toString())); | |||
} | |||
// returned by nested types... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@costin I'm not sure about this, but doesn't seem to break something. Should we check more in depth and add tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be removed now with Joda out. Likely this will eliminate asDateTime
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like we are close to finish this, looks good but left some comments
@@ -40,21 +41,17 @@ public void testDoubleAsNative() throws Exception { | |||
} | |||
|
|||
public void testTimestampAsNative() throws Exception { | |||
DateTime now = DateTime.now(); | |||
ZonedDateTime now = ZonedDateTime.now(Clock.tick(Clock.system(ZoneId.of("Z")), Duration.ofMillis(1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why using Clock here?
maybe we could just use
ZonedDateTime.now(ZoneOffset.UTC)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall correctly, it's because the clock approach had unpredictable behavior between jdk 8 and 11.
@astefan do you remember?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right - you guys made an excellent fix here #38437 (comment)
I had similar problems this morning with watcher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be the same if we use DateFormatters.forPattern("date_optional_time") ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might work (see the TODO above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I need the same formatter but with:
.parseDefaulting(HOUR_OF_DAY, 0)
.parseDefaulting(MINUTE_OF_HOUR, 0)
.parseDefaulting(SECOND_OF_MINUTE, 0)
@@ -82,7 +109,7 @@ public static ZonedDateTime asDateOnly(ZonedDateTime zdt) { | |||
* Parses the given string into a DateTime using UTC as a default timezone. | |||
*/ | |||
public static ZonedDateTime asDateTime(String dateFormat) { | |||
return asDateTime(UTC_DATE_FORMATTER.parseDateTime(dateFormat)); | |||
return asDateTime(Instant.from(UTC_DATE_FORMATTER.parse(dateFormat)).toEpochMilli()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about something like?
return DateFormatters.from(DATE_TIME_FORMATTER.parse(dateFormat)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I need return DateFormatters.from(UTC_DATE_FORMATTER.parse(dateFormat)).withZoneSameInstant(UTC);
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we need this (looks like date_optional_time to me) but if so, maybe we could have it in DateFormatters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it looks to be date_optional_time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my response here: #38437 (comment)
Is there an elegant way to overcome this when using the date_optional_time
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch that, I didn't check again after using DateFormatters.from
.
This method has the logic to handle the date only part.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably matter of aesthetics preference and aesthetics but wouldn't direct calls for DateUtils.UTC
or DateUtils.asDateOnly
be more obvious here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 - in other places we call DateUtils explicit.
@@ -820,18 +822,18 @@ public Literal visitTimestampEscapedLiteral(TimestampEscapedLiteralContext ctx) | |||
|
|||
Source source = source(ctx); | |||
// parse yyyy-mm-dd hh:mm:ss(.f...) | |||
DateTime dt = null; | |||
ZonedDateTime zdt = null; | |||
try { | |||
DateTimeFormatter formatter = new DateTimeFormatterBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a Formatter in DateFormatters
that we can use here?
if not, maybe we could make this static final field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Left few comments.
@@ -40,21 +41,17 @@ public void testDoubleAsNative() throws Exception { | |||
} | |||
|
|||
public void testTimestampAsNative() throws Exception { | |||
DateTime now = DateTime.now(); | |||
ZonedDateTime now = ZonedDateTime.now(Clock.tick(Clock.system(ZoneId.of("Z")), Duration.ofMillis(1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same approach is used here maybe you can re-use that method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, ZoneId.of("Z")
seems to have a constant defined in DateUtils
. Can you use that one or is not visible in this project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jdbc module doesn't have access to DateUtils
or TestUtils
so both are not possible.
try { | ||
dt = ISODateTimeFormat.hourMinuteSecond().parseDateTime(string); | ||
} catch (IllegalArgumentException ex) { | ||
lt = LocalTime.parse(string, ISO_LOCAL_TIME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a number of comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall - left another round.
@@ -132,10 +131,6 @@ private Object unwrapMultiValue(Object values) { | |||
if (values instanceof String) { | |||
return DateUtils.asDateTime(Long.parseLong(values.toString())); | |||
} | |||
// returned by nested types... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be removed now with Joda out. Likely this will eliminate asDateTime
as well.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 - in other places we call DateUtils explicit.
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it looks to be date_optional_time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for looking into this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Make sure to add the version tag as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@elasticmachine run elasticsearch-ci/1 |
@elasticmachine run elasticsearch-ci/1 |
@elasticmachine run elasticsearch-ci/1 |
Replace remaining usages of joda classes with java time. Fixes: #37703
Replace remaining usages of joda classes with java time. Fixes: #37703
* 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)
Replace remaining usages of joda classes with java time.
Fixes: #37703