Esql/create DATE_DIFF function by nicolasgras · Pull Request #103208 · elastic/elasticsearch

nicolasgras · 2023-12-08T15:07:31Z

This pull request resolves #101942.

A new functionality DATE_DIFF is added to ESQL. This functionaliy is similar to the ones in SQL plugin.

It calculates the diffenrence between this 2 dates using a unit specified and takes 3 input parameters:

unit (text)
startTimestamp (dateTime)
endTimestamp (dateTime)

github-actions · 2023-12-08T15:08:53Z

Documentation preview:

✨ Changed pages

cla-checker-service · 2023-12-08T15:09:01Z

💚 CLA has been signed

elasticsearchmachine · 2023-12-19T13:52:39Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2023-12-19T13:52:40Z

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

bpintea

Thanks for your interest and contribution, @nicolasgras.
I had a first pass. My main question is about the code duplication. Any reason you didn't extract the common parts (in the ql module) and reused it here?

bpintea · 2023-12-21T11:32:29Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateDiff.java

+            return aliases;
+        }
+
+        private static int safeInt(long diff) {


There's DataTypeConverter#safeToInt available. But I'm mostly wondering why not return long.

I have updated the code to use DataTypeConverter#safeToInt and removed the one I implemented that was identical.

bpintea · 2023-12-21T11:34:30Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateDiff.java

+
+        static {
+            NAME_TO_PART = DateTimeField.initializeResolutionMap(values());
+            VALID_VALUES = DateTimeField.initializeValidValues(values());


This isn't used anywhere atm. I guess the initialization of NAME_TO_PART can then be inlined too.

I agree, VALID_VALUES List is not used. I should have cleant this part.
The initialization of NAME_TO_PART is inlined now.

bpintea · 2023-12-21T11:43:16Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateDiff.java

+        private BiFunction<ZonedDateTime, ZonedDateTime, Integer> diffFunction;
+        private Set<String> aliases;
+


"may be final"

I agree, they are final now.

bpintea · 2023-12-21T12:15:44Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateDiff.java

+        description = "Subtract 2 dates and return their difference in multiples of a unit specified in the 1st argument")
+    public DateDiff(
+        Source source,
+        @Param(name = "unit", type = { "text" }, description = "A valid date unit") Expression unit,


The type can also be keyword.

I agree, I have updated the code.

bpintea · 2023-12-21T12:18:06Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateDiff.java

+        this.endTimestamp = endTimestamp;
+    }
+
+    @Evaluator


We'll want to catch the thrown exceptions in the evaluator implementor so that we can return a null, rather than stop the execution. Have a look at DateParse for an example.

I agree, I have updated the code.

bpintea · 2023-12-21T12:35:00Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateDiff.java

+        ZonedDateTime zdtStart = ZonedDateTime.ofInstant(Instant.ofEpochMilli(startTimestamp), UTC);
+        ZonedDateTime zdtEnd = ZonedDateTime.ofInstant(Instant.ofEpochMilli(endTimestamp), UTC);
+
+        Part datePartField = Part.resolve(unit.utf8ToString());


We could maybe have two evaluators here: in case you'd do smth like date_diff(some_string_field, ..., ...), then we'd need to evaluate the first parameter to establish what's the unit we need to apply, as done already. But if we write something like date_diff("hours", ..., ...), we can do this work upfront, not with every evaluation. And it's likely this would be the most common use case.

I have updated the code by adding a new evaluator (I get something similar to DateParse now).

bpintea · 2023-12-21T12:38:15Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateDiff.java

+            type = { "date" },
+            description = "A string representing a start timestamp"
+        ) Expression startTimestamp,
+        @Param(name = "endTimestamp", type = { "date" }, description = "A string representing a end timestamp") Expression endTimestamp


aN end timestamp

This typo has been fixed.

bpintea · 2023-12-21T13:18:52Z

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateDiff.java

+        if (datePartField == null) {
+            List<String> similar = Part.findSimilar(unit.utf8ToString());
+            if (similar.isEmpty()) {
+                throw new InvalidArgumentException(


A nit on style: you could prepare the message in the two branches and throw just once.

I agree, I have updated the code.

nicolasgras · 2023-12-26T04:36:14Z

Thanks for your interest and contribution, @nicolasgras. I had a first pass. My main question is about the code duplication. Any reason you didn't extract the common parts (in the ql module) and reused it here?

Thank you for you review @bpintea.
As this issue is about esql, I thought I should not update any other plugin. But it is better to use it in a common module.
As you suggest, I have moved DateTimeFiled interface in ql module. Maybe let's keep in mind to also use it in sql module if this PR is accepted and merged.

I have also added unit tests in esql/qa/testFixtures directory to have a better code coverage.

Let me know if it is ok for you.

luigidellaquila

Thank you very much @nicolasgras!
Just two comments from my side:

I see the CI still complains about CLA signature; I checked your commits and I see they appear with two different email addresses (one is invalid of course):

Author: Nicolas Gras <nicolasgras01@gmail.com>
Author: Nicolas Gras <nicolas@system76-pc.localdomain>

You have to sign the CLA with both to make the checks green, even though the second one is obviously a local one.

I think @bpintea referred to the content of org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateDiff (the enum Part and some logic) when he asked about common parts that can be extracted to QL.
IMHO it's not mandatory, we could also keep the two things separated and have this PR self-contained in ESQL module, but in that case I'd avoid to move DateTimeField to QL

elasticsearchmachine · 2024-01-02T19:48:57Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

nicolasgras · 2024-01-03T06:12:14Z

Thank you @luigidellaquila for your review.

You have to sign the CLA with both to make the checks green, even though the second one is obviously a local one.

I have rebased my commits on top of the branch and updated the author of them, then the CI does not complain anymore about CLA signature.

I think @bpintea referred to the content of org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateDiff (the enum Part and some logic) when he asked about common parts that can be extracted to QL.
IMHO it's not mandatory, we could also keep the two things separated and have this PR self-contained in ESQL module, but in that case I'd avoid to move DateTimeField to QL.

@bpintea , what do you think about it?
After your review I updated and cleant org.elasticsearch.xpack.esql.expression.function.scalar.date.DateDiff.Part, even if it is similar to org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateDiff.Part, they are a little different now.
And do you think we should move back DateTimeField to ESQL?

bpintea · 2024-01-03T12:31:42Z

@bpintea , what do you think about it?

In some cases sharing code between other *ql modules within the ql one makes things restrictive/tricky. If the code is very similar, it makes sense, however. But I see you've used some SDK functionality over custom implementation, which is preferred, and if that covers the needs for this PR, I'm OK containing the changes to the esql module, as @luigidellaquila suggested. (We can extract these changes in QL and update SQL later if needed.)

nicolasgras · 2024-01-04T04:25:15Z

@bpintea and @luigidellaquila, I have moved back DateTimeField interface into ESQL.
An issue could be created later to extract some code in QL and rework some parts in SQL if needed.
Is it ok for you?

bpintea

I've left two notes, otherwise it LGTM.

I guess with SQL's DateDiff we've follow other implementations returning int and we follow suite also here. This is OK, though we might want to update it, if we'll add support for nano timestamps (which we don't have yet), in which case an int will not cover even 3s.

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateDiff.java

bpintea

Thanks, LGTM.

luigidellaquila

Thank you very much @nicolasgras, LGTM

This reverts commit ec2e185.

DaveCTurner · 2024-01-09T09:54:49Z

Unfortunately this commit broke the build (checkstyle violations) so I reverted it in 943b2ea. Luigi will open a PR to reinstate it.

luigidellaquila · 2024-01-09T10:15:40Z

#104118

Same as #103208 Fixes #101942 We had to revert it after a Checkstyle failure (strange it didn't pop up in the CI before merging)

elasticsearchmachine added needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team v8.13.0 labels Dec 8, 2023

kingherc added the :Analytics/ES|QL AKA ESQL label Dec 19, 2023

elasticsearchmachine added Team:QL (Deprecated) Meta label for query languages team and removed needs:triage Requires assignment of a team area label labels Dec 19, 2023

costin requested a review from luigidellaquila December 19, 2023 16:58

bpintea reviewed Dec 21, 2023

View reviewed changes

nicolasgras requested a review from bpintea December 26, 2023 04:36

luigidellaquila added the >feature label Jan 2, 2024

luigidellaquila self-assigned this Jan 2, 2024

luigidellaquila reviewed Jan 2, 2024

View reviewed changes

wchaparro added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 2, 2024

elasticsearchmachine removed the Team:QL (Deprecated) Meta label for query languages team label Jan 2, 2024

nicolasgras added 8 commits January 3, 2024 00:20

Create DateDiff class in ESQL plugin

68c7448

Add date_diff function in ESQL plugin

6f54b9b

Update date_diff function in csv-spec

c2a8c19

Update doc with date_diff in ESQL

b5ba7b7

Add changelog file for issue 101942

149bf8b

Update code after code review

b3427b8

Remove date_diff tests from docs.csv-spec file

6acad80

Merge branch 'main' into esql/create-date-diff-function

bb62dfd

nicolasgras force-pushed the esql/create-date-diff-function branch from 0bf177d to bb62dfd Compare January 3, 2024 05:41

nicolasgras requested a review from luigidellaquila January 3, 2024 06:12

Rollback: DateTimeField interface in esql plugin

61986a2

bpintea reviewed Jan 8, 2024

View reviewed changes

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateDiff.java Outdated Show resolved Hide resolved

...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateDiff.java Outdated Show resolved Hide resolved

Update code after review

e1ebed9

nicolasgras requested a review from bpintea January 9, 2024 05:01

bpintea approved these changes Jan 9, 2024

View reviewed changes

luigidellaquila approved these changes Jan 9, 2024

View reviewed changes

luigidellaquila merged commit ec2e185 into elastic:main Jan 9, 2024

DaveCTurner added a commit that referenced this pull request Jan 9, 2024

Revert "Esql/create DATE_DIFF function (#103208)"

943b2ea

This reverts commit ec2e185.

luigidellaquila mentioned this pull request Jan 9, 2024

ESQL: add date_diff function #104118

Merged

elasticsearchmachine pushed a commit that referenced this pull request Jan 9, 2024

ESQL: add date_diff function (#104118)

770fc19

Same as #103208 Fixes #101942 We had to revert it after a Checkstyle failure (strange it didn't pop up in the CI before merging)

abdonpijpelink mentioned this pull request Jan 26, 2024

[DOCS] Add DATE_DIFF function #104794

Merged

		private BiFunction<ZonedDateTime, ZonedDateTime, Integer> diffFunction;
		private Set<String> aliases;

Conversation

nicolasgras commented Dec 8, 2023

Uh oh!

github-actions bot commented Dec 8, 2023

Uh oh!

cla-checker-service bot commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Dec 19, 2023

Uh oh!

elasticsearchmachine commented Dec 19, 2023

Uh oh!

bpintea left a comment • 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!

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!

nicolasgras commented Dec 26, 2023

Uh oh!

luigidellaquila left a comment

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jan 2, 2024

Uh oh!

nicolasgras commented Jan 3, 2024

Uh oh!

bpintea commented Jan 3, 2024

Uh oh!

nicolasgras commented Jan 4, 2024

Uh oh!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

luigidellaquila left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Jan 9, 2024

Uh oh!

luigidellaquila commented Jan 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

cla-checker-service bot commented Dec 8, 2023 •

edited

Loading

bpintea left a comment •

edited

Loading