ESQL: Add ability to perform date math #98870

bpintea · 2023-08-25T09:56:50Z

This adds the ability to perform date math: allow date types be updated with a time span (a duration or a period).

Ex. : row n = now() | eval then = n - 1 year + 2 minutes

This adds the ability to perform date math: allow date types be updated with a time span (a duration or a period). Ex. : `row n = now() | eval then = n - 1 year + 2 minutes`

elasticsearchmachine · 2023-08-25T09:57:14Z

Hi @bpintea, I've created a changelog YAML for you.

elasticsearchmachine · 2023-08-25T09:57:15Z

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

elasticsearchmachine · 2023-08-25T09:57:15Z

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

alex-spies

LGTM, I only have minor remarks which are all meant to be optional.

Additionally, maybe it would make sense to add the following tests:

unit tests tests for period + datetime and duration + datetime
unit tests for period - datetime and duration - datetime
unit or csv tests for weird edge cases, like date math for leap years, adding days or hours past the end of a month, etc. Although this may be less important since we know that we can trust the date math functions from the ql package.

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

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypes.java

...csearch/xpack/esql/expression/predicate/operator/arithmetic/DateTimeArithmeticOperation.java

alex-spies · 2023-08-25T14:05:47Z

...csearch/xpack/esql/expression/predicate/operator/arithmetic/DateTimeArithmeticOperation.java

+        if (EsqlDataTypes.isDateTime(leftType) || EsqlDataTypes.isDateTime(rightType)) {
+            Expression dateTime = argumentOfType(dt -> dt == DataTypes.DATETIME);
+            Expression nonDateTime = argumentOfType(dt -> dt != DataTypes.DATETIME);
+            if (dateTime == null || nonDateTime == null || EsqlDataTypes.isTemporalAmount(nonDateTime.dataType()) == false) {


question: instead of using nonDateTime together with isTemporalAmount, wouldn't it be easier to just use argumentOfType(EsqlDataTypes::isTemporalAmount, like below in line 73?

costin

The main comment I have is around the use of timezones.
Date functions need to be time zone aware yet here the UTC tz is used - that makes sense only if the dates are considered to be coming from ES and thus they originate in UTC but the output is converted into the user TZ.
If that is not the case, this should be clarified.

Lastly some docs help - I would reuse the structure and content from SQL since its fairly similar.

costin · 2023-08-25T22:10:40Z

...src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/Add.java

+
+    @Evaluator(extraName = "Datetimes", warnExceptions = { ArithmeticException.class, DateTimeException.class })
+    static long processDatetimes(long datetime, @Fixed TemporalAmount temporalAmount) {
+        return asMillis(asDateTime(datetime, DEFAULT_TZ).plus(temporalAmount));


Why use UTC and not the request timezone?

I've added a comment.
Both timestamps that come from ES as well as those converted by our functions are in UTC.

costin · 2023-08-25T22:15:08Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/DateUtils.java

@@ -52,6 +52,14 @@ public static ZonedDateTime asDateTime(long millis) {
        return ZonedDateTime.ofInstant(Instant.ofEpochMilli(millis), UTC);
    }

+    public static ZonedDateTime asDateTime(long millis, ZoneId zoneId) {


Since you always pass UTC, why not use the above?

Remanent from a first attempt to use session's zoneID. Updated to use the existing one.

Add Add/Sub unit tests Make temporal units negatable. Rename data type tests methods. More tests.

bpintea · 2023-08-28T21:38:52Z

The main comment I have is around the use of timezones.
If that is not the case, this should be clarified.

I've added a comment.

Lastly some docs help - I would reuse the structure and content from SQL since its fairly similar.

Will follow with a subsequent PR.

bpintea · 2023-08-28T21:40:58Z

unit tests tests for period + datetime and duration + datetime

added.

unit tests for period - datetime and duration - datetime

Good point, thanks. Increased the Sub type resolution checks to prevent this.

unit or csv tests for weird edge cases, like date math for leap years, adding days or hours past the end of a month, etc. Although this may be less important since we know that we can trust the date math functions from the ql package.

Indeed, left this out.

costin

LGTM

alex-spies

Still LGTM, only added very minor remarks (all supposed to be completely optional). Did not look too closely into what appeared to be refactors of the test that probably became necessary due to #98890.

alex-spies · 2023-08-29T07:36:22Z

...src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/Add.java

@@ -85,6 +85,7 @@ static double processDoubles(double lhs, double rhs) {

    @Evaluator(extraName = "Datetimes", warnExceptions = { ArithmeticException.class, DateTimeException.class })
    static long processDatetimes(long datetime, @Fixed TemporalAmount temporalAmount) {
-        return asMillis(asDateTime(datetime, DEFAULT_TZ).plus(temporalAmount));
+        // using a UTC conversion since `datetime` is always a UTC-Epoch timestamp, either read from ES or converted through a function


praise: very helpful comment.

alex-spies · 2023-08-29T09:34:40Z

...est/java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/NegTests.java

@@ -112,14 +137,47 @@ public void testEdgeCases() {

            return;
        }
+        if (testCaseType == EsqlDataTypes.DATE_PERIOD) {
+            Period minPeriod = Period.of(Integer.MIN_VALUE, Integer.MIN_VALUE, Integer.MIN_VALUE);


praise: good call testing the Neg edge cases :)

...est/java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/NegTests.java

...est/java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/SubTests.java

...est/java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/AddTests.java

alex-spies · 2023-08-29T09:57:50Z

...ch/xpack/esql/expression/predicate/operator/comparison/AbstractBinaryComparisonTestCase.java

+        if (false == (lhsType == rhsType || lhsType.isNumeric() && rhsType.isNumeric())) {
+            return false;
+        }
+        return super.supportsTypes(lhsType, rhsType);


question (bikeshed-y, so not meant as a suggestion in any way): couldn't this just be

Suggested change

if (false == (lhsType == rhsType || lhsType.isNumeric() && rhsType.isNumeric())) {

return false;

}

return super.supportsTypes(lhsType, rhsType);

return (lhsType == rhsType || lhsType.isNumeric() && rhsType.isNumeric()))

&& super.supportsTypes(lhsType, rhsType);

Address review comments

Add ability to perform date math

e9a76cb

This adds the ability to perform date math: allow date types be updated with a time span (a duration or a period). Ex. : `row n = now() | eval then = n - 1 year + 2 minutes`

bpintea added >enhancement :Analytics/ES|QL AKA ESQL v8.11.0 labels Aug 25, 2023

elasticsearchmachine added the Team:QL (Deprecated) Meta label for query languages team label Aug 25, 2023

Update docs/changelog/98870.yaml

3188404

bpintea mentioned this pull request Aug 25, 2023

ESQL: Add ability to perform date math #98848

Closed

alex-spies approved these changes Aug 25, 2023

View reviewed changes

costin reviewed Aug 25, 2023

View reviewed changes

costin added the ES|QL-ui Impacts ES|QL UI label Aug 25, 2023

bpintea added 2 commits August 28, 2023 22:43

Add unit testing. Make temporal units negatable

9d21145

Add Add/Sub unit tests Make temporal units negatable. Rename data type tests methods. More tests.

Merge branch 'main' into esql/feature/date_math

39ff5d4

bpintea requested review from nik9000 and not-napoleon August 28, 2023 21:41

costin approved these changes Aug 29, 2023

View reviewed changes

alex-spies approved these changes Aug 29, 2023

View reviewed changes

bpintea added 2 commits August 29, 2023 12:52

Address review comments

e4fc696

Address review comments

Merge branch 'main' into esql/feature/date_math

3c40d03

bpintea merged commit 8c89f31 into elastic:main Aug 30, 2023

bpintea deleted the esql/feature/date_math branch August 30, 2023 10:55

dej611 mentioned this pull request Sep 7, 2023

ESQL: syntax error on date math if date period variable is at the end of the expression #99293

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Add ability to perform date math #98870

ESQL: Add ability to perform date math #98870

bpintea commented Aug 25, 2023

elasticsearchmachine commented Aug 25, 2023

elasticsearchmachine commented Aug 25, 2023

elasticsearchmachine commented Aug 25, 2023

alex-spies left a comment

alex-spies Aug 25, 2023

bpintea Aug 29, 2023

costin left a comment

costin Aug 25, 2023

bpintea Aug 28, 2023

costin Aug 25, 2023

bpintea Aug 28, 2023

bpintea commented Aug 28, 2023

bpintea commented Aug 28, 2023

costin left a comment

alex-spies left a comment

alex-spies Aug 29, 2023

alex-spies Aug 29, 2023

alex-spies Aug 29, 2023

ESQL: Add ability to perform date math #98870

ESQL: Add ability to perform date math #98870

Conversation

bpintea commented Aug 25, 2023

elasticsearchmachine commented Aug 25, 2023

elasticsearchmachine commented Aug 25, 2023

elasticsearchmachine commented Aug 25, 2023

alex-spies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea commented Aug 28, 2023

bpintea commented Aug 28, 2023

costin left a comment

Choose a reason for hiding this comment

alex-spies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment