Esql - Support date nanos in date extract function #120727
Conversation
|
Documentation preview: |
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @not-napoleon, I've created a changelog YAML for you. |
not-napoleon
added
the
auto-backport
| ChronoField chrono = chronoField(toEvaluator.foldCtx()); | ||
| if (chrono == null) { | ||
| BytesRef field = (BytesRef) children().get(0).fold(toEvaluator.foldCtx()); | ||
| throw new InvalidArgumentException("invalid date field for [{}]: {}", sourceText(), field.utf8ToString()); | ||
| } |
There was a problem hiding this comment.
A minor comment: these few lines seem to be duplicated with those under date_nanos field a few lines below, they can be refactored a bit further.
There was a problem hiding this comment.
I made a little refactor, and added a comment there. I feel like it's a bit simpler now. I have my doubts, but let's see what you think 👀
| return isStringAndExact(children().get(0), sourceText(), TypeResolutions.ParamOrdinal.FIRST).and( | ||
| isDate(children().get(1), sourceText(), TypeResolutions.ParamOrdinal.SECOND) | ||
| TypeResolutions.isType(children().get(1), DataType::isDate, operationName, TypeResolutions.ParamOrdinal.SECOND, "datetime") |
There was a problem hiding this comment.
In DateFormat we use "datetime or date_nanos" as the message
| List.of(DataType.TEXT, DataType.DATE_NANOS), | ||
| () -> new TestCaseSupplier.TestCase( | ||
| List.of( | ||
| new TestCaseSupplier.TypedData(new BytesRef("YeAr"), DataType.TEXT, "chrono"), |
There was a problem hiding this comment.
I would refactor those text/keyword tests with a for+DataType.stringTypes(), and add a case asking for nanos as the chrono field
| * @param zone - Timezone for the given date | ||
| * @return - long representing the given ChronoField value | ||
| */ | ||
| public static long chronoToLongNanos(long dateNanos, BytesRef chronoField, ZoneId zone) { |
There was a problem hiding this comment.
Optional: this method seems very specific, the BytesRef conversion could also be done in the processor. But even if kept here, once having a ChronoField, it would be good to reuse the chronoToLongNanos(long, ChronoField, ZoneId) below.
| return chronoToLong(value, chronoField, zone); | ||
| } | ||
|
|
||
| @Evaluator(extraName = "Nanos", warnExceptions = { IllegalArgumentException.class }) | ||
| static long processNanos(long value, BytesRef chronoField, @Fixed ZoneId zone) { | ||
| return chronoToLongNanos(value, chronoField, zone); |
There was a problem hiding this comment.
As mentioned before, an alternative to having the util that takes a BytesRef would be to convert that here (which would also make it clearer where the IllegalArgumentException could stem from) and reuse the processNanos(long, @Fixed ChronoField, @Fixed ZoneId) below.
There was a problem hiding this comment.
(Answering both comments)
Just pushed the change, reusing one chronoToLongNanos from the other. I didn't remove the second helper though, as both are really oneliners abstracting some logic (One the Instant.ofEpochMilli and the other the ChronoField.valueOf). So I feel like if we have one, the other makes sense too.
Also, many converters take BytesRef or Strings in that class, so also to follow the general ""standard"" there
Resolves elastic#110000 Add support for running the date extract function on nanosecond dates.
💚 Backport successful
|
…120908) * Esql - Support date nanos in date extract function (#120727) Resolves #110000 Add support for running the date extract function on nanosecond dates. * Fix switch error * ESQL: Fix DateExtract with nanos tests --------- Co-authored-by: Iván Cea Fontenla <[email protected]> Co-authored-by: Iván Cea Fontenla <[email protected]>
Resolves #110000
Add support for running the date extract function on nanosecond dates.