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

…pensearch-project#1196) (opensearch-project#1294)

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

* Extend comparison methods to accept different datetime types.

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

* Typo fix.

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

* Rework fix according to @dai-chen's comment.
opensearch-project#1196 (review)

* Revert `BinaryPredicateOperator`.
* Add automatic cast rules `DATE`/`TIME`/`DATETIME` -> `DATETIME`/TIMESTAMP`.
* Update unit tests.

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

* Add doctest sample.

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

* Docs typo fix.

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

* Minor comments update.

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

* Doctest typo fix.

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

* Doctest typo fix.

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

* Doctest typo fix.

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

* Modify `ExprCoreType` dependencies.

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

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
(cherry picked from commit a4f8066)

Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>

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

@@ -19,9 +19,11 @@ public abstract class AbstractExprValue implements ExprValue {
   public int compareTo(ExprValue other) {
     if (this.isNull() || this.isMissing() || other.isNull() || other.isMissing()) {
       throw new IllegalStateException(
-           String.format("[BUG] Unreachable, Comparing with NULL or MISSING is undefined"));
+           "[BUG] Unreachable, Comparing with NULL or MISSING is undefined");
     }
-    if ((this.isNumber() && other.isNumber()) || this.type() == other.type()) {
+    if ((this.isNumber() && other.isNumber())
+        || (this.isDateTime() && other.isDateTime())
+        || this.type() == other.type()) {
       return compare(other);
     } else {
       throw new ExpressionEvaluationException(

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

@@ -71,6 +71,11 @@ public Instant timestampValue() {
     return ZonedDateTime.of(date, timeValue(), ExprTimestampValue.ZONE).toInstant();
   }
 
+  @Override
+  public boolean isDateTime() {
+    return true;
+  }
+
   @Override
   public String toString() {
     return String.format("DATE '%s'", value());

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

@@ -73,6 +73,11 @@ public Instant timestampValue() {
     return ZonedDateTime.of(datetime, ExprTimestampValue.ZONE).toInstant();
   }
 
+  @Override
+  public boolean isDateTime() {
+    return true;
+  }
+
   @Override
   public int compare(ExprValue other) {
     return datetime.compareTo(other.datetimeValue());

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

@@ -70,6 +70,11 @@ public Instant timestampValue(FunctionProperties functionProperties) {
         .toInstant();
   }
 
+  @Override
+  public boolean isDateTime() {
+    return true;
+  }
+
   @Override
   public String toString() {
     return String.format("TIME '%s'", value());

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

@@ -81,6 +81,11 @@ public LocalDateTime datetimeValue() {
     return timestamp.atZone(ZONE).toLocalDateTime();
   }
 
+  @Override
+  public boolean isDateTime() {
+    return true;
+  }
+
   @Override
   public String toString() {
     return String.format("TIMESTAMP '%s'", value());

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

@@ -60,6 +60,15 @@ default boolean isNumber() {
     return false;
   }
 
+  /**
+   * Is Datetime value.
+   *
+   * @return true: is a datetime value, otherwise false
+   */
+  default boolean isDateTime() {
+    return false;
+  }
+
   /**
    * Get the {@link BindingTuple}.
    */

core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java

@@ -53,12 +53,11 @@ public enum ExprCoreType implements ExprType {
 
   /**
    * Date.
-   * Todo. compatible relationship.
    */
-  TIMESTAMP(STRING),
   DATE(STRING),
   TIME(STRING),
-  DATETIME(STRING),
+  DATETIME(STRING, DATE, TIME),
+  TIMESTAMP(STRING, DATETIME),
   INTERVAL(UNDEFINED),
 
   /**

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

@@ -540,28 +540,52 @@ public static FunctionExpression not(Expression... expressions) {
     return compile(FunctionProperties.None, BuiltinFunctionName.NOT, expressions);
   }
 
+  public static FunctionExpression equal(FunctionProperties fp, Expression... expressions) {
+    return compile(fp, BuiltinFunctionName.EQUAL, expressions);
+  }
+
   public static FunctionExpression equal(Expression... expressions) {
-    return compile(FunctionProperties.None, BuiltinFunctionName.EQUAL, expressions);
+    return equal(FunctionProperties.None, expressions);
+  }
+
+  public static FunctionExpression notequal(FunctionProperties fp, Expression... expressions) {
+    return compile(fp, BuiltinFunctionName.NOTEQUAL, expressions);
   }
 
   public static FunctionExpression notequal(Expression... expressions) {
-    return compile(FunctionProperties.None, BuiltinFunctionName.NOTEQUAL, expressions);
+    return notequal(FunctionProperties.None, expressions);
+  }
+
+  public static FunctionExpression less(FunctionProperties fp, Expression... expressions) {
+    return compile(fp, BuiltinFunctionName.LESS, expressions);
   }
 
   public static FunctionExpression less(Expression... expressions) {
-    return compile(FunctionProperties.None, BuiltinFunctionName.LESS, expressions);
+    return less(FunctionProperties.None, expressions);
+  }
+
+  public static FunctionExpression lte(FunctionProperties fp, Expression... expressions) {
+    return compile(fp, BuiltinFunctionName.LTE, expressions);
   }
 
   public static FunctionExpression lte(Expression... expressions) {
-    return compile(FunctionProperties.None, BuiltinFunctionName.LTE, expressions);
+    return lte(FunctionProperties.None, expressions);
+  }
+
+  public static FunctionExpression greater(FunctionProperties fp, Expression... expressions) {
+    return compile(fp, BuiltinFunctionName.GREATER, expressions);
   }
 
   public static FunctionExpression greater(Expression... expressions) {
-    return compile(FunctionProperties.None, BuiltinFunctionName.GREATER, expressions);
+    return greater(FunctionProperties.None, expressions);
+  }
+
+  public static FunctionExpression gte(FunctionProperties fp, Expression... expressions) {
+    return compile(fp, BuiltinFunctionName.GTE, expressions);
   }
 
   public static FunctionExpression gte(Expression... expressions) {
-    return compile(FunctionProperties.None, BuiltinFunctionName.GTE, expressions);
+    return gte(FunctionProperties.None, expressions);
   }
 
   public static FunctionExpression like(Expression... expressions) {

core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperator.java

@@ -19,7 +19,9 @@
 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;
@@ -176,12 +178,18 @@ private static DefaultFunctionResolver castToTime() {
     );
   }
 
+  // `DATE`/`TIME`/`DATETIME` -> `DATETIME`/TIMESTAMP` cast tested in BinaryPredicateOperatorTest
   private static DefaultFunctionResolver castToTimestamp() {
     return FunctionDSL.define(BuiltinFunctionName.CAST_TO_TIMESTAMP.getName(),
         impl(nullMissingHandling(
             (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)
     );
   }
@@ -193,7 +201,11 @@ private static DefaultFunctionResolver castToDatetime() {
         impl(nullMissingHandling(
             (v) -> new ExprDatetimeValue(v.datetimeValue())), DATETIME, TIMESTAMP),
         impl(nullMissingHandling(
-            (v) -> new ExprDatetimeValue(v.datetimeValue())), DATETIME, DATE)
+            (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)
     );
   }
 }

core/src/test/java/org/opensearch/sql/data/model/ExprBooleanValueTest.java

@@ -8,18 +8,55 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.opensearch.sql.utils.ComparisonUtil.compare;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.opensearch.sql.data.type.ExprCoreType.BOOLEAN;
 
 import org.junit.jupiter.api.Test;
 import org.opensearch.sql.exception.ExpressionEvaluationException;
 
 public class ExprBooleanValueTest {
 
   @Test
-  public void comparabilityTest() {
-    ExprValue booleanValue = ExprValueUtils.booleanValue(false);
-    ExpressionEvaluationException exception = assertThrows(ExpressionEvaluationException.class,
-        () -> compare(booleanValue, booleanValue));
-    assertEquals("ExprBooleanValue instances are not comparable", exception.getMessage());
+  public void equals_to_self() {
+    ExprValue value = ExprValueUtils.booleanValue(false);
+    assertEquals(value.booleanValue(), false);
+    assertTrue(value.equals(value));
+  }
+
+  @Test
+  public void equal() {
+    ExprValue v1 = ExprBooleanValue.of(true);
+    ExprValue v2 = ExprBooleanValue.of(true);
+    assertTrue(v1.equals(v2));
+    assertTrue(v2.equals(v1));
+    assertEquals(0, ((ExprBooleanValue)v1).compare((ExprBooleanValue)v2));
+    assertEquals(0, ((ExprBooleanValue)v2).compare((ExprBooleanValue)v1));
+  }
+
+  @Test
+  public void compare() {
+    var v1 = ExprBooleanValue.of(true);
+    var v2 = ExprBooleanValue.of(false);
+    assertEquals(1, v1.compare(v2));
+    assertEquals(-1, v2.compare(v1));
+  }
+
+  @Test
+  public void invalid_get_value() {
+    ExprDateValue value = new ExprDateValue("2020-08-20");
+    assertThrows(ExpressionEvaluationException.class, value::booleanValue,
+        String.format("invalid to get booleanValue from value of type %s", value.type()));
+  }
+
+  @Test
+  public void value() {
+    ExprValue value = ExprBooleanValue.of(true);
+    assertEquals(true, value.value());
+  }
+
+  @Test
+  public void type() {
+    ExprValue value = ExprBooleanValue.of(false);
+    assertEquals(BOOLEAN, value.type());
   }
 }

core/src/test/java/org/opensearch/sql/data/model/ExprStringValueTest.java

@@ -0,0 +1,60 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.opensearch.sql.data.model;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.opensearch.sql.data.type.ExprCoreType.STRING;
+
+import org.junit.jupiter.api.Test;
+import org.opensearch.sql.exception.ExpressionEvaluationException;
+
+public class ExprStringValueTest {
+  @Test
+  public void equals_to_self() {
+    ExprValue string = ExprValueUtils.stringValue("str");
+    assertEquals(string.stringValue(), "str");
+    assertTrue(string.equals(string));
+  }
+
+  @Test
+  public void equal() {
+    ExprValue v1 = new ExprStringValue("str");
+    ExprValue v2 = ExprValueUtils.stringValue("str");
+    assertTrue(v1.equals(v2));
+    assertTrue(v2.equals(v1));
+    assertEquals(0, ((ExprStringValue)v1).compare((ExprStringValue)v2));
+    assertEquals(0, ((ExprStringValue)v2).compare((ExprStringValue)v1));
+  }
+
+  @Test
+  public void compare() {
+    ExprStringValue v1 = new ExprStringValue("str1");
+    ExprStringValue v2 = new ExprStringValue("str2");
+    assertEquals(-1, v1.compare(v2));
+    assertEquals(1, v2.compare(v1));
+  }
+
+  @Test
+  public void invalid_get_value() {
+    ExprDateValue value = new ExprDateValue("2020-08-20");
+    assertThrows(ExpressionEvaluationException.class, value::stringValue,
+        String.format("invalid to get intervalValue from value of type %s", value.type()));
+  }
+
+  @Test
+  public void value() {
+    ExprValue value = new ExprStringValue("string");
+    assertEquals("string", value.value());
+  }
+
+  @Test
+  public void type() {
+    ExprValue value = new ExprStringValue("string");
+    assertEquals(STRING, value.type());
+  }
+}