Extend comparison methods to accept different datetime types. (#129)

[Yury-Fridlyand]

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

Description

Functions affected: =, !=, >, <, <= >=.

Added automatic cast rules:

TIME -> DATETIME
DATE -> DATETIME
DATETIME -> TIMESTAMP

This allows to compare values of any of datetime types.

Reverted

Signatures added:

TIME, DATE               -> BOOLEAN
TIME, DATETIME           -> BOOLEAN
TIME, TIMESTAMP          -> BOOLEAN
DATE, TIME               -> BOOLEAN
DATE, DATETIME           -> BOOLEAN
DATE, TIMESTAMP          -> BOOLEAN
DATETIME, TIME           -> BOOLEAN
DATETIME, DATE           -> BOOLEAN
DATETIME, TIMESTAMP      -> BOOLEAN
TIMESTAMP, TIME          -> BOOLEAN
TIMESTAMP, DATE          -> BOOLEAN
TIMESTAMP, DATETIME      -> BOOLEAN

See team review and discussion in Bit-Quill#129.

Test 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    |       |
+-----------+-----------+------+-----+---------+-------+
mysql> select `key`, date0, time0, time1, datetime0 from calcs limit 7;
+-------+------------+---------------------+----------+---------------------+
| key   | date0      | time0               | time1    | datetime0           |
+-------+------------+---------------------+----------+---------------------+
| key00 | 2004-04-15 | 1899-12-30 21:07:32 | 19:36:22 | 2004-07-09 10:17:35 |
| key01 | 1972-07-04 | 1900-01-01 13:48:48 | 02:05:25 | 2004-07-26 12:30:34 |
| key02 | 1975-11-12 | 1900-01-01 18:21:08 | 09:33:31 | 2004-08-02 07:59:23 |
| key03 | 2004-06-04 | 1900-01-01 18:51:48 | 22:50:16 | 2004-07-05 13:14:20 |
| key04 | 2004-06-19 | 1900-01-01 15:01:19 | NULL     | 2004-07-28 23:30:22 |
| key05 | NULL       | 1900-01-01 08:59:39 | 19:57:33 | 2004-07-22 00:30:23 |
| key06 | NULL       | 1900-01-01 07:37:48 | NULL     | 2004-07-28 06:54:50 |
+-------+------------+---------------------+----------+---------------------+

Test queries

MySQL

select `key`, date0, time0, time1, datetime0, date0 < now(), time0 < now(), time1 < now(), datetime0 < now() from calcs limit 6;
select `key`, date0, time0, time1, datetime0, date0 <= now(), time0 <= now(), time1 <= now(), datetime0 <= now() from calcs limit 6;

OpenSearch

SELECT CAST(date0 AS date)                  AS `date`,       CAST(date0 AS date)             < now() AS `date < datetime`,
       datetime(CAST(time0 AS STRING))      AS `datetime`,   datetime(CAST(time0 AS STRING)) < now() AS `datetime < datetime`,
       CAST(time1 AS time)                  AS `time`,       CAST(time1 AS time)             < now() AS `time < datetime`,
       CAST(datetime0 AS timestamp)         AS `timestamp`,  CAST(datetime0 AS timestamp)    < now() AS `timestamp < datetime`
FROM calcs LIMIT 6;
   
SELECT CAST(date0 AS date)                  AS `date`,       CAST(date0 AS date)             <= now() AS `date <= datetime`,
       datetime(CAST(time0 AS STRING))      AS `datetime`,   datetime(CAST(time0 AS STRING)) <= now() AS `datetime <= datetime`,
       CAST(time1 AS time)                  AS `time`,       CAST(time1 AS time)             <= now() AS `time <= datetime`,
       CAST(datetime0 AS timestamp)         AS `timestamp`,  CAST(datetime0 AS timestamp)    <= now() AS `timestamp <= datetime`
FROM calcs LIMIT 6;

Issues Resolved

Fix comparison between different datetime data types. #294

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-commenter]

Codecov Report

Merging #1196 (50fbdc0) into main (1108379) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1196      +/-   ##
============================================
- Coverage     98.35%   98.35%   -0.01%     
- Complexity     3604     3617      +13     
============================================
  Files           344      343       -1     
  Lines          8933     8927       -6     
  Branches        562      569       +7     
============================================
- Hits           8786     8780       -6     
  Misses          142      142              
  Partials          5        5              

Flag	Coverage Δ
sql-engine	98.35% <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/AbstractExprValue.java	100.00% <100.00%> (ø)
...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%> (ø)
.../java/org/opensearch/sql/data/model/ExprValue.java	100.00% <100.00%> (ø)
...ava/org/opensearch/sql/data/type/ExprCoreType.java	100.00% <100.00%> (ø)
...c/main/java/org/opensearch/sql/expression/DSL.java	100.00% <100.00%> (ø)
.../expression/operator/convert/TypeCastOperator.java	100.00% <100.00%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[dai-chen]

Sorry to point this out late. I just realized our current implicit cast mechanism may handle this more properly. Its basic idea is to wrap cast function on argument which has different type than resolved argument but compatible. In this way, all functions will cast the argument automatically instead of only comparison operator. Here is the design doc: https://github.com/opensearch-project/sql/blob/main/docs/dev/TypeConversion.md

Code changes in 2 places for test:

1. Make DATE and TIME compatible with TIMESTAMP for our test
2. Add missing cast_to_timestamp(date) function impl

diff --git a/core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java b/core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java
index ad2e4259..58a60779 100644
--- a/core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java
+++ b/core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java
@@ -55,10 +55,10 @@ public enum ExprCoreType implements ExprType {
    * Date.
    * Todo. compatible relationship.
    */
-  TIMESTAMP(STRING),
   DATE(STRING),
   TIME(STRING),
   DATETIME(STRING),
+  TIMESTAMP(DATE, TIME),
   INTERVAL(UNDEFINED),

   /**
diff --git a/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperator.java b/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperator.java
index 8f904bfb..637905a5 100644
--- a/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperator.java
+++ b/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperator.java
@@ -182,6 +182,8 @@ public class TypeCastOperator {
             (v) -> new ExprTimestampValue(v.stringValue())), TIMESTAMP, STRING),
         impl(nullMissingHandling(
             (v) -> new ExprTimestampValue(v.timestampValue())), TIMESTAMP, DATETIME),
+        impl(nullMissingHandling(
+            (v) -> new ExprTimestampValue(v.timestampValue())), TIMESTAMP, DATE),
         impl(nullMissingHandling((v) -> v), TIMESTAMP, TIMESTAMP)
     );
   }

Here is my test: (Note that the reason it return true is: ExprDateValue.timestampValue() seems consider timezone and return 2020-09-16 07:00:00 for date value 2020-09-16. I don't see other issue than this. Please let me know if this can work for you. Thanks!

curl -XPOST "localhost:9200/_plugins/_sql" -H 'Content-Type: application/json' -d'
{
  "query": "SELECT DATE('\''2020-09-16'\'') > TIMESTAMP('\''2020-09-16 00:00:00'\'') "
}'
{
  "schema": [
    {
      "name": "DATE('2020-09-16') > TIMESTAMP('2020-09-16 00:00:00')",
      "type": "boolean"
    }
  ],
  "datarows": [
    [
      true
    ]
  ],
  "total": 1,
  "size": 1,
  "status": 200
}

[Yury-Fridlyand]

@dai-chen, thank you for detailed feedback!

I'll try to rework it according to you notes.
So far I see those things:

1. TypeCastOperator should be extended to support TIME -> TIMESTAMP cast which requires FunctionProperties
2. Same casts should be added for X -> DATETIME (optional, because X -> TIMESTAMP -> DATETIME is possible)
3. LuceneQuery should be updated to reflect changes in TypeCastOperator
4. ExprCoreType should be like this:

  DATE(STRING),
  TIME(STRING),
  DATETIME(STRING, DATE, TIME),
  TIMESTAMP(STRING, DATE, TIME),
  INTERVAL(UNDEFINED),

[Yury-Fridlyand]

@dai-chen,
I tried to apply your proposal and did a performance measurements. It has a bit worse performance, unfortunately.
Also it is impossible to update castMap in LuceneQuery - TIME -> X cast requires FunctionProperties, but castMap isn't compatible with it.
Please, see info about performance testing below:
First performance measurement point

diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchQueryManager.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearc>
index 9c6fcdb82..0b2629562 100644
--- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchQueryManager.java
+++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchQueryManager.java
@@ -30,7 +30,12 @@ public class OpenSearchQueryManager implements QueryManager {

   @Override
   public QueryId submit(AbstractPlan queryPlan) {
-    schedule(nodeClient, () -> queryPlan.execute());
+    schedule(nodeClient, () -> {
+      long timeBefore = System.currentTimeMillis();
+      queryPlan.execute();
+      long elapsed = System.currentTimeMillis() - timeBefore;
+      System.out.println(String.format("\u001B[31m===== %d =====\u001B[0m", elapsed));
+    });

     return queryPlan.getQueryId();
   }

Second:

   diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSe>
index 9a136a3be..98bffb85c 100644
--- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java
+++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java
@@ -41,7 +41,7 @@ public class OpenSearchExecutionEngine implements ExecutionEngine {
         () -> {
           try {
             List<ExprValue> result = new ArrayList<>();
-
+            long timeBefore = System.currentTimeMillis();
             context.getSplit().ifPresent(plan::add);
             plan.open();

@@ -50,6 +50,8 @@ public class OpenSearchExecutionEngine implements ExecutionEngine {
             }

             QueryResponse response = new QueryResponse(physicalPlan.schema(), result);
+            long elapsed = System.currentTimeMillis() - timeBefore;
+            System.out.println(String.format("\u001B[31m===== %d =====\u001B[0m", elapsed));
             listener.onResponse(response);
           } catch (Exception e) {
             listener.onFailure(e);

Changes I applied:

diff --git a/core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java b/core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java
index ad2e42596..fc964d205 100644
--- a/core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java
+++ b/core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java
@@ -55,10 +55,16 @@ public enum ExprCoreType implements ExprType {
    * Date.
    * Todo. compatible relationship.
    */
-  TIMESTAMP(STRING),
   DATE(STRING),
   TIME(STRING),
+/*
   DATETIME(STRING),
+  TIMESTAMP(STRING),
+//*/
+//*
+  DATETIME(STRING, DATE, TIME),
+  TIMESTAMP(STRING, DATE, TIME, DATETIME),
+//*/
   INTERVAL(UNDEFINED),

   /**
diff --git a/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperator.java b/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOpe>
index 8f904bfbf..46e178aed 100644
--- a/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperator.java
+++ b/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperator.java
@@ -19,7 +19,9 @@ import static org.opensearch.sql.data.type.ExprCoreType.STRING;
 import static org.opensearch.sql.data.type.ExprCoreType.TIME;
 import static org.opensearch.sql.data.type.ExprCoreType.TIMESTAMP;
 import static org.opensearch.sql.expression.function.FunctionDSL.impl;
+import static org.opensearch.sql.expression.function.FunctionDSL.implWithProperties;
 import static org.opensearch.sql.expression.function.FunctionDSL.nullMissingHandling;
+import static org.opensearch.sql.expression.function.FunctionDSL.nullMissingHandlingWithProperties;

 import java.util.Arrays;
  import java.util.stream.Collectors;
@@ -182,6 +184,12 @@ public class TypeCastOperator {
             (v) -> new ExprTimestampValue(v.stringValue())), TIMESTAMP, STRING),
         impl(nullMissingHandling(
             (v) -> new ExprTimestampValue(v.timestampValue())), TIMESTAMP, DATETIME),
+//*
+        impl(nullMissingHandling(
+            (v) -> new ExprTimestampValue(v.timestampValue())), TIMESTAMP, DATE),
+        implWithProperties(nullMissingHandlingWithProperties(
+            (fp, v) -> new ExprTimestampValue(((ExprTimeValue)v).timestampValue(fp))), TIMESTAMP, TIME),
+//*/
         impl(nullMissingHandling((v) -> v), TIMESTAMP, TIMESTAMP)
     );
   }
@@ -192,6 +200,11 @@ public class TypeCastOperator {
             (v) -> new ExprDatetimeValue(v.stringValue())), DATETIME, STRING),
         impl(nullMissingHandling(
             (v) -> new ExprDatetimeValue(v.datetimeValue())), DATETIME, TIMESTAMP),
+//*
+        implWithProperties(nullMissingHandlingWithProperties(
+            (fp, v) -> new ExprDatetimeValue(((ExprTimeValue)v).datetimeValue(fp))), DATETIME, TIME),
+        impl(nullMissingHandling((v) -> v), DATETIME, DATETIME),
+//*/
         impl(nullMissingHandling(
             (v) -> new ExprDatetimeValue(v.datetimeValue())), DATETIME, DATE)
     );
diff --git a/core/src/main/java/org/opensearch/sql/expression/operator/predicate/BinaryPredicateOperator.java b/core/src/main/java/org/opensearch/sql/expression/operator/predicate/>
index 23547e8d4..5baaa357b 100644
--- a/core/src/main/java/org/opensearch/sql/expression/operator/predicate/BinaryPredicateOperator.java
+++ b/core/src/main/java/org/opensearch/sql/expression/operator/predicate/BinaryPredicateOperator.java
@@ -202,6 +202,13 @@ public class BinaryPredicateOperator {
   private static DefaultFunctionResolver compareImpl(
       FunctionName function, SerializableBiFunction<Comparable, Comparable, Boolean> comparator) {
     return FunctionDSL.define(function,
+//*
+            ExprCoreType.coreTypes().stream()
+                .map(type -> impl(nullMissingHandling(
+                    (v1, v2) -> ExprBooleanValue.of(comparator.apply(v1, v2))),
+                        BOOLEAN, type, type))
+//*/
+/*
         Stream.concat(
             ExprCoreType.coreTypes().stream()
                 .map(type -> impl(nullMissingHandling(
@@ -212,6 +219,7 @@ public class BinaryPredicateOperator {
                     (fp, v1, v2) -> ExprBooleanValue.of(comparator.apply(
                             extractDateTime(v1, fp), extractDateTime(v2, fp)))),
                         BOOLEAN, pair.getLeft(), pair.getRight())))
+//*/
         .collect(Collectors.toList()));
   }

To test I swapped comments by switching //* to /* and run different queries. Each time I restart the cluster before making a test.

SELECT CAST(date0 AS date)                  AS `date`,       CAST(date0 AS date)             < now() AS `date < datetime`,
       datetime(CAST(time0 AS STRING))      AS `datetime`,   datetime(CAST(time0 AS STRING)) < now() AS `datetime < datetime`,
       CAST(time1 AS time)                  AS `time`,       CAST(time1 AS time)             < now() AS `time < datetime`,
       CAST(datetime0 AS timestamp)         AS `timestamp`,  CAST(datetime0 AS timestamp)    < now() AS `timestamp < datetime`
FROM calcs LIMIT 6;

(there is only one time measurement point)

chen: 435, 17, 14, 13, 14
yury: 532, 16, 15, 14, 14

---

select count(*), timestamp > '2018-01-21 05:32:25' from flights group by 2;

yury: 362, 88, 52, 82, 41
      765, 96, 57, 87, 45
chen: 342, 97, 65, 92, 47
      743, 103, 70, 97, 51

---

select count(*), timestamp > TIME '05:32:25' from flights group by 2;

yury: 288, 68, 38, 27, 28
      728, 75, 43, 32, 33
chen: 294, 47, 47, 35, 30
      831, 55, 52, 40, 34

---

select count(*), timestamp > DATE '2018-01-21' from flights group by 2;

yury: 268, 41, 37, 23, 23
      656, 48, 41, 28, 27
chen: 259, 58, 28, 30, 26
      658, 6
5, 32, 34, 30

---

select count(*), datetime0 > '2004-07-14 08:16:44' from calcs group by 2;

yury: 175, 15, 9, 10, 7
      576, 21, 14, 14, 11
chen: 152, 12, 9, 9, 8
      541, 19, 15, 13, 12

---

select count(*), datetime0 > TIME '08:16:44' from calcs group by 2;

yury: 187, 11, 8, 7, 6
      585, 19, 13, 12, 9
chen: 139, 12, 8, 7, 7
      470, 18, 12, 11, 11

---

select count(*), datetime0 > DATE '2004-07-14' from calcs group by 2;

yury: 149, 12, 7, 7, 8
      511, 19, 11, 10, 11
chen: 170, 18, 7, 7, 8
      642, 25, 12, 12, 12

[dai-chen]

@Yury-Fridlyand Thanks for sharing the test result! I'm not sure if the dominant factor is nested lambda evaluation on argument or resolving cast function signature per query. Because for now I think we don't have performance requirement and the existing mechanism can simplify our code significantly, we may be able to add cache mechanism in function repository or other optimization as needed. I'm looking into the code from my side and will update if any finding. Thanks!

[dai-chen]

@Yury-Fridlyand I just did some test by JMH which can microbenchmark on fine-grain class level. Here is my branch and test result: https://github.com/dai-chen/sql-1/tree/test-mircro-benchmark. I think the result is acceptable and it would be very close if we ignore time spent on function compile. I did test quick and not sure if I miss anything. We can double check and let me know if any question. Thanks!

Approach 1: Existing Implicit Cast

Benchmark                                                               (dateTimeValue)  (dateValue)  Mode  Cnt   Score    Error  Units
TypeCastOperatorBenchmark.testDateEqualsDateTimeCompileAndEvaluate  2022-01-01 00:00:00   2022-01-01  avgt    3   0.006 ±  0.003  ms/op
TypeCastOperatorBenchmark.testDateEqualsDateTimeEvaluate            2022-01-01 00:00:00   2022-01-01  avgt    3  ≈ 10⁻⁵           ms/op

Approach 2: Current PR Approach

Benchmark                                                               (dateTimeValue)  (dateValue)  Mode  Cnt   Score    Error  Units
TypeCastOperatorBenchmark.testDateEqualsDateTimeCompileAndEvaluate  2022-01-01 00:00:00   2022-01-01  avgt    3   0.004 ±  0.002  ms/op
TypeCastOperatorBenchmark.testDateEqualsDateTimeEvaluate            2022-01-01 00:00:00   2022-01-01  avgt    3  ≈ 10⁻⁵           ms/op

[Yury-Fridlyand]

Great job @dai-chen! Thank you for sharing your test results.
I'll proceed with your proposed approach.

[Yury-Fridlyand]

Ready for re-review.

[dai-chen]

LGTM. Thanks for the changes!

[dai-chen]

Since we enhanced CAST operator in this PR, can #853 be linked and closed as well?

[Yury-Fridlyand]

Since we enhanced CAST operator in this PR, can #853 be linked and closed as well?

No,
#853 is about unsupported DATETIME in cast in V2. It is supported in V1, so #853 is a part of legacy deprecation.

opensearchsql> select cast('1961-04-12 09:07:00' as DATETIME);
{'reason': 'Invalid SQL query', 'details': '', 'type': 'NullPointerException'}

[dai-chen]

Since we enhanced CAST operator in this PR, can #853 be linked and closed as well?

No, #853 is about unsupported DATETIME in cast in V2. It is supported in V1, so #853 is a part of legacy deprecation.

opensearchsql> select cast('1961-04-12 09:07:00' as DATETIME);
{'reason': 'Invalid SQL query', 'details': '', 'type': 'NullPointerException'}

You mean with your changes in this PR below, CAST(string as datetime) still not work in v2? Is it because grammar changes still missing?

  private static DefaultFunctionResolver castToDatetime() {
    return FunctionDSL.define(BuiltinFunctionName.CAST_TO_DATETIME.getName(),
        impl(nullMissingHandling(
            (v) -> new ExprDatetimeValue(v.stringValue())), DATETIME, STRING),
        impl(nullMissingHandling(
            (v) -> new ExprDatetimeValue(v.datetimeValue())), DATETIME, TIMESTAMP),
        impl(nullMissingHandling(
            (v) -> new ExprDatetimeValue(v.datetimeValue())), DATETIME, DATE),
        implWithProperties(nullMissingHandlingWithProperties(
            (fp, v) -> new ExprDatetimeValue(((ExprTimeValue)v).datetimeValue(fp))),
            DATETIME, TIME),
        impl(nullMissingHandling((v) -> v), DATETIME, DATETIME)
    );

[Yury-Fridlyand]

Is it because grammar changes still missing?

Exactly, grammar fix is required

[dai-chen]

Is it because grammar changes still missing?

Exactly, grammar fix is required

I see. Thanks!