Esql Support date nanos on date diff function#120645
Esql Support date nanos on date diff function#120645not-napoleon merged 14 commits intoelastic:mainfrom
Conversation
not-napoleon
added
>enhancement
auto-backport
|
Documentation preview: |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
|
Hi @not-napoleon, I've created a changelog YAML for you. |
ivancea
left a comment
ivancea
left a comment
There was a problem hiding this comment.
LGTM! Commented some nits and some improvements on testing
|
|
||
| FROM date_nanos | ||
| | EVAL n = MV_MAX(nanos) | ||
| | EVAL diff_sec = DATE_DIFF("seconds", TO_DATE_NANOS("2023-10-23T12:15:03.360103847Z"), n) |
There was a problem hiding this comment.
Should we add two extra cases for the combinations of millis and nanos? Maybe in this same test, as extra columns
There was a problem hiding this comment.
I've added one such.
| ) | ||
| ), | ||
| new TestCaseSupplier( | ||
| "DateDiff(" + unit + "<TEXT>, " + startTimestamp + "<NANOS>, " + endTimestamp + "<NANOS>) == " + expected, |
There was a problem hiding this comment.
Nit: We can merge the test andkeyword in a for over DataType.stringTypes(). Not related with this PR, but now that there are that many cases, would be a quick improvement
There was a problem hiding this comment.
Yeah, there's a lot of test refactoring we could do. I just don't have time to do it right now.
| } | ||
|
|
||
| @Evaluator(extraName = "ConstantNanosMillis", warnExceptions = { IllegalArgumentException.class, InvalidArgumentException.class }) | ||
| static int processNanosMillis(@Fixed Part datePartFieldUnit, long startTimestamp, long endTimestamp) throws IllegalArgumentException { |
There was a problem hiding this comment.
nit: I would suffix the parameters like "startTimestampNanos", to clearly see which one is each. For future readability mostly, as the only way to know it here is the method name
| if (startTimestamp.dataType() == DATETIME && endTimestamp.dataType() == DATETIME) { | ||
| return toEvaluator(toEvaluator, DateDiffConstantMillisEvaluator.Factory::new, DateDiffMillisEvaluator.Factory::new); | ||
| } else if (startTimestamp.dataType() == DATE_NANOS && endTimestamp.dataType() == DATE_NANOS) { | ||
| return toEvaluator(toEvaluator, DateDiffConstantNanosEvaluator.Factory::new, DateDiffNanosEvaluator.Factory::new); | ||
| } else if (startTimestamp.dataType() == DATE_NANOS && endTimestamp.dataType() == DATETIME) { | ||
| return toEvaluator(toEvaluator, DateDiffConstantNanosMillisEvaluator.Factory::new, DateDiffNanosMillisEvaluator.Factory::new); | ||
| } else if (startTimestamp.dataType() == DATETIME && endTimestamp.dataType() == DATE_NANOS) { | ||
| return toEvaluator(toEvaluator, DateDiffConstantMillisNanosEvaluator.Factory::new, DateDiffMillisNanosEvaluator.Factory::new); | ||
| } | ||
| throw new UnsupportedOperationException("How'd we get here?"); |
There was a problem hiding this comment.
This is technically impossible as long as the resolveType and the rules/ESQL is right, but I would show the startTimestamp and endTimestamp types in the error, just in case
| TypeResolution resolution = isString(unit, sourceText(), FIRST).and(isDate(startTimestamp, sourceText(), SECOND)) | ||
| .and(isDate(endTimestamp, sourceText(), THIRD)); | ||
| String operationName = sourceText(); | ||
| String operationName1 = sourceText(); |
There was a problem hiding this comment.
I suppose this isn't supposed to be here
|
|
||
| FROM date_nanos | ||
| | EVAL n = MV_MAX(nanos) | ||
| | EVAL diff_sec = DATE_DIFF("seconds", TO_DATE_NANOS("2023-10-23T12:15:03.360103847Z"), n) |
There was a problem hiding this comment.
Also, maybe another test with the first param coming from an index, as we don't test it here. From an index, or from a CASE(). Whatever, as long as it's not a constant
…os-date-diff' into esql-date-nanos-date-diff
Resolves elastic#109999 This adds support for date nanos in the date diff function, as well as mixed nanos/millis use cases. --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>
💚 Backport successful
|
3 participants
Resolves #109999
This adds support for date nanos in the date diff function, as well as mixed nanos/millis use cases.