Update `DATE_ADD`/`ADDDATE` and `DATE_SUB`/`SUBDATE` functions. #122

Yury-Fridlyand · 2022-09-19T23:31:13Z

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.

Rework on `DATE_ADD`/`ADDDATE` functions.

Changes

Fixes:

Function accepts TIME, considering today's date. If argument is DATE, it interpreted as midnight of this DATE.
Function returns always DATETIME.
Function can interact with any combination of argument types and date and time intervals both.

Future changes (TODOs):

Rework after [FEATURE] Add 3 interval types instead of 1 to support complex interval expressions opensearch-project/sql#859
Accept and return STRING

Removed function `DATE_ADD(date, int)`

Left function (matches MySQL):

ADDDATE(date, interval)
DATE_ADD(date, interval)
ADDDATE(date, int)

Update signature list

Added TIME
Removed STRING - all datetime types have implicit cast from a string already

More details

Function Before After

DATE_ADD

(STRING/DATE/DATETIME/TIMESTAMP, INTERVAL) -> DATETIME

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

ADDDATE

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

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

Rework on `DATE_SUB`/`SUBDATE` functions.

Changes

Removed function `DATE_SUB(date, int)`

The same fixes and notes as for `DATE_ADD`/`ADDDATE` functions.

Test queries:

NB: Due to opensearch-project#853 you can't test with DATETIME (without code changes), but I tested with it

OpenSearch

SELECT date0, DATE_ADD(CAST(date0 AS date), INTERVAL 1 HOUR) AS `date + 1h`,
       time0, DATE_ADD(CAST(time0 AS datetime), INTERVAL 1 HOUR) AS `datetime + 1h`,
	   time1, DATE_ADD(CAST(time1 AS time), INTERVAL 1 HOUR) AS `time + 1h`,
   datetime0, DATE_ADD(CAST(datetime0 AS timestamp), INTERVAL 1 HOUR) AS `timestamp + 1h` FROM calcs LIMIT 6;
   
SELECT date0, DATE_ADD(CAST(date0 AS date), INTERVAL 1 DAY) AS `date + 1d`,
       time0, DATE_ADD(CAST(time0 AS datetime), INTERVAL 1 DAY) AS `datetime + 1d`,
	   time1, DATE_ADD(CAST(time1 AS time), INTERVAL 1 DAY) AS `time + 1d`,
   datetime0, DATE_ADD(CAST(datetime0 AS timestamp), INTERVAL 1 DAY) AS `timestamp + 1d` FROM calcs LIMIT 6;
   
SELECT date0, DATE_ADD(CAST(date0 AS date), INTERVAL 1 HOUR) AS `date + 1h`, DATE_ADD(CAST(date0 AS date), INTERVAL 1 DAY) AS `date + 1d` FROM calcs LIMIT 6;
SELECT time0, DATE_ADD(CAST(time0 AS datetime), INTERVAL 1 HOUR) AS `datetime + 1h`, DATE_ADD(CAST(time0 AS datetime), INTERVAL 1 DAY) AS `datetime + 1d` FROM calcs LIMIT 6;
SELECT time1, DATE_ADD(CAST(time1 AS time), INTERVAL 24 HOUR) AS `time + 24h` FROM calcs LIMIT 6;
SELECT datetime0, DATE_ADD(CAST(datetime0 AS timestamp), INTERVAL 1 HOUR) AS `timestamp + 1h`, DATE_ADD(CAST(datetime0 AS timestamp), INTERVAL 1 DAY) AS `timestamp + 1d` FROM calcs LIMIT 6;

SELECT time1, CAST(time1 AS time), ADDDATE(CAST(time1 AS time), 1) FROM calcs LIMIT 6;
SELECT date0, CAST(date0 AS date), ADDDATE(CAST(date0 AS date), 1) FROM calcs LIMIT 6;
SELECT time0, CAST(time0 AS datetime), ADDDATE(CAST(time0 AS datetime), 1) FROM calcs LIMIT 6;
SELECT datetime0, CAST(datetime0 AS timestamp), ADDDATE(CAST(datetime0 AS timestamp), 1) FROM calcs LIMIT 6;

MySQL

SELECT date0, DATE_ADD(date0, INTERVAL 1 HOUR) AS `date + 1h`, time0, DATE_ADD(time0, INTERVAL 1 HOUR) AS `datetime + 1h`, time1, DATE_ADD(time1, INTERVAL 1 HOUR) AS `time + 1h`, datetime0, DATE_ADD(datetime0, INTERVAL 1 HOUR) AS `timestamp + 1h` FROM calcs LIMIT 6;
SELECT date0, DATE_ADD(date0, INTERVAL 1 DAY) AS `date + 1d`, time0, DATE_ADD(time0, INTERVAL 1 DAY) AS `datetime + 1d`, time1, DATE_ADD(time1, INTERVAL 1 DAY) AS `time + 1d`, datetime0, DATE_ADD(datetime0, INTERVAL 1 DAY) AS `timestamp + 1d` FROM calcs LIMIT 6;

SELECT time1, DATE_ADD(time1, INTERVAL 20 HOUR) AS `time + 20h` FROM calcs LIMIT 6;
SELECT date0, DATE_ADD(date0, INTERVAL 1 HOUR) AS `date + 1h`, DATE_ADD(date0, INTERVAL 1 DAY) AS `date + 1d` FROM calcs LIMIT 6;
SELECT time0, DATE_ADD(time0, INTERVAL 1 HOUR) AS `datetime + 1h`, DATE_ADD(time0, INTERVAL 1 DAY) AS `datetime + 1d` FROM calcs LIMIT 6;
SELECT datetime0, DATE_ADD(datetime0, INTERVAL 1 HOUR) AS `timestamp + 1h`, DATE_ADD(datetime0, INTERVAL 1 DAY) AS `timestamp + 1d` FROM calcs LIMIT 6;

SELECT time1, ADDDATE(time1, 1) FROM calcs LIMIT 6;
SELECT date0, ADDDATE(date0, 1) FROM calcs LIMIT 6;
SELECT time0, ADDDATE(time0, 1) FROM calcs LIMIT 6;
SELECT datetime0, ADDDATE(datetime0, 1) FROM calcs LIMIT 6;

PostgreSQL

SELECT time1, pg_typeof(time1), "time + 5h", pg_typeof("time + 5h"), "time + 1d", pg_typeof("time + 1d") FROM (SELECT time1, time1 + INTERVAL '5 HOURS' AS "time + 5h", time1 + INTERVAL '1 DAYS' AS "time + 1d" FROM calcs LIMIT 6) foo;

SELECT date0, pg_typeof(date0), "date + 5h", pg_typeof("date + 5h"), "date + 1d", pg_typeof("date + 1d") FROM (SELECT date0, date0 + INTERVAL '5 HOURS' AS "date + 5h", date0 + INTERVAL '1 DAYS' AS "date + 1d" FROM calcs LIMIT 6) foo;

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)

Related issue:

opensearch-project#291

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.

Yury-Fridlyand · 2022-09-29T04:02:58Z

Oops. Do I need to update docs with DATETIME examples?

MaxKsyunz · 2022-09-29T18:23:14Z

@Yury-Fridlyand could yup update integ-datetime-calculations from upstream/2.x -- commit list includes unrelated ones.

Also SQL Java CI build says there's a failing integration test.

common/src/main/java/org/opensearch/sql/common/grok/GrokUtils.java

+      "%\\{"
+          + "(?<name>"
+          + "(?<pattern>[A-z0-9]+)"
+          + "(?::(?<subname>[A-z0-9_:;,\\-\\/\\s\\.']+))?"
+          + ")"
+          + "(?:=(?<definition>"
+          + "(?:"
+          + "(?:[^{}]+|\\.+)+"
+          + ")+"
+          + ")"
+          + ")?"
+          + "\\}");


common/src/main/java/org/opensearch/sql/common/grok/GrokUtils.java

+      "%\\{"
+          + "(?<name>"
+          + "(?<pattern>[A-z0-9]+)"
+          + "(?::(?<subname>[A-z0-9_:;,\\-\\/\\s\\.']+))?"
+          + ")"
+          + "(?:=(?<definition>"
+          + "(?:"
+          + "(?:[^{}]+|\\.+)+"
+          + ")+"
+          + ")"
+          + ")?"
+          + "\\}");


common/src/main/java/org/opensearch/sql/common/grok/GrokCompiler.java

+public class GrokCompiler implements Serializable {
+
+  // We don't want \n and commented line
+  private static final Pattern patternLinePattern = Pattern.compile("^([A-z0-9_]+)\\s+(.*)$");


common/src/main/java/org/opensearch/sql/common/grok/GrokUtils.java

+      "%\\{"
+          + "(?<name>"
+          + "(?<pattern>[A-z0-9]+)"
+          + "(?::(?<subname>[A-z0-9_:;,\\-\\/\\s\\.']+))?"
+          + ")"
+          + "(?:=(?<definition>"
+          + "(?:"
+          + "(?:[^{}]+|\\.+)+"
+          + ")+"
+          + ")"
+          + ")?"
+          + "\\}");


common/src/main/java/org/opensearch/sql/common/grok/GrokUtils.java

+      "%\\{"
+          + "(?<name>"
+          + "(?<pattern>[A-z0-9]+)"
+          + "(?::(?<subname>[A-z0-9_:;,\\-\\/\\s\\.']+))?"
+          + ")"
+          + "(?:=(?<definition>"
+          + "(?:"
+          + "(?:[^{}]+|\\.+)+"
+          + ")+"
+          + ")"
+          + ")?"
+          + "\\}");


MaxKsyunz

You know there are a lot of changes when GitHub refuses to load a diff...

These changes would be easier to understand if they were done as separate PRs.
It's easier to review six small PRs vs one large one.

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

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

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

MaxKsyunz · 2022-10-06T05:55:11Z

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

+  public void testSubTime() throws IOException {
+    var result = executeQuery("SELECT"
+        + " SUBTIME(DATE('2008-12-12'), DATE('2008-11-15')) AS `'2008-12-12' - 0`,"
+        + " SUBTIME(TIME('23:59:59'), DATE('2004-01-01')) AS `'23:59:59' - 0`,"
+        + " SUBTIME(DATE('2004-01-01'), TIME('23:59:59')) AS `'2004-01-01' - '23:59:59'`,"
+        + " SUBTIME(TIME('10:20:30'), TIME('00:05:42')) AS `'10:20:30' - '00:05:42'`,"
+        + " SUBTIME(TIMESTAMP('1999-12-31 15:42:13'), DATETIME('1961-04-12 09:07:00')) AS `'15:42:13' - '09:07:00'`");
+    verifySchema(result,
+        schema("SUBTIME(DATE('2008-12-12'), DATE('2008-11-15'))", "'2008-12-12' - 0", "datetime"),
+        schema("SUBTIME(TIME('23:59:59'), DATE('2004-01-01'))", "'23:59:59' - 0", "time"),
+        schema("SUBTIME(DATE('2004-01-01'), TIME('23:59:59'))", "'2004-01-01' - '23:59:59'", "datetime"),
+        schema("SUBTIME(TIME('10:20:30'), TIME('00:05:42'))", "'10:20:30' - '00:05:42'", "time"),
+        schema("SUBTIME(TIMESTAMP('1999-12-31 15:42:13'), DATETIME('1961-04-12 09:07:00'))", "'15:42:13' - '09:07:00'", "datetime"));
+    verifyDataRows(result, rows("2008-12-12 00:00:00", "23:59:59", "2003-12-31 00:00:01", "10:14:48", "1999-12-31 06:35:13"));
+  }


It'd be a lot easier to understand and debug failures if these test cases were separated.

sql/src/main/antlr/OpenSearchSQLParser.g4

MaxKsyunz · 2022-10-06T06:13:07Z

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

-    return new ExprIntervalValue(Duration.ofDays(getIntegerValue(value)));
+    return new ExprIntervalValue(Period.ofDays(getIntegerValue(value)));


Why change from Duration to Period?

It was required by changes in a4bca4f:

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

Lines 467 to 483 in a4bca4f

private ExprValue exprAddDateInterval(ExprValue date, ExprValue expr) {

var dt = LocalDateTime.MIN;

if (date.type() == TIME) {

if (expr.intervalValue() instanceof Duration) {

return new ExprTimeValue(date.timeValue().plus(expr.intervalValue()));

} else {

dt = LocalDateTime.of(LocalDate.now(), date.timeValue());

}

} else {

dt = date.datetimeValue();

}

dt = dt.plus(expr.intervalValue());

if (date.type() == DATE && expr.intervalValue() instanceof Period) {

return new ExprDateValue(dt.toLocalDate());

}

return new ExprDatetimeValue(dt);

}

These changes were mostly reverted, because they add ambiguous function signatures:

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

Lines 109 to 110 in a4bca4f

impl(nullMissingHandling(DateTimeFunction::exprAddDateInterval), DATETIME, DATE, INTERVAL),

impl(nullMissingHandling(DateTimeFunction::exprAddDateInterval), DATE, DATE, INTERVAL),

returnType, arg1Type, arg2Type

I still want to keep this change regardless of the revert, because this makes INTERVAL more consistent:

opensearch-project-sql/core/src/main/java/org/opensearch/sql/expression/datetime/IntervalClause.java

Lines 87 to 101 in 2c1e83d

private ExprValue minute(ExprValue value) {

return new ExprIntervalValue(Duration.ofMinutes(getLongValue(value)));

}

private ExprValue hour(ExprValue value) {

return new ExprIntervalValue(Duration.ofHours(getLongValue(value)));

}

private ExprValue day(ExprValue value) {

return new ExprIntervalValue(Period.ofDays(getIntegerValue(value)));

}

private ExprValue week(ExprValue value) {

return new ExprIntervalValue(Period.ofWeeks(getIntegerValue(value)));

}

Period for Date-like intervals and Duration for Time-like intervals

forestmvey · 2022-10-06T18:01:25Z

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

+            "source=%s | eval f = adddate(TIME('07:40:00'), interval 1 day) | fields f", TEST_INDEX_DATE));
+    verifySchema(result,
+        schema("f", null, "datetime"));
+    verifySome(result.getJSONArray("datarows"),


If you use the HEAD command, could you validate with verifyDataRows?

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

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

acarbonetto · 2022-10-07T15:40:49Z

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

-   * (DATE, LONG) -> DATE
-   * (STRING/DATETIME/TIMESTAMP, LONG) -> DATETIME
+   * (DATE/DATETIME/TIMESTAMP/TIME, INTERVAL) -> DATETIME
+   * TODO: MySQL has these signatures too


is this a TODO? or just a comment?

There is a room for future changes to completely align with MySQL; they depends on other features which are not implemented yet.

If possible, don't write TODO then.
That will often get caught in checkstyle rules and cause an automatic failure.

acarbonetto · 2022-10-07T15:41:21Z

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

-   * (DATE, LONG) -> DATE
-   * (STRING/DATETIME/TIMESTAMP, LONG) -> DATETIME
+   * (DATE/DATETIME/TIMESTAMP/TIME, INTERVAL) -> DATETIME
+   * TODO: MySQL has these signatures too


is this a TODO, or just a comment?

https://github.com/Bit-Quill/opensearch-project-sql/pull/122/files#r990305163

Ok - just avoid the word TODO since it will catch as a checkstyle warning in some sets

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

docs/user/dql/functions.rst

docs/user/ppl/functions/datetime.rst

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

acarbonetto · 2022-10-14T18:30:58Z

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

-   * (DATE, LONG) -> DATE
-   * (STRING/DATETIME/TIMESTAMP, LONG) -> DATETIME
+   * (DATE/DATETIME/TIMESTAMP/TIME, INTERVAL) -> DATETIME
+   * TODO: MySQL has these signatures too


If possible, don't write TODO then.
That will often get caught in checkstyle rules and cause an automatic failure.

acarbonetto · 2022-10-14T18:31:44Z

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

-   * (DATE, LONG) -> DATE
-   * (STRING/DATETIME/TIMESTAMP, LONG) -> DATETIME
+   * (DATE/DATETIME/TIMESTAMP/TIME, INTERVAL) -> DATETIME
+   * TODO: MySQL has these signatures too


Ok - just avoid the word TODO since it will catch as a checkstyle warning in some sets

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

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

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

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

MaxKsyunz · 2022-10-17T18:45:01Z

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

@@ -51,6 +56,21 @@ public LocalTime timeValue() {
    return time;
  }

+  @Override
+  public LocalDate dateValue() {
+    return LocalDate.now();


dateValue() needs to return the same any time it's called when evaluating a query. Using LocalDate.now() means that queries like SELECT CAST(TIME(12, 4) AS DATE), CAST(TIME(12, 4) AS DATE) will produce different results depending on when they are called.

As we discussed, that will be a separate feature.

Fixed in b19ea96 after rebase. Include FunctionProperties feature from opensearch-project#1047.

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

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

MaxKsyunz · 2022-10-17T18:52:44Z

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

+
+  @Test
+  public void adddate_returns_date_when_args_are_date_and_days() {
+    var res = adddate(LocalDate.of(1961, 4, 12), 100500);


what's 100500?

100500 days. Why not?

It's not clear that it's an arbitrary number. Since it's repeated several times here, I started to wonder if it's in any way meaningful.

MaxKsyunz · 2022-10-17T18:58:23Z

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

+    var res = subdate(LocalTime.of(21, 0), Duration.ofHours(1).plusMinutes(2));
+    assertEquals(DATETIME, res.type());
+    assertEquals(LocalTime.of(19, 58).atDate(LocalDate.now()), res.datetimeValue());


Will fail if lines 27 and 29 are executed on different sides of midnight.

Line 29 needs to use the "current date value for query".

As we discussed, that will be a separate feature.

Fixed in b19ea96 after rebase. Include FunctionProperties feature from opensearch-project#1047.

MaxKsyunz · 2022-10-17T18:58:58Z

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

+    var res = date_sub(LocalTime.of(10, 20, 30),
+        Duration.ofHours(1).plusMinutes(2).plusSeconds(42));
+    assertEquals(DATETIME, res.type());
+    assertEquals(LocalTime.of(9, 17, 48).atDate(LocalDate.now()), res.datetimeValue());


WIll fail if test is executed around midnight.

Needs to use "current date value for this query"

As we discussed, that will be a separate feature.

Fixed in b19ea96 after rebase. Include FunctionProperties feature from opensearch-project#1047.

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

Yury-Fridlyand · 2022-12-08T02:34:47Z

Rebased and updated to include FunctionProperties feature from opensearch-project#1047.
Please, re-review.

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

GumpacG · 2022-12-10T03:52:02Z

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

+   * (STRING, INTERVAL) -> STRING
+   * (DATE, INTERVAL) -> DATE                                     // when interval has no time part
+   * (TIME, INTERVAL) -> TIME                                     // when interval has no date part
+   * (STRING, INTERVAL) -> STRING         // when argument has date or datetime string,


Duplicate of line 226?

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

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

core/src/main/java/org/opensearch/sql/expression/function/FunctionDSL.java

MaxKsyunz · 2022-12-13T01:03:13Z

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

+
+  @Test
+  public void adddate_returns_date_when_args_are_date_and_days() {
+    var res = adddate(LocalDate.of(1961, 4, 12), 100500);


It's not clear that it's an arbitrary number. Since it's repeated several times here, I started to wonder if it's in any way meaningful.

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

GabeFernandez310 · 2022-12-15T19:01:08Z

core/src/main/java/org/opensearch/sql/expression/DSL.java

@@ -290,10 +290,6 @@ public static FunctionExpression multiply(Expression... expressions) {
    return compile(FunctionProperties.None, BuiltinFunctionName.MULTIPLY, expressions);
  }

-  public static FunctionExpression adddate(Expression... expressions) {


What was the reason for getting rid of all of these functions? Were they simply not used?

They were used in tests only

I rewrote tests and made new DSL definitions for those functions in DateTimeTestBase

These functions require a FunctionProperties object as of now, they crash on FunctionProperties.None

MitchellGale · 2022-12-15T21:50:02Z

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");


Maybe specify that ZONE is UTC_ZONE?

* Rework on `DATE_ADD`/`ADDDATE` and `DATE_SUB`/`SUBDATE` functions. Signed-off-by: Yury-Fridlyand <[email protected]>

opensearch-project#1182) * Update `DATE_ADD`/`ADDDATE` and `DATE_SUB`/`SUBDATE` functions. (#122) Signed-off-by: Yury-Fridlyand <[email protected]>

opensearch-project#1182) (opensearch-project#1325) * Update `DATE_ADD`/`ADDDATE` and `DATE_SUB`/`SUBDATE` functions. (#122) Signed-off-by: Yury-Fridlyand <[email protected]> (cherry picked from commit af188a3) Co-authored-by: Yury-Fridlyand <[email protected]>

This comment was marked as spam.

Sign in to view

Yury-Fridlyand marked this pull request as ready for review September 28, 2022 02:51

Yury-Fridlyand requested review from acarbonetto, forestmvey, GabeFernandez310, MaxKsyunz and MitchellGale September 28, 2022 02:51

Yury-Fridlyand changed the title ~~Update date/time calculation functions~~ Add and update date/time calculation functions Sep 29, 2022

Yury-Fridlyand force-pushed the dev-datetime-calculations branch from d1dfe33 to 315bc23 Compare September 29, 2022 03:52

Yury-Fridlyand force-pushed the dev-datetime-calculations branch from 315bc23 to c690f52 Compare October 3, 2022 17:48

github-advanced-security bot found potential problems Oct 3, 2022

View reviewed changes

Yury-Fridlyand force-pushed the dev-datetime-calculations branch from c690f52 to 82f47ac Compare October 3, 2022 18:03

Yury-Fridlyand mentioned this pull request Oct 5, 2022

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

Open

MaxKsyunz reviewed Oct 6, 2022

View reviewed changes

forestmvey reviewed Oct 6, 2022

View reviewed changes

Yury-Fridlyand mentioned this pull request Oct 7, 2022

Extend comparison methods to accept different datetime types. #129

Merged

6 tasks

Yury-Fridlyand force-pushed the dev-datetime-calculations branch from 82f47ac to 2c1e83d Compare October 7, 2022 04:05

Yury-Fridlyand changed the title ~~Add and update date/time calculation functions~~ Update DATE_ADD/ADDDATE and DATE_SUB/SUBDATE functions. Oct 7, 2022

Yury-Fridlyand requested review from MaxKsyunz, forestmvey and GumpacG October 7, 2022 04:33

acarbonetto requested changes Oct 7, 2022

View reviewed changes

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

acarbonetto requested changes Oct 17, 2022

View reviewed changes

acarbonetto approved these changes Oct 17, 2022

View reviewed changes

GumpacG approved these changes Oct 17, 2022

View reviewed changes

MaxKsyunz reviewed Oct 17, 2022

View reviewed changes

Yury-Fridlyand requested a review from MaxKsyunz October 18, 2022 02:36

Yury-Fridlyand added 3 commits December 7, 2022 15:29

Fix conversion to timestamp.

9845ad3

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

Address PR feedback.

1b33a1c

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

Update implementation and tests to use FunctionProperties.

b19ea96

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

Yury-Fridlyand force-pushed the dev-datetime-calculations branch from a64d6f9 to b19ea96 Compare December 8, 2022 02:32

Yury-Fridlyand requested review from MitchellGale, GumpacG and margarit-h December 8, 2022 02:35

Yury-Fridlyand removed the blocked label Dec 8, 2022

Yury-Fridlyand changed the title ~~[BLOCKED] Update DATE_ADD/ADDDATE and DATE_SUB/SUBDATE functions.~~ Update DATE_ADD/ADDDATE and DATE_SUB/SUBDATE functions. Dec 8, 2022

Minor improvements.

734a822

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

GumpacG reviewed Dec 10, 2022

View reviewed changes

GumpacG approved these changes Dec 10, 2022

View reviewed changes

Minor improvements.

02576bc

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

MaxKsyunz reviewed Dec 13, 2022

View reviewed changes

Yury-Fridlyand added 2 commits December 12, 2022 17:54

Address PR feedback.

c206b9d

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

Address PR feedback.

8a452c9

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

Yury-Fridlyand mentioned this pull request Dec 15, 2022

JDBC driver ADDDATE and SUBDATE issue[BUG] opensearch-project/sql#291

Closed

MaxKsyunz approved these changes Dec 15, 2022

View reviewed changes

margarit-h approved these changes Dec 15, 2022

View reviewed changes

GabeFernandez310 reviewed Dec 15, 2022

View reviewed changes

forestmvey approved these changes Dec 15, 2022

View reviewed changes

MitchellGale reviewed Dec 15, 2022

View reviewed changes

MitchellGale approved these changes Dec 15, 2022

View reviewed changes

Yury-Fridlyand merged commit 679bf19 into integ-datetime-calculations Dec 16, 2022

Yury-Fridlyand deleted the dev-datetime-calculations branch December 16, 2022 02:46

Yury-Fridlyand mentioned this pull request Dec 16, 2022

Update DATE_ADD/ADDDATE and DATE_SUB/SUBDATE functions. (#122) opensearch-project/sql#1182

Merged

6 tasks

Yury-Fridlyand added a commit that referenced this pull request Jan 5, 2023

Update DATE_ADD/ADDDATE and DATE_SUB/SUBDATE functions. (#122)

99c3708

* Rework on `DATE_ADD`/`ADDDATE` and `DATE_SUB`/`SUBDATE` functions. Signed-off-by: Yury-Fridlyand <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `DATE_ADD`/`ADDDATE` and `DATE_SUB`/`SUBDATE` functions. #122

Update `DATE_ADD`/`ADDDATE` and `DATE_SUB`/`SUBDATE` functions. #122

Yury-Fridlyand commented Sep 19, 2022 •

edited

Loading

This comment was marked as spam.

Yury-Fridlyand commented Sep 29, 2022

MaxKsyunz commented Sep 29, 2022 •

edited

Loading

MaxKsyunz left a comment

MaxKsyunz Oct 6, 2022

MaxKsyunz Oct 6, 2022

Yury-Fridlyand Oct 7, 2022 •

edited

Loading

forestmvey Oct 6, 2022

acarbonetto Oct 7, 2022

Yury-Fridlyand Oct 7, 2022

acarbonetto Oct 14, 2022

acarbonetto Oct 7, 2022

Yury-Fridlyand Oct 12, 2022

acarbonetto Oct 14, 2022

acarbonetto Oct 14, 2022

acarbonetto Oct 14, 2022

MaxKsyunz Oct 17, 2022

Yury-Fridlyand Oct 18, 2022

Yury-Fridlyand Dec 8, 2022

MaxKsyunz Oct 17, 2022

Yury-Fridlyand Oct 17, 2022

MaxKsyunz Dec 13, 2022

MaxKsyunz Oct 17, 2022

Yury-Fridlyand Oct 17, 2022

Yury-Fridlyand Dec 8, 2022

MaxKsyunz Oct 17, 2022

Yury-Fridlyand Oct 17, 2022

Yury-Fridlyand Dec 8, 2022

Yury-Fridlyand commented Dec 8, 2022

GumpacG Dec 10, 2022

MaxKsyunz Dec 13, 2022

GabeFernandez310 Dec 15, 2022

Yury-Fridlyand Dec 15, 2022

MitchellGale Dec 15, 2022

		return new ExprIntervalValue(Duration.ofDays(getIntegerValue(value)));
		return new ExprIntervalValue(Period.ofDays(getIntegerValue(value)));

	private ExprValue exprAddDateInterval(ExprValue date, ExprValue expr) {
	var dt = LocalDateTime.MIN;
	if (date.type() == TIME) {
	if (expr.intervalValue() instanceof Duration) {
	return new ExprTimeValue(date.timeValue().plus(expr.intervalValue()));
	} else {
	dt = LocalDateTime.of(LocalDate.now(), date.timeValue());
	}
	} else {
	dt = date.datetimeValue();
	}
	dt = dt.plus(expr.intervalValue());
	if (date.type() == DATE && expr.intervalValue() instanceof Period) {
	return new ExprDateValue(dt.toLocalDate());
	}
	return new ExprDatetimeValue(dt);
	}

	impl(nullMissingHandling(DateTimeFunction::exprAddDateInterval), DATETIME, DATE, INTERVAL),
	impl(nullMissingHandling(DateTimeFunction::exprAddDateInterval), DATE, DATE, INTERVAL),

	private ExprValue minute(ExprValue value) {
	return new ExprIntervalValue(Duration.ofMinutes(getLongValue(value)));
	}

	private ExprValue hour(ExprValue value) {
	return new ExprIntervalValue(Duration.ofHours(getLongValue(value)));
	}

	private ExprValue day(ExprValue value) {
	return new ExprIntervalValue(Period.ofDays(getIntegerValue(value)));
	}

	private ExprValue week(ExprValue value) {
	return new ExprIntervalValue(Period.ofWeeks(getIntegerValue(value)));
	}

Update DATE_ADD/ADDDATE and DATE_SUB/SUBDATE functions. #122

Update DATE_ADD/ADDDATE and DATE_SUB/SUBDATE functions. #122

Conversation

Yury-Fridlyand commented Sep 19, 2022 • edited Loading

Description

Rework on DATE_ADD/ADDDATE functions.

Fixes:

Future changes (TODOs):

Removed function DATE_ADD(date, int)

Update signature list

Rework on DATE_SUB/SUBDATE functions.

Removed function DATE_SUB(date, int)

The same fixes and notes as for DATE_ADD/ADDDATE functions.

Test queries:

Test data

Related issue:

Check List

This comment was marked as spam.

Yury-Fridlyand commented Sep 29, 2022

MaxKsyunz commented Sep 29, 2022 • edited Loading

MaxKsyunz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand Oct 7, 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

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

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

Yury-Fridlyand commented Dec 8, 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

Update `DATE_ADD`/`ADDDATE` and `DATE_SUB`/`SUBDATE` functions. #122

Update `DATE_ADD`/`ADDDATE` and `DATE_SUB`/`SUBDATE` functions. #122

Yury-Fridlyand commented Sep 19, 2022 •

edited

Loading

Rework on `DATE_ADD`/`ADDDATE` functions.

Removed function `DATE_ADD(date, int)`

Rework on `DATE_SUB`/`SUBDATE` functions.

Removed function `DATE_SUB(date, int)`

The same fixes and notes as for `DATE_ADD`/`ADDDATE` functions.

MaxKsyunz commented Sep 29, 2022 •

edited

Loading

Yury-Fridlyand Oct 7, 2022 •

edited

Loading