Add `TIMEDIFF` and `DATEDIFF` functions. #131

Yury-Fridlyand · 2022-10-07T03:39:37Z

Signed-off-by: Yury-Fridlyand yuryf@bitquilltech.com

Description

I referred to MySQL docs and tried to reproduce MySQL v.8.0.30 behavior as a reference.

New function: `TIMEDIFF`.

Changes

Signature

(TIME, TIME) -> TIME

Future changes (TODOs):

Accept more/different types if and after [Discussion] Do we need to support more than 24 hours in TIME? opensearch-project/sql#852
Accept strings

Test queries:

-- OpenSearch
SELECT TIMEDIFF('23:59:59', '13:00:00');
-- MySQL
SELECT TIMEDIFF('2007-12-31 23:59:59', '13:00:00');
SELECT TIMEDIFF('2007-12-31 23:59:59', '2004-01-01 13:00:00');
SELECT TIMEDIFF('2000-01-01 00:00:00', now());
SELECT TIMEDIFF(DATE('2008-12-12'), DATE('2008-11-15'));

New function: `DATEDIFF`.

Changes

Calculates the difference of date part of given values.

Fully compliant with MySQL.

Signature

(DATE/DATETIME/TIMESTAMP/TIME, DATE/DATETIME/TIMESTAMP/TIME) -> LONG

Future changes (TODOs):

Accept strings if it gives performance gain

Test queries:

SELECT DATEDIFF(TIMESTAMP('2000-01-02 00:00:00'), TIMESTAMP('2000-01-01 23:59:59'));
SELECT DATEDIFF(TIME('23:59:59'), TIMESTAMP('2004-01-01 00:00:00'));
SELECT DATEDIFF(TIME('23:59:59'), TIME('00:00:00'));
SELECT DATEDIFF(TIMESTAMP('2000-01-02 00:00:00'), DATETIME('2000-01-01 23:59:59'));

Test data

I found that first 6 rows from date0, time0, time1, datetime0 are good for testing - these columns have different data types in MySQL. In OpenSearch SQL all [date][time] columns have timestamp type, so I use CAST for clear testing.

data

mysql> show fields from Calcs where field IN ('date0', 'time0', 'time1', 'datetime0');
+-----------+-----------+------+-----+---------+-------+
| Field     | Type      | Null | Key | Default | Extra |
+-----------+-----------+------+-----+---------+-------+
| date0     | date      | YES  |     | NULL    |       |
| time0     | datetime  | YES  |     | NULL    |       |
| time1     | time      | YES  |     | NULL    |       |
| datetime0 | timestamp | YES  |     | NULL    |       |
+-----------+-----------+------+-----+---------+-------+
4 rows in set (0.00 sec)
mysql> select date0, time0, time1, datetime0 from calcs;
+------------+---------------------+----------+---------------------+
| date0      | time0               | time1    | datetime0           |
+------------+---------------------+----------+---------------------+
| 2004-04-15 | 1899-12-30 21:07:32 | 19:36:22 | 2004-07-09 10:17:35 |
| 1972-07-04 | 1900-01-01 13:48:48 | 02:05:25 | 2004-07-26 12:30:34 |
| 1975-11-12 | 1900-01-01 18:21:08 | 09:33:31 | 2004-08-02 07:59:23 |
| 2004-06-04 | 1900-01-01 18:51:48 | 22:50:16 | 2004-07-05 13:14:20 |
| 2004-06-19 | 1900-01-01 15:01:19 | NULL     | 2004-07-28 23:30:22 |
| NULL       | 1900-01-01 08:59:39 | 19:57:33 | 2004-07-22 00:30:23 |
| NULL       | 1900-01-01 07:37:48 | NULL     | 2004-07-28 06:54:50 |
| NULL       | 1900-01-01 19:45:54 | 19:48:23 | 2004-07-12 17:30:16 |
| NULL       | 1900-01-01 09:00:59 | 22:20:14 | 2004-07-04 22:49:28 |
| NULL       | 1900-01-01 20:36:00 | NULL     | 2004-07-23 21:13:37 |
| NULL       | 1900-01-01 01:31:32 | 00:05:57 | 2004-07-14 08:16:44 |
| NULL       | 1899-12-30 22:15:40 | 04:40:49 | 2004-07-25 15:22:26 |
| NULL       | 1900-01-01 13:53:46 | 04:48:07 | 2004-07-17 14:01:56 |
| NULL       | 1900-01-01 04:57:51 | NULL     | 2004-07-19 22:21:31 |
| NULL       | 1899-12-30 22:42:43 | 18:58:41 | 2004-07-31 11:57:52 |
| NULL       | 1899-12-30 22:24:08 | NULL     | 2004-07-14 07:43:00 |
| NULL       | 1900-01-01 11:58:29 | 12:33:57 | 2004-07-28 12:34:28 |
+------------+---------------------+----------+---------------------+
17 rows in set (0.00 sec)

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-10-07T03:47:25Z

Codecov Report

Merging #131 (8f97419) into integ-datetime-diff-functions (64a3794) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@                         Coverage Diff                         @@
##             integ-datetime-diff-functions     #131      +/-   ##
===================================================================
+ Coverage                            95.78%   95.81%   +0.02%     
- Complexity                            3503     3528      +25     
===================================================================
  Files                                  350      350              
  Lines                                 9310     9367      +57     
  Branches                               669      674       +5     
===================================================================
+ Hits                                  8918     8975      +57     
  Misses                                 334      334              
  Partials                                58       58

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

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

Impacted Files	Coverage Δ
...a/org/opensearch/sql/data/model/ExprDateValue.java	`100.00% <100.00%> (ø)`
...g/opensearch/sql/data/model/ExprDatetimeValue.java	`100.00% <100.00%> (ø)`
...a/org/opensearch/sql/data/model/ExprTimeValue.java	`100.00% <100.00%> (ø)`
.../opensearch/sql/data/model/ExprTimestampValue.java	`100.00% <100.00%> (ø)`
.../org/opensearch/sql/data/model/ExprValueUtils.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%> (ø)`
...pensearch/sql/expression/function/FunctionDSL.java	`100.00% <100.00%> (ø)`
...n/java/org/opensearch/sql/utils/DateTimeUtils.java	`100.00% <100.00%> (ø)`

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

MitchellGale · 2022-10-07T07:54:23Z

It can accept invalid dates.

opensearchsql> SELECT DATEDIFF(TIMESTAMP('2050-01-02 10:00:00'), TIMESTAMP('2000-01-01 23:59:59')); 
..............                                                                                                                                                                                                                                              
fetched rows / total rows = 1/1
+--------------------------------------------------------------------------------+
| DATEDIFF(TIMESTAMP('2050-01-02 10:00:00'), TIMESTAMP('2000-01-01 23:59:59'))   |
|--------------------------------------------------------------------------------|
| 18264                                                                          |
+--------------------------------------------------------------------------------+
opensearchsql> SELECT DATEDIFF(TIMESTAMP('2050-02-30 10:00:00'), TIMESTAMP('2000-01-01 23:59:59')); 
..............                                                                                                                                                                                                                                              
fetched rows / total rows = 1/1
+--------------------------------------------------------------------------------+
| DATEDIFF(TIMESTAMP('2050-02-30 10:00:00'), TIMESTAMP('2000-01-01 23:59:59'))   |
|--------------------------------------------------------------------------------|
| 18321                                                                          |
+--------------------------------------------------------------------------------+
opensearchsql> SELECT DATEDIFF(TIMESTAMP('2050-02-31 10:00:00'), TIMESTAMP('2000-01-01 23:59:59')); 
..............                                                                                                                                                                                                                                              
fetched rows / total rows = 1/1
+--------------------------------------------------------------------------------+
| DATEDIFF(TIMESTAMP('2050-02-31 10:00:00'), TIMESTAMP('2000-01-01 23:59:59'))   |
|--------------------------------------------------------------------------------|
| 18321                                                                          |
+--------------------------------------------------------------------------------+

Expected:

mysql> select datediff('2050-02-31 10:00:00', '2000-01-01 23:59:59');
+--------------------------------------------------------+
| datediff('2050-02-31 10:00:00', '2000-01-01 23:59:59') |
+--------------------------------------------------------+
|                                                   NULL |
+--------------------------------------------------------+
1 row in set, 1 warning (0.00 sec)

MitchellGale · 2022-10-07T07:56:42Z

24:00:00 should return null

opensearchsql> SELECT DATEDIFF(TIMESTAMP('2000-01-02 24:00:00'), TIMESTAMP('2000-01-01 23:59:59')); 
..............                                                                                                                                                                                                                                              
fetched rows / total rows = 1/1
+--------------------------------------------------------------------------------+
| DATEDIFF(TIMESTAMP('2000-01-02 24:00:00'), TIMESTAMP('2000-01-01 23:59:59'))   |
|--------------------------------------------------------------------------------|
| 2                                                                              |
+--------------------------------------------------------------------------------+

Expected:

mysql> select datediff('2050-02-10 24:00:00', '2000-01-01 23:59:59');
+--------------------------------------------------------+
| datediff('2050-02-10 24:00:00', '2000-01-01 23:59:59') |
+--------------------------------------------------------+
|                                                   NULL |
+--------------------------------------------------------+
1 row in set, 1 warning (0.01 sec)

** Also extends to any invalid time, it should return null

MitchellGale · 2022-10-07T08:02:30Z

When second argument is larger than the first argument the returned time is second argument

opensearchsql> select timediff('20:00:00', '22:00:00');                                                                                                                                                                                                     
fetched rows / total rows = 1/1
+------------------------------------+
| timediff('20:00:00', '22:00:00')   |
|------------------------------------|
| 22:00:00                           |
+------------------------------------+

Should give negative time:

mysql> select timediff('20:00:00', '22:00:00'); 
+----------------------------------+
| timediff('20:00:00', '22:00:00') |
+----------------------------------+
| -02:00:00                        |
+----------------------------------+

MitchellGale · 2022-10-07T08:11:54Z

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

+
+  // Function signature is:
+  // (DATE/DATETIME/TIMESTAMP/TIME, DATE/DATETIME/TIMESTAMP/TIME) -> LONG
+  private static Stream<Arguments> getTestData() {


Should add test cases covering the comments I made in the conversations tab.

When second argument is larger than the first argument the returned time is second argument

Should give negative time:

Unfortunately, there is no negative time or time > 24 hours in OpenSearch.
If you start a 22 hour timer at 22:00, it will stop at 20:00 next day. Try playing with other numbers to avoid being confused, e.g. 7 AM/PM and 10 AM/PM.

Yury-Fridlyand · 2022-10-11T17:21:09Z

24:00:00 should return null

That's a bug of TIMESTAMP function as well as accepting February 30th - it doesn't use STRICT parser. Will be fixed in scope of another PR.

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

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

core/src/main/java/org/opensearch/sql/data/model/ExprTimeValue.java

core/src/main/java/org/opensearch/sql/data/model/ExprDateValue.java

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

docs/user/dql/functions.rst

docs/user/ppl/functions/datetime.rst

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

…orresponding tests. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Co-authored-by: Max Ksyunz <maxk@bitquilltech.com>

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

MitchellGale · 2022-12-15T21:45:41Z

core/src/main/java/org/opensearch/sql/data/model/ExprTimestampValue.java

@@ -30,7 +30,7 @@ public class ExprTimestampValue extends AbstractExprValue {
  /**
   * todo. only support UTC now.
   */
-  private static final ZoneId ZONE = ZoneId.of("UTC");
+  public static final ZoneId ZONE = ZoneId.of("UTC");


Might want to make it UTC_ZONE in case other zones are made later

) * Add `TIMEDIFF` and `DATEDIFF` functions. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> Co-authored-by: Max Ksyunz <maxk@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> Co-authored-by: Max Ksyunz <maxk@bitquilltech.com>

) (opensearch-project#1234) * Add `TIMEDIFF` and `DATEDIFF` functions. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> Co-authored-by: Max Ksyunz <maxk@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> Co-authored-by: Max Ksyunz <maxk@bitquilltech.com> (cherry picked from commit 438c44d) Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand requested review from GumpacG, forestmvey, GabeFernandez310, MaxKsyunz, acarbonetto and MitchellGale October 7, 2022 03:39

MitchellGale requested changes Oct 7, 2022

View reviewed changes

Yury-Fridlyand mentioned this pull request Oct 7, 2022

SQL ingestion and ops - standup opensearch-project/sql#791

Open

Yury-Fridlyand requested a review from MitchellGale October 12, 2022 02:48

MitchellGale approved these changes Oct 12, 2022

View reviewed changes

acarbonetto approved these changes Oct 13, 2022

View reviewed changes

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

acarbonetto reviewed Oct 17, 2022

View reviewed changes

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

MaxKsyunz requested changes Oct 17, 2022

View reviewed changes

GumpacG approved these changes Oct 17, 2022

View reviewed changes

acarbonetto changed the title ~~Add TIMEDIFF and DATEDIFF functions.~~ [BLOCKED] Add TIMEDIFF and DATEDIFF functions. Oct 25, 2022

MaxKsyunz added the blocked label Nov 15, 2022

Yury-Fridlyand and others added 7 commits December 9, 2022 19:17

Add TIMEDIFF and DATEDIFF functions.

e333aa5

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Update ExprTimeValue by datetimeValue and other interfaces. Add c…

2789a3d

…orresponding tests. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Simplify function implementation.

68bce05

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Fix conversion to timestamp.

e9d387a

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Apply suggestions from code review

bea1c80

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com> Co-authored-by: Max Ksyunz <maxk@bitquilltech.com>

Address PR feedback.

110c291

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Rework to use FunctionProperties.

5468caa

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Yury-Fridlyand force-pushed the dev-datetime-diff-functions branch from 8cd8f78 to 5468caa Compare December 10, 2022 03:43

Yury-Fridlyand changed the title ~~[BLOCKED] Add TIMEDIFF and DATEDIFF functions.~~ Add TIMEDIFF and DATEDIFF functions. Dec 10, 2022

Yury-Fridlyand removed the blocked label Dec 10, 2022

Yury-Fridlyand requested review from acarbonetto, GumpacG, margarit-h, MaxKsyunz and MitchellGale December 10, 2022 03:44

Simplify impl/implWithProperties.

8f97419

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

MitchellGale reviewed Dec 15, 2022

View reviewed changes

MaxKsyunz approved these changes Dec 15, 2022

View reviewed changes

acarbonetto approved these changes Dec 19, 2022

View reviewed changes

Yury-Fridlyand merged commit b235550 into integ-datetime-diff-functions Dec 19, 2022

Yury-Fridlyand deleted the dev-datetime-diff-functions branch December 19, 2022 20:57

Yury-Fridlyand mentioned this pull request Dec 19, 2022

Add TIMEDIFF and DATEDIFF functions. (#131) opensearch-project/sql#1195

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `TIMEDIFF` and `DATEDIFF` functions. #131

Add `TIMEDIFF` and `DATEDIFF` functions. #131

Yury-Fridlyand commented Oct 7, 2022 •

edited

Loading

codecov bot commented Oct 7, 2022 •

edited

Loading

MitchellGale commented Oct 7, 2022

MitchellGale commented Oct 7, 2022

MitchellGale commented Oct 7, 2022

MitchellGale Oct 7, 2022

Yury-Fridlyand Oct 7, 2022

Yury-Fridlyand commented Oct 11, 2022

MitchellGale Dec 15, 2022

Add TIMEDIFF and DATEDIFF functions. #131

Add TIMEDIFF and DATEDIFF functions. #131

Conversation

Yury-Fridlyand commented Oct 7, 2022 • edited Loading

Description

New function: TIMEDIFF.

Signature

Future changes (TODOs):

Test queries:

New function: DATEDIFF.

Signature

Future changes (TODOs):

Test queries:

Test data

Check List

codecov bot commented Oct 7, 2022 • edited Loading

Codecov Report

MitchellGale commented Oct 7, 2022

MitchellGale commented Oct 7, 2022

MitchellGale commented Oct 7, 2022

MitchellGale Oct 7, 2022

Choose a reason for hiding this comment

Yury-Fridlyand Oct 7, 2022

Choose a reason for hiding this comment

Yury-Fridlyand commented Oct 11, 2022

MitchellGale Dec 15, 2022

Choose a reason for hiding this comment

Add `TIMEDIFF` and `DATEDIFF` functions. #131

Add `TIMEDIFF` and `DATEDIFF` functions. #131

Yury-Fridlyand commented Oct 7, 2022 •

edited

Loading

New function: `TIMEDIFF`.

New function: `DATEDIFF`.

codecov bot commented Oct 7, 2022 •

edited

Loading