Adding convert_tz and datetime functions to SQL and PPL #109

MitchellGale · 2022-08-19T16:40:25Z

Description

PR adds support for convert_tz and Datetime for PPL and SQL languages.

convert_tz

Syntax for convert_tz:
convert_tz(datetime string, from timezone, to timezone)
It converts the datetime string from the from timezone to the to timezone. Converts from negative to positive timezone, from positive to negative, from positive to positive or negative to negative time zones. Works across year roll-over and leap years.
Example:

opensearchsql> SELECT convert_tz('2021-05-12 00:00:00','-12:00','+13:00'); 
fetched rows / total rows = 1/1
+-------------------------------------------------------+
| convert_tz('2021-05-12 00:00:00','-12:00','+13:00')   |
|-------------------------------------------------------|
| 2021-05-13 01:00:00                                   |
+-------------------------------------------------------+

opensearchsql> SELECT convert_tz('2021-05-12 12:34:56','-00:00','+01:00');
fetched rows / total rows = 1/1
+-------------------------------------------------------+
| convert_tz('2021-05-12 12:34:56','-00:00','+01:00')   |
|-------------------------------------------------------|
| 2021-05-12 13:34:56                                   |
+-------------------------------------------------------+

nulls

It returns null for time zones outside of the allowed range (allow range is -12:00 -> +14:00)

opensearchsql> SELECT convert_tz('2021-05-12 12:34:56','-00:00','+15:00');
fetched rows / total rows = 1/1
+-------------------------------------------------------+
| convert_tz('2021-05-12 12:34:56','-00:00','+15:00')   |
|-------------------------------------------------------|
| null                                                  |
+-------------------------------------------------------+

opensearchsql> SELECT convert_tz('2021-05-12 12:34:56','-13:00','+10:00');
fetched rows / total rows = 1/1
+-------------------------------------------------------+
| convert_tz('2021-05-12 12:34:56','-13:00','+10:00')   |
|-------------------------------------------------------|
| null                                                  |
+-------------------------------------------------------+

Leap year

opensearchsql> SELECT convert_tz('2005-02-28 20:00:00','-10:00','+10:00');
fetched rows / total rows = 1/1
+-------------------------------------------------------+
| convert_tz('2005-02-28 20:00:00','-10:00','+10:00')   |
|-------------------------------------------------------|
| 2005-03-01 16:00:00                                   |
+-------------------------------------------------------+

opensearchsql> SELECT convert_tz('2004-02-28 20:00:00','-10:00','+10:00');
fetched rows / total rows = 1/1
+-------------------------------------------------------+
| convert_tz('2004-02-28 20:00:00','-10:00','+10:00')   |
|-------------------------------------------------------|
| 2004-02-29 16:00:00                                   |
+-------------------------------------------------------+

DateTime

DATETIME(timestamp_expression [, time_zone])

fetched rows / total rows = 1/1
+---------------------------------------------------+
| DATETIME('2008-12-25 05:30:00-05:00', '+05:00')   |
|---------------------------------------------------|
| 2008-12-25 15:30:00                               |
+---------------------------------------------------+

opensearchsql> SELECT DATETIME('2008-12-25 05:30:00', '+05:00');
fetched rows / total rows = 1/1
+---------------------------------------------+
| DATETIME('2008-12-25 05:30:00', '+05:00')   |
|---------------------------------------------|
| 2008-12-25 10:30:00                         |
+---------------------------------------------+

opensearchsql> SELECT DATETIME('2008-12-25 05:30:00+10:00'); 
fetched rows / total rows = 1/1
+-----------------------------------------+
| DATETIME('2008-12-25 05:30:00+10:00')   |
|-----------------------------------------|
| 2008-12-25 05:30:00                     |
+-----------------------------------------+

opensearchsql> SELECT DATETIME('2008-12-25 05:30:00');
fetched rows / total rows = 1/1
+-----------------------------------+
| DATETIME('2008-12-25 05:30:00')   |
|-----------------------------------|
| 2008-12-25 05:30:00               |
+-----------------------------------+

Issues Resolved

#703
#46

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov · 2022-08-19T16:44:03Z

Codecov Report

Merging #109 (45a6d2e) into integ-Add-convert_tz-function (929ebfe) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@                         Coverage Diff                         @@
##             integ-Add-convert_tz-function     #109      +/-   ##
===================================================================
+ Coverage                            94.83%   94.87%   +0.04%     
- Complexity                            2916     2932      +16     
===================================================================
  Files                                  291      291              
  Lines                                 7795     7857      +62     
  Branches                               567      574       +7     
===================================================================
+ Hits                                  7392     7454      +62     
  Misses                                 349      349              
  Partials                                54       54

Flag	Coverage Δ
query-workbench	`62.76% <ø> (ø)`
sql-engine	`97.80% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...g/opensearch/sql/data/model/ExprDatetimeValue.java	`100.00% <100.00%> (ø)`
...c/main/java/org/opensearch/sql/expression/DSL.java	`100.00% <100.00%> (ø)`
...arch/sql/expression/datetime/DateTimeFunction.java	`100.00% <100.00%> (ø)`
...h/sql/expression/function/BuiltinFunctionName.java	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

docs/user/ppl/functions/datetime.rst

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

Yury-Fridlyand · 2022-08-19T18:05:49Z

Add tests with non-round-hour-offset timzones, like Asia/Katmandu +05:45, Asia/Kabul +04:30, Australia/Broken_Hill +09:30.
And a test where TZ shift is exactly 24 hours - you should get the same time, but on another date.
Non existing TZ

Other optional tests:

Switch between winter/summer timezones when they have shift
Switch from a TZ to the same TZ

Please, keep in mind, we don't test java, but we should ensure that user will get a valid result or a clear error message of incorrect input/limitation.

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

docs/user/ppl/functions/datetime.rst

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

MaxKsyunz · 2022-08-26T00:14:46Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

@@ -679,4 +780,29 @@ private ExprValue exprYear(ExprValue date) {
    return new ExprIntegerValue(date.dateValue().getYear());
  }

+  private Boolean isTimeZoneValid(ZoneId zone) {


Isn't every ZoneId a valid time zone identifier? Why do we need this validation?

If we do, needs unit tests to show which subset of ZoneIds is valid and which are invalid.

The valid/invalid test is just to check if the timezone is outside of the range
+14 and - 13:59. Also needs validation if the input was actually a valid timezone, if not it should return null too. Current commit doesn't have it, being pushed soon.

I'd like to follow-up on this. As far as I can see, in Java, every ZoneId instance is a valid time zone.

mysql> select convert_tz("2000-01-01 10:00:00", "+14:00", "+00:00"); +-------------------------------------------------------+ | convert_tz("2000-01-01 10:00:00", "+14:00", "+00:00") | +-------------------------------------------------------+ | 1999-12-31 20:00:00 | +-------------------------------------------------------+ 1 row in set (0.00 sec) mysql> select convert_tz("2000-01-01 10:00:00", "+14:01", "+00:00"); +-------------------------------------------------------+ | convert_tz("2000-01-01 10:00:00", "+14:01", "+00:00") | +-------------------------------------------------------+ | NULL | +-------------------------------------------------------+ 1 row in set (0.00 sec) mysql> select convert_tz("2000-01-01 10:00:00", "-13:59", "+00:00"); +-------------------------------------------------------+ | convert_tz("2000-01-01 10:00:00", "-13:59", "+00:00") | +-------------------------------------------------------+ | 2000-01-01 23:59:00 | +-------------------------------------------------------+ 1 row in set (0.00 sec) mysql> select convert_tz("2000-01-01 10:00:00", "-14:00", "+00:00"); +-------------------------------------------------------+ | convert_tz("2000-01-01 10:00:00", "-14:00", "+00:00") | +-------------------------------------------------------+ | NULL | +-------------------------------------------------------+ 1 row in set (0.00 sec)

The validator is to validate the timezones that match with MySQL standard. Java can handle all timezones being accepted and converted, but it won't match MySQL standard.

If that's the case, we need to rename this as isValidMySqlTimeZoneId is similar

See my comment above where the function is being used.

Function name updated to isValidMySqlTimeZoneId.

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

integ-test/src/test/java/org/opensearch/sql/ppl/DateTimeFunctionIT.java

integ-test/src/test/java/org/opensearch/sql/sql/DateTimeFunctionIT.java

Yury-Fridlyand · 2022-09-02T01:19:14Z

opensearchsql> SELECT convert_tz('2021-02-30 12:34:56','UTC','+03:00');
fetched rows / total rows = 1/1
+----------------------------------------------------+
| convert_tz('2021-02-30 12:34:56','UTC','+03:00')   |
|----------------------------------------------------|
| 2021-02-28 15:34:56                                |
+----------------------------------------------------+
opensearchsql> SELECT DATETIME('2021-02-30 12:34:56');
fetched rows / total rows = 1/1
+-----------------------------------+
| DATETIME('2021-02-30 12:34:56')   |
|-----------------------------------|
| 2021-02-28 12:34:56               |
+-----------------------------------+

Queries on Feb 30 don't fail. A bug?

Yury-Fridlyand · 2022-09-02T01:22:48Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

+  /**
+   * DateTime implementation for ExprValue.
+   *
+   * @param dateTime ExprValue of String type.
+   * @return ExprValue of date type.
+   */


Please, describe both params in javadoc

Second parameter added to java doc. Java doc also added to exprDateTimeNoTimezone function.

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java

MaxKsyunz · 2022-09-01T21:40:30Z

integ-test/src/test/java/org/opensearch/sql/ppl/DateTimeFunctionIT.java

@@ -463,4 +462,4 @@ public void testDateFormatISO8601() throws IOException {
    String dateFormatted = "1998-01-31T00:00:00Z";
    verifyDateFormat(date, "date", dateFormat, dateFormatted);
  }
-}
+}


newline would be great here.

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

MaxKsyunz · 2022-09-02T01:37:22Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

+   * @param dateTime ExprValue of String type.
+   * @return ExprValue of date type.
+   */
+  private ExprValue exprDateTime(ExprValue dateTime, ExprValue timeZone) {


Let's discuss this method. I'd like to understand a few things:

How do we know dateTime can be successfully parsed? This is done in two places without try-catch, yet it was parsing was wrapped in try-catch elsewhere.

We are checking if timeZone is null but not dateTime -- how come?

If timeZone.isNull block is special case only for exprDateTimeNoTZ then it needs to move to that method.

try block on line 549 guards against parse on line 550 failing? Then why is it safe to assume parse call on line 558 will succeed?

What is the difference between LocalDateTime and default time zone?

We are checking if timeZone is null but not dateTime -- how come?

The null field will come from exprDateTimeNoTimezone which is used when only one parameter to datetime is called.

If I call SELECT DATETIME('2021-02-31 12:34:56) then exprDateTimeNoTimezone calls exprDateTime with null in the timezone field.

A better pattern would be to have function overloads: one with date and timezone, and one without timezone.
As it stands, you should include the null as an expected/handled expected @param value for timeZone.

MaxKsyunz · 2022-09-02T01:39:30Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

@@ -679,4 +780,29 @@ private ExprValue exprYear(ExprValue date) {
    return new ExprIntegerValue(date.dateValue().getYear());
  }

+  private Boolean isTimeZoneValid(ZoneId zone) {


I'd like to follow-up on this. As far as I can see, in Java, every ZoneId instance is a valid time zone.

integ-test/src/test/java/org/opensearch/sql/ppl/DateTimeImplementationIT.java

integ-test/src/test/java/org/opensearch/sql/sql/ConvertTZFunctionIT.java

integ-test/src/test/java/org/opensearch/sql/sql/DateTimeImplementationIT.java

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

Yury-Fridlyand · 2022-09-09T19:38:07Z

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

+    DateTimeFormatter formatDT = DateTimeFormatter.ofPattern("uuuu-MM-dd HH:mm:ss[xxx]")
+        .withResolverStyle(ResolverStyle.STRICT);
+
+    try {
+      LocalDateTime ldtFormatted = LocalDateTime.parse(dateTime.stringValue(), formatDT);


You can use new ExprDatetimeValue(dateTime.stringValue()) and don't worry about parser and format.
In that case you just need to update ExprDatetimeValue ctor to use ResolverStyle.STRICT - it will be good in any case.

I don't understand what the benefit of this change would be.

You can reduce a bit your code an delegate parsing to code that already exists and covered by tests.

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

…field number. Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

…est in ConvertTZTest.java. Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

MitchellGale requested review from forestmvey, Yury-Fridlyand, raymond-lum, MaxKsyunz and acarbonetto August 19, 2022 16:40

Yury-Fridlyand reviewed Aug 19, 2022

View reviewed changes

docs/user/ppl/functions/datetime.rst Outdated Show resolved Hide resolved

Yury-Fridlyand reviewed Aug 19, 2022

View reviewed changes

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java Outdated Show resolved Hide resolved

MitchellGale requested a review from Yury-Fridlyand August 23, 2022 19:08

Yury-Fridlyand reviewed Aug 23, 2022

View reviewed changes

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java Outdated Show resolved Hide resolved

Yury-Fridlyand reviewed Aug 23, 2022

View reviewed changes

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java Outdated Show resolved Hide resolved

Yury-Fridlyand reviewed Aug 23, 2022

View reviewed changes

docs/user/ppl/functions/datetime.rst Outdated Show resolved Hide resolved

Yury-Fridlyand reviewed Aug 23, 2022

View reviewed changes

docs/user/ppl/functions/datetime.rst Show resolved Hide resolved

Yury-Fridlyand reviewed Aug 23, 2022

View reviewed changes

docs/user/ppl/functions/datetime.rst Show resolved Hide resolved

forestmvey reviewed Aug 23, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated Show resolved Hide resolved

forestmvey reviewed Aug 23, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Show resolved Hide resolved

MaxKsyunz requested changes Aug 26, 2022

View reviewed changes

MitchellGale requested review from MaxKsyunz, forestmvey and Yury-Fridlyand September 1, 2022 16:56

Yury-Fridlyand reviewed Sep 2, 2022

View reviewed changes

core/src/test/java/org/opensearch/sql/expression/datetime/DateTimeFunctionTest.java Outdated Show resolved Hide resolved

MaxKsyunz requested changes Sep 2, 2022

View reviewed changes

MaxKsyunz self-requested a review September 8, 2022 00:21

Yury-Fridlyand reviewed Sep 9, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated Show resolved Hide resolved

Yury-Fridlyand reviewed Sep 9, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated Show resolved Hide resolved

Yury-Fridlyand reviewed Sep 9, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Show resolved Hide resolved

Yury-Fridlyand reviewed Sep 9, 2022

View reviewed changes

MitchellGale added 28 commits September 16, 2022 09:29

Added more information about invalid date for convert_tz

c819a6f

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Converted "from Field" and "To Field" to use "Fieldn" where n is the …

b80d010

…field number. Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Added date validation. Added test cases in IT to cover cases. Added t…

d5fe5e5

…est in ConvertTZTest.java. Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Fixed date validation function. Broke up some unit tests.

1221d26

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Fixed formatting.

7068111

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Added DateTime tests, broke up functions.

d8344b5

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Added space in DateTimeTest.java.

c6de504

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Tidied up code and tests.

ab1fab7

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Fixed local date time rst test.

0245c4f

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Removed nested try/catch exceptions.

c722b7f

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

removed exprConvertTZ function call from within try catch statement.

30a5bd6

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Reverted change from parse localdate

bff1510

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Removed extra casting around fromTz variable.

8d4f011

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Updated wording for functions.rst

6c50b1a

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Updated wording for datetime.rst to describe null for conert_tz

978ec99

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Added more test cases for functions.rst

6a1eec9

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Fixed doctests.

67ea356

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Removed extra import.

d97780c

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Renamed isValidTimeZone function to isValidMySqlTimeZoneId.

23099db

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Has ExprDatetimeValue doing work for exprConvertTZ call.

674494d

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Deleted fromTZ

9c0e719

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Moved fixed variables to top of class.

5961b22

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Added Null to support of exprDateTime.

a474966

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Added missing expr functions for makedate/time

fa4bd48

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

cleaning up after rebase merge.

c87df79

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Rebase merge conflict resolution.

5e6aba7

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Fixed missing DATETIME in SQL Parser.

fa5e74f

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

Fixed IT test.

45a6d2e

Signed-off-by: MitchellGale-BitQuill <mitchellg@bitquilltech.com>

MitchellGale force-pushed the dev-Add-convert_tz-function branch from bb409a4 to 45a6d2e Compare September 19, 2022 22:51

MitchellGale merged commit f19780b into integ-Add-convert_tz-function Sep 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding convert_tz and datetime functions to SQL and PPL #109

Adding convert_tz and datetime functions to SQL and PPL #109

MitchellGale commented Aug 19, 2022

codecov bot commented Aug 19, 2022 •

edited

Loading

Yury-Fridlyand commented Aug 19, 2022

MaxKsyunz Aug 26, 2022

MitchellGale Aug 26, 2022

MaxKsyunz Sep 2, 2022

MitchellGale Sep 2, 2022 •

edited

Loading

acarbonetto Sep 12, 2022

acarbonetto Sep 12, 2022

MitchellGale Sep 13, 2022

Yury-Fridlyand commented Sep 2, 2022

Yury-Fridlyand Sep 2, 2022

MitchellGale Sep 2, 2022

MaxKsyunz Sep 1, 2022

MaxKsyunz Sep 2, 2022

MitchellGale Sep 2, 2022

acarbonetto Sep 14, 2022

MaxKsyunz Sep 2, 2022

Yury-Fridlyand Sep 9, 2022

MitchellGale Sep 12, 2022

Yury-Fridlyand Sep 12, 2022

MitchellGale Sep 14, 2022

Adding convert_tz and datetime functions to SQL and PPL #109

Adding convert_tz and datetime functions to SQL and PPL #109

Conversation

MitchellGale commented Aug 19, 2022

Description

convert_tz

nulls

Leap year

DateTime

Issues Resolved

Check List

codecov bot commented Aug 19, 2022 • edited Loading

Codecov Report

Yury-Fridlyand commented Aug 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchellGale Sep 2, 2022 • edited Loading

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

Yury-Fridlyand commented Sep 2, 2022

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

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

Choose a reason for hiding this comment

codecov bot commented Aug 19, 2022 •

edited

Loading

MitchellGale Sep 2, 2022 •

edited

Loading