Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add functions ADDTIME and SUBTIME. #132

Merged
merged 9 commits into from
Dec 19, 2022
Merged

Add functions ADDTIME and SUBTIME. #132

merged 9 commits into from
Dec 19, 2022

Conversation

Yury-Fridlyand
Copy link

@Yury-Fridlyand Yury-Fridlyand commented Oct 7, 2022 •
edited
Loading

Signed-off-by: Yury-Fridlyand [email protected]

Description

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

New functions: ADDTIME/SUBTIME.

Changes

Adds or subtracts time to/from date and returns the result.

Signature

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

Future changes (TODOs):

  1. Accept strings as MySQL does
  2. Update if and after [Discussion] Do we need to support more than 24 hours in TIME? opensearch-project/sql#852

Test queries:

SELECT ADDTIME(TIME('23:59:59'), TIMESTAMP('2008-11-15 01:00:00'));
SELECT ADDTIME(TIME('23:59:59'), DATE('2004-01-01'));
SELECT ADDTIME(DATE('2008-12-12'), DATE('2008-11-15'));
SELECT ADDTIME(TIME('23:59:59'), DATETIME('2008-11-15 01:00:00'));

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)

Fixes

https://forum.opensearch.org/t/subdate-date-sub-query-method-not-supported/9252

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
Copy link

codecov bot commented Oct 7, 2022 •
edited
Loading

Codecov Report

Merging #132 (e6843e2) into integ-datetime-addsubtime-functions (64a3794) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@                            Coverage Diff                            @@
##             integ-datetime-addsubtime-functions     #132      +/-   ##
=========================================================================
+ Coverage                                  95.78%   95.82%   +0.03%     
- Complexity                                  3503     3531      +28     
=========================================================================
  Files                                        350      350              
  Lines                                       9310     9386      +76     
  Branches                                     669      676       +7     
=========================================================================
+ Hits                                        8918     8994      +76     
  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
MitchellGale approved these changes Oct 7, 2022
View reviewed changes
Copy link

@MitchellGale MitchellGale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

docs/user/dql/functions.rst Outdated Show resolved Hide resolved
docs/user/ppl/functions/datetime.rst Outdated Show resolved Hide resolved
docs/user/ppl/functions/datetime.rst Outdated Show resolved Hide resolved
docs/user/dql/functions.rst Outdated Show resolved Hide resolved
@MitchellGale
Copy link

Will the two "todo"'s be done in a separate PR?

@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Oct 7, 2022
Open
MaxKsyunz
MaxKsyunz reviewed Oct 8, 2022
View reviewed changes
docs/user/ppl/functions/datetime.rst Outdated Show resolved Hide resolved
docs/user/ppl/functions/datetime.rst Outdated Show resolved Hide resolved
docs/user/dql/functions.rst Outdated Show resolved Hide resolved
docs/user/dql/functions.rst Outdated Show resolved Hide resolved
core/src/test/java/org/opensearch/sql/expression/datetime/AddTimeAndSubTimeTest.java
Comment on lines +114 to +116
@ParameterizedTest
@MethodSource("getTestData")
public void return_datetime_when_first_arg_is_not_time(Temporal arg1, Temporal arg2,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

core/src/test/java/org/opensearch/sql/expression/datetime/AddTimeAndSubTimeTest.java
@Test
public void time_limited_by_24_hours() {
var res = addtime(LocalTime.of(21, 0), LocalTime.of(14, 5));
assertEquals(TIME, res.type());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be skipped -- already covered in the test above.

core/src/test/java/org/opensearch/sql/expression/datetime/AddTimeAndSubTimeTest.java
assertEquals(LocalTime.of(11, 5), res.timeValue());

res = subtime(LocalTime.of(14, 0), LocalTime.of(21, 5));
assertEquals(TIME, res.type());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be skipped -- already covered in the test above.

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

@Test
// (TIME, TIME/DATE/DATETIME/TIMESTAMP) -> TIME
public void return_time_when_first_arg_is_time() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove asserts that test values -- the intent is to test return type and there's already a test for arithmetic.

core/src/test/java/org/opensearch/sql/expression/datetime/AddTimeAndSubTimeTest.java
Comment on lines +76 to +105
// DATETIME and TIME/DATE/DATETIME/TIMESTAMP
Arguments.of(LocalDateTime.of(1961, 4, 12, 9, 7), LocalTime.of(1, 48),
LocalDateTime.of(1961, 4, 12, 10, 55), LocalDateTime.of(1961, 4, 12, 7, 19)),
Arguments.of(LocalDateTime.of(1961, 4, 12, 9, 7), LocalDate.of(2000, 1, 1),
LocalDateTime.of(1961, 4, 12, 9, 7), LocalDateTime.of(1961, 4, 12, 9, 7)),
Arguments.of(LocalDateTime.of(1961, 4, 12, 9, 7), LocalDateTime.of(1235, 5, 6, 1, 48),
LocalDateTime.of(1961, 4, 12, 10, 55), LocalDateTime.of(1961, 4, 12, 7, 19)),
Arguments.of(LocalDateTime.of(1961, 4, 12, 9, 7), Instant.ofEpochSecond(42),
LocalDateTime.of(1961, 4, 12, 9, 7, 42), LocalDateTime.of(1961, 4, 12, 9, 6, 18)),
// DATE and TIME/DATE/DATETIME/TIMESTAMP
Arguments.of(LocalDate.of(1961, 4, 12), LocalTime.of(9, 7),
LocalDateTime.of(1961, 4, 12, 9, 7), LocalDateTime.of(1961, 4, 11, 14, 53)),
Arguments.of(LocalDate.of(1961, 4, 12), LocalDate.of(2000, 1, 1),
LocalDateTime.of(1961, 4, 12, 0, 0), LocalDateTime.of(1961, 4, 12, 0, 0)),
Arguments.of(LocalDate.of(1961, 4, 12), LocalDateTime.of(1235, 5, 6, 1, 48),
LocalDateTime.of(1961, 4, 12, 1, 48), LocalDateTime.of(1961, 4, 11, 22, 12)),
Arguments.of(LocalDate.of(1961, 4, 12), Instant.ofEpochSecond(42),
LocalDateTime.of(1961, 4, 12, 0, 0, 42), LocalDateTime.of(1961, 4, 11, 23, 59, 18)),
// TIMESTAMP and TIME/DATE/DATETIME/TIMESTAMP
Arguments.of(Instant.ofEpochSecond(42), LocalTime.of(9, 7),
LocalDateTime.of(1970, 1, 1, 9, 7, 42), LocalDateTime.of(1969, 12, 31, 14, 53, 42)),
Arguments.of(Instant.ofEpochSecond(42), LocalDate.of(1961, 4, 12),
LocalDateTime.of(1970, 1, 1, 0, 0, 42), LocalDateTime.of(1970, 1, 1, 0, 0, 42)),
Arguments.of(Instant.ofEpochSecond(42), LocalDateTime.of(1961, 4, 12, 9, 7),
LocalDateTime.of(1970, 1, 1, 9, 7, 42), LocalDateTime.of(1969, 12, 31, 14, 53, 42)),
Arguments.of(Instant.ofEpochSecond(42), Instant.ofEpochMilli(42),
LocalDateTime.of(1970, 1, 1, 0, 0, 42, 42000000),
LocalDateTime.of(1970, 1, 1, 0, 0, 41, 958000000))
);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be split by commented groups into independent test source methods.

@Yury-Fridlyand
Copy link
Author

Will the two "todo"'s be done in a separate PR?

Yes, these TODOs depend on other features.

GabeFernandez310
GabeFernandez310 reviewed Oct 17, 2022
View reviewed changes
core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java
* Adds expr2 to expr1 and returns the result.
* (TIME, TIME/DATE/DATETIME/TIMESTAMP) -> TIME
* (DATE/DATETIME/TIMESTAMP, TIME/DATE/DATETIME/TIMESTAMP) -> DATETIME
* TODO: MySQL has these signatures too
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to implement these other signatures in a separate PR or were they meant to be done here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In another PR

@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Oct 20, 2022
Merged
6 tasks
@acarbonetto acarbonetto changed the title Add functions ADDTIME and SUBTIME. [BLOCKED] Add functions ADDTIME and SUBTIME. Oct 25, 2022
@Yury-Fridlyand Yury-Fridlyand force-pushed the dev-datetime-addsubtime-functions branch from 0a878ba to ee5eb3e Compare December 10, 2022 04:19
@Yury-Fridlyand Yury-Fridlyand changed the title [BLOCKED] Add functions ADDTIME and SUBTIME. Add functions ADDTIME and SUBTIME. Dec 10, 2022
Yury-Fridlyand and others added 2 commits December 9, 2022 21:35
@Yury-Fridlyand
Fix doctests.
fcb361a 
Signed-off-by: Yury-Fridlyand <[email protected]>
@Yury-Fridlyand
Simplify impl/implWithProperties.
e6843e2 
Signed-off-by: Yury-Fridlyand <[email protected]>
MitchellGale
MitchellGale reviewed Dec 15, 2022
View reviewed changes
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");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps specify this is UTC_ZONE?

acarbonetto
acarbonetto approved these changes Dec 19, 2022
View reviewed changes
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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any chance you know about this TODO?

core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java
implWithProperties(nullMissingHandlingWithProperties(DateTimeFunction::exprSubTime),
TIME, TIME, DATE),
implWithProperties(nullMissingHandlingWithProperties(DateTimeFunction::exprSubTime),
TIME, TIME, DATETIME),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this case match with SQL?

core/src/main/java/org/opensearch/sql/expression/function/FunctionDSL.java
return (functionProperties, v1, v2) -> {
if (v1.isMissing() || v2.isMissing()) {
return ExprValueUtils.missingValue();
} else if (v1.isNull() || v2.isNull()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else not required

core/src/main/java/org/opensearch/sql/expression/function/FunctionDSL.java
return ExprValueUtils.missingValue();
} else if (v1.isNull() || v2.isNull()) {
return ExprValueUtils.nullValue();
} else {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else not required

@Yury-Fridlyand Yury-Fridlyand merged commit b2a46f9 into integ-datetime-addsubtime-functions Dec 19, 2022
@Yury-Fridlyand Yury-Fridlyand deleted the dev-datetime-addsubtime-functions branch December 19, 2022 20:50
@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Dec 19, 2022
Merged
6 tasks
Yury-Fridlyand added a commit that referenced this pull request Jan 9, 2023
@Yury-Fridlyand
Add functions ADDTIME and SUBTIME. (#132) (opensearch-project#1194)
7630f87 
* Add functions `ADDTIME` and `SUBTIME`. (#132)

Signed-off-by: Yury-Fridlyand <[email protected]>
matthewryanwells pushed a commit that referenced this pull request Feb 1, 2023
@opensearch-trigger-bot @Yury-Fridlyand
Add functions ADDTIME and SUBTIME. (#132) (opensearch-project#1194)…
571dcd1 
… (opensearch-project#1252)

* Add functions `ADDTIME` and `SUBTIME`. (#132)

Signed-off-by: Yury-Fridlyand <[email protected]>
(cherry picked from commit 7630f87)

Co-authored-by: Yury-Fridlyand <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MitchellGale MitchellGale MitchellGale approved these changes

@acarbonetto acarbonetto acarbonetto approved these changes

@MaxKsyunz MaxKsyunz MaxKsyunz approved these changes

@forestmvey forestmvey forestmvey approved these changes

@GabeFernandez310 GabeFernandez310 Awaiting requested review from GabeFernandez310

@GumpacG GumpacG Awaiting requested review from GumpacG

@margarit-h margarit-h Awaiting requested review from margarit-h

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Yury-Fridlyand @MitchellGale @GumpacG @forestmvey @GabeFernandez310 @MaxKsyunz @acarbonetto