[ESQL] Add TO_DATE_NANOS conversion function by not-napoleon · Pull Request #112150 · elastic/elasticsearch

not-napoleon · 2024-08-23T14:25:12Z

This adds a conversion function that yields DATE_NANOS. Mostly this is straight forward.

It is worth noting that when converting a millisecond date into a nanosecond date, the conversion function truncates it to 0 nanoseconds (i.e. first nanosecond of that millisecond). This is, of course, a bit of an assumption, but I don't have a better assumption we can make. I'd thought about adding a second, optional, parameter to control this behavior, but it's important that TO_DATE_NANOS extend AbstractConvertFunction, which itself extends UnaryScalarFunction, so that it will work correctly with union types. Also, it's unlikely the user will have any better guess than we do for filling in the nanoseconds.

Making that assumption does, however, create some weirdness. Consider two comparisons:

TO_DATETIME("2023-03-23T12:15:03.360103847") == TO_DATETIME("2023-03-23T12:15:03.360") will return true while TO_DATE_NANOS("2023-03-23T12:15:03.360103847") == TO_DATE_NANOS("2023-03-23T12:15:03.360") will return false. This is akin to casting between longs and doubles, where things may compare equal in one type that are not equal in the other. This seems fine, and I can't think of a better way to do it, but it's worth being aware of.

Conflicts: x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/EsqlFunctionRegistry.java

elasticsearchmachine · 2024-09-11T17:32:49Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

bpintea · 2024-09-16T14:59:14Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/EsqlFunctionRegistry.java

                def(ToCartesianShape.class, ToCartesianShape::new, "to_cartesianshape"),
                def(ToDatePeriod.class, ToDatePeriod::new, "to_dateperiod"),
                def(ToDatetime.class, ToDatetime::new, "to_datetime", "to_dt"),
+                def(ToDateNanos.class, ToDateNanos::new, "to_date_nanos", "to_datenanos"),


Should we add a to_datetime_nanos? If I were accustomed to to_datetime I'd expect a _nanos equivalent function.
Otherwise, I guess the name of the function maps on the datetype name, but I guess I do find it a tad confusing the difference between name types (some relevant comments found in #99006)

Personally, I don't think so. If I was going to add another alias, I'd opt for date_nanoseconds, to align with the title of https://www.elastic.co/guide/en/elasticsearch/reference/master/date_nanos.html, but really I think date_nanos is fine.

...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDateNanos.java

bpintea · 2024-09-16T15:12:58Z

...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDateNanos.java

+
+    @ConvertEvaluator(extraName = "FromLong", warnExceptions = { IllegalArgumentException.class })
+    static long fromLong(long in) {
+        if (in < 0) {


bpintea · 2024-09-16T16:12:40Z

...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDateNanos.java

+    }
+
+    @ConvertEvaluator(extraName = "FromString", warnExceptions = { IllegalArgumentException.class })
+    static long fromKeyword(BytesRef in) {


Would we want to have this method delegate to something in EsqlDataTypeConverter (similar to nanoTimeToString())? I imagine this conversion might be useful when we add support for parsing nanos or bucketing them.

So, this conversion actually exists as org.elasticsearch.common.time.DateUtils#toLong, and in retrospect I should probably just delegate to that.

bpintea

We should add the type to the EsqlDataTypeConverter.TYPE_TO_CONVERTER_FUNCTION mapping and add a few tests with the :: conversion operator.

astefan

Added few comments. Nothing major. Also:

some more tests in csv-spec would be welcomed:
- a foldable function like concat("2023-01-01","T12:12:12") should be used as part of eval or row and its value computed in a to_date_nanos or ::date_nanos.
- tests with null as a constant and as a computed value: to_date_nanos(null + 1::long), eval x = null, y = concat("2023", null) | limit 1 | stats max(123) by to_date_nanos(y)
- some constant that fails the range check: eval g = to_date_nanos(-5::long)
- tests with real indices data
some tests with union types (date and date_nanos for example, long and date_nanos)
date_nanos being a date data type, I think it's worth taking our date functions for a spin. I looked only at date_extract("nano_of_day", to_date_nanos(z)) which fails with second argument of [date_extract("nano_of_day", to_date_nanos(z))] must be [datetime], found value [to_date_nanos(z)] type [date_nanos] and it shouldn't. Indeed, I didn't want to extend the support you are adding with this PR and have it more contained, I only tried to test the new function in nested functions calls

astefan · 2024-09-16T17:17:09Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/date_nanos.csv-spec

+string to date nanos, out of range
+required_capability: date_nanos_type
+
+ROW d = TO_DATE_NANOS("2262-04-12T00:00:00.000") | KEEP d;


I see the syntax ::date_nanos is not supported (yet?). Is this intentional?

astefan · 2024-09-16T17:26:36Z

...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDateNanos.java

+        try {
+            return Math.multiplyExact(in, 1_000_000L);
+        } catch (ArithmeticException e) {
+            // This seems like a more useful error message than "Long Overflow"


...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDateNanos.java

ivancea · 2024-09-17T12:22:34Z

...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDateNanos.java

+
+    @ConvertEvaluator(extraName = "FromString", warnExceptions = { IllegalArgumentException.class })
+    static long fromKeyword(BytesRef in) {
+        Instant parsed = DateFormatters.from(DEFAULT_DATE_NANOS_FORMATTER.parse(in.utf8ToString())).toInstant();


Is something here throwing already on pre-1970 dates?
I don't see a test case for pre-1970 in ToDateNanosTests, would be nice adding it

I've updated this to use DateUtils.toLong() which does that check.

ivancea · 2024-09-17T12:23:52Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/date_nanos.csv-spec

Nit: I'm missing checks for pre-1970 dates for all types here. A single test checking all would be enough.
Nitpick because we already have the ToDateNanosTests cases, but I'd add it here too

I've added row tests for pre-1970 dates.

not-napoleon · 2024-09-18T15:56:19Z

@astefan

some tests with union types (date and date_nanos for example, long and date_nanos)

I have a separate ticket for this, as I expect it will be quite involved. #112885. That said, if you'd rather I did it in this PR, I would be fine with that. I just thought this PR was already getting pretty big.

date_nanos being a date data type, I think it's worth taking our date functions for a spin. I looked only at date_extract("nano_of_day", to_date_nanos(z)) which fails with second argument of [date_extract("nano_of_day", to_date_nanos(z))] must be [datetime], found value [to_date_nanos(z)] type [date_nanos] and it shouldn't. Indeed, I didn't want to extend the support you are adding with this PR and have it more contained, I only tried to test the new function in nested functions calls

Indeed, passing date nanos into date functions is not (yet) supported. The roadmap for the feature lists that work further down: #109352. At the moment we have it marked as not required for an MVP (i.e. before taking off the feature flag), but we can move it into the per-release work if we want to. It will definitely be required if we implement the auto casting into date nanos that we talked about recently.

astefan · 2024-09-18T15:58:01Z

I have a separate ticket for this, as I expect it will be quite involved. #112885.

That's ok 👍

astefan

LGTM

not-napoleon · 2024-09-26T13:23:05Z

@elasticmachine update branch

elasticsearchmachine · 2024-09-26T16:04:44Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 112150

Resolves elastic#111842 This adds a conversion function that yields DATE_NANOS. Mostly this is straight forward. It is worth noting that when converting a millisecond date into a nanosecond date, the conversion function truncates it to 0 nanoseconds (i.e. first nanosecond of that millisecond). This is, of course, a bit of an assumption, but I don't have a better assumption we can make. I'd thought about adding a second, optional, parameter to control this behavior, but it's important that TO_DATE_NANOS extend AbstractConvertFunction, which itself extends UnaryScalarFunction, so that it will work correctly with union types. Also, it's unlikely the user will have any better guess than we do for filling in the nanoseconds. Making that assumption does, however, create some weirdness. Consider two comparisons: TO_DATETIME("2023-03-23T12:15:03.360103847") == TO_DATETIME("2023-03-23T12:15:03.360") will return true while TO_DATE_NANOS("2023-03-23T12:15:03.360103847") == TO_DATE_NANOS("2023-03-23T12:15:03.360") will return false. This is akin to casting between longs and doubles, where things may compare equal in one type that are not equal in the other. This seems fine, and I can't think of a better way to do it, but it's worth being aware of. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Resolves #111842 This adds a conversion function that yields DATE_NANOS. Mostly this is straight forward. It is worth noting that when converting a millisecond date into a nanosecond date, the conversion function truncates it to 0 nanoseconds (i.e. first nanosecond of that millisecond). This is, of course, a bit of an assumption, but I don't have a better assumption we can make. I'd thought about adding a second, optional, parameter to control this behavior, but it's important that TO_DATE_NANOS extend AbstractConvertFunction, which itself extends UnaryScalarFunction, so that it will work correctly with union types. Also, it's unlikely the user will have any better guess than we do for filling in the nanoseconds. Making that assumption does, however, create some weirdness. Consider two comparisons: TO_DATETIME("2023-03-23T12:15:03.360103847") == TO_DATETIME("2023-03-23T12:15:03.360") will return true while TO_DATE_NANOS("2023-03-23T12:15:03.360103847") == TO_DATE_NANOS("2023-03-23T12:15:03.360") will return false. This is akin to casting between longs and doubles, where things may compare equal in one type that are not equal in the other. This seems fine, and I can't think of a better way to do it, but it's worth being aware of. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

not-napoleon added 4 commits August 21, 2024 09:16

I don't understand why this doesn't work

4f948a1

visibility. That's why it doesn't work.

2f768b2

working for relevant types

f268544

bounds check strings also

96cdc78

not-napoleon added >non-issue WIP :Analytics/ES|QL AKA ESQL v8.16.0 labels Aug 23, 2024

not-napoleon added 12 commits September 4, 2024 09:16

Merge branch 'main' into esql-to-date-nanos

55887a8

lots of tests, some still failing

71ce9fb

fix serialization and evaluator strings

ba7d8a8

actually fix evaluator strings

68bc403

fix input type

344298a

spotless apply

8eb9bac

fix a bunch of exception types and messages

c9eb39e

fix function description

856aa79

Merge branch 'main' into esql-to-date-nanos

0f3e763

Conflicts: x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/EsqlFunctionRegistry.java

spotless apply

1fab66a

fix argument list description

85ecb09

fix meta tests

016bb1c

not-napoleon marked this pull request as ready for review September 11, 2024 13:36

not-napoleon requested a review from a team as a code owner September 11, 2024 13:36

mark-vieira added v9.0.0 and removed v8.16.0 labels Sep 11, 2024

not-napoleon added auto-backport-and-merge v8.16.0 and removed WIP labels Sep 11, 2024

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 11, 2024

not-napoleon mentioned this pull request Sep 13, 2024

[ESQL] Testing for date nanos union type #112885

Closed

bpintea reviewed Sep 16, 2024

View reviewed changes

astefan reviewed Sep 16, 2024

View reviewed changes

Merge branch 'main' into esql-to-date-nanos

6510a45

ivancea reviewed Sep 17, 2024

View reviewed changes

Merge branch 'main' into esql-to-date-nanos

3c65275

not-napoleon added 7 commits September 19, 2024 15:52

index-based csv tests for a couple of conversions

2f8f613

Merge branch 'main' into esql-to-date-nanos

c930b9f

use DateUtils for string conversion

98b3761

deal with a few more pre 1970 cases

968e390

wire up :: notation

6d81306

null testing in CSV tests

3536d64

add note and regenerate docs

98ec7c2

not-napoleon requested review from astefan, bpintea, ivancea and nik9000 September 24, 2024 20:19

spotless apply

819e520

astefan approved these changes Sep 25, 2024

View reviewed changes

elasticmachine and others added 2 commits September 26, 2024 15:23

Merge branch 'main' into esql-to-date-nanos

b827bd0

add a new capability

f24c5e3

not-napoleon merged commit 122e728 into elastic:main Sep 26, 2024

elasticsearchmachine added the backport pending label Sep 26, 2024

not-napoleon mentioned this pull request Sep 26, 2024

[ESQL] Add TO_DATE_NANOS conversion function (#112150) #113641

Merged

Conversation

not-napoleon commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

bpintea left a comment

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!

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!

not-napoleon commented Sep 18, 2024

Uh oh!

astefan commented Sep 18, 2024

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

not-napoleon commented Sep 26, 2024

Uh oh!

elasticsearchmachine commented Sep 26, 2024

💔 Backport failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

not-napoleon commented Aug 23, 2024 •

edited

Loading