Address PR feedback.

Yury-Fridlyand · Yury-Fridlyand · commit b04a92e1f1df · 2023-07-27T20:39:44.000-07:00
Signed-off-by: Yury-Fridlyand &lt;yury.fridlyand@improving.com&gt;
diff --git a/core/src/main/java/org/opensearch/sql/data/type/ExprType.java b/core/src/main/java/org/opensearch/sql/data/type/ExprType.java
@@ -20,8 +20,8 @@ public interface ExprType {
    * Is compatible with other types.
    */
   default boolean isCompatible(ExprType other) {
-    // Do double direction check with `equals`, because a derived class may override it
-    if (this.equals(other) || other.equals(this)) {
+    other = other.getExprType();
+    if (getExprType().equals(other)) {
       return true;
     } else {
       if (other.equals(UNKNOWN)) {
@@ -78,4 +78,11 @@ default String convertFieldForSearchQuery(String fieldName) {
   default Object convertValueForSearchQuery(ExprValue value) {
     return value.value();
   }
+
+  /**
+   * Get a simplified type {@link ExprCoreType} if possible. Used in {@link #isCompatible}.
+   */
+  default ExprType getExprType() {
+    return this;
+  }
 }
diff --git a/core/src/test/java/org/opensearch/sql/data/type/ExprTypeTest.java b/core/src/test/java/org/opensearch/sql/data/type/ExprTypeTest.java
@@ -50,22 +50,6 @@ public void isCompatible() {
     assertTrue(DATETIME.isCompatible(STRING));
   }
 
-  @Test
-  public void isCompatibleTwoDirectionCheck() {
-    ExprType other = new ExprType() {
-      @Override
-      public String typeName() {
-        return null;
-      }
-
-      @Override
-      public boolean equals(Object obj) {
-        return true;
-      }
-    };
-    assertTrue(UNDEFINED.isCompatible(other));
-  }
-
   @Test
   public void isNotCompatible() {
     assertFalse(INTEGER.isCompatible(DOUBLE));
diff --git a/docs/dev/img/type-hierarchy-tree-final.png b/docs/dev/img/type-hierarchy-tree-final.png
diff --git a/docs/dev/query-type-conversion.md b/docs/dev/query-type-conversion.md
@@ -85,7 +85,7 @@ Compiling time:
  Unresolved signature: equal(BOOL, STRING)
  Resovled signature: equal(BOOL, BOOL)
  Function builder: 1) returns equal(BOOL, cast_to_bool(STRING)) impl
-                   1) returns equal(BOOL, BOOL) impl
+                   2) returns equal(BOOL, BOOL) impl
 Runtime:
  equal impl: false.equals(cast_to_bool('FALSE'))
  cast_to_bool impl: Boolean.valueOf('FALSE')
diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java
@@ -15,6 +15,7 @@
 import java.util.function.BiConsumer;
 import lombok.Getter;
 import org.apache.commons.lang3.EnumUtils;
+import org.opensearch.sql.data.model.ExprValue;
 import org.opensearch.sql.data.type.ExprCoreType;
 import org.opensearch.sql.data.type.ExprType;
 import org.opensearch.sql.data.type.WideningTypeRule;
@@ -28,6 +29,7 @@ public class OpenSearchDataType implements ExprType, Serializable {
    * Redefine comparison operation: class (or derived) could be compared with ExprCoreType too.
    * Used in {@link WideningTypeRule#distance(ExprType, ExprType)}.
    */
+  @Override
   public boolean equals(final Object o) {
     if (o == this) {
       return true;
@@ -45,6 +47,10 @@ public boolean equals(final Object o) {
     return exprCoreType.equals(other.exprCoreType);
   }
 
+  /**
+   * Redefine hash calculation to enforce comparing using {@link #equals(Object)}.
+   */
+  @Override
   public int hashCode() {
     return 42 + exprCoreType.hashCode();
   }
@@ -120,6 +126,7 @@ public String toString() {
    * To avoid returning `UNKNOWN` for `OpenSearch*Type`s, e.g. for IP, returns itself.
    * @return An {@link ExprType}.
    */
+  @Override
   public ExprType getExprType() {
     if (exprCoreType != ExprCoreType.UNKNOWN) {
       return exprCoreType;
@@ -292,7 +299,7 @@ protected OpenSearchDataType cloneEmpty() {
   /**
    * Flattens mapping tree into a single layer list of objects (pairs of name-types actually),
    * which don't have nested types.
-   * See {@see OpenSearchDataTypeTest#traverseAndFlatten() test} for example.
+   * See <em>OpenSearchDataTypeTest::traverseAndFlatten()</em> test for example.
    * @param tree A list of `OpenSearchDataType`s - map between field name and its type.
    * @return A list of all `OpenSearchDataType`s from given map on the same nesting level (1).
    *         Nested object names are prefixed by names of their host.
diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeTest.java
@@ -42,6 +42,7 @@
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.EnumSource;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.opensearch.sql.data.model.ExprDoubleValue;
 import org.opensearch.sql.data.type.ExprCoreType;
 import org.opensearch.sql.data.type.ExprType;
 
@@ -450,12 +451,14 @@ private Map<String, OpenSearchDataType> getSampleMapping() {
 
   @Test
   public void getExprType() {
-    assertEquals(OpenSearchTextType.of(),
-            OpenSearchDataType.of(MappingType.Text).getExprType());
-    assertEquals(FLOAT, OpenSearchDataType.of(MappingType.Float).getExprType());
-    assertEquals(FLOAT, OpenSearchDataType.of(MappingType.HalfFloat).getExprType());
-    assertEquals(DOUBLE, OpenSearchDataType.of(MappingType.Double).getExprType());
-    assertEquals(DOUBLE, OpenSearchDataType.of(MappingType.ScaledFloat).getExprType());
-    assertEquals(TIMESTAMP, OpenSearchDataType.of(MappingType.Date).getExprType());
+    assertAll(
+        () -> assertEquals(OpenSearchTextType.of(),
+            OpenSearchDataType.of(MappingType.Text).getExprType()),
+        () -> assertEquals(FLOAT, OpenSearchDataType.of(MappingType.Float).getExprType()),
+        () -> assertEquals(FLOAT, OpenSearchDataType.of(MappingType.HalfFloat).getExprType()),
+        () -> assertEquals(DOUBLE, OpenSearchDataType.of(MappingType.Double).getExprType()),
+        () -> assertEquals(DOUBLE, OpenSearchDataType.of(MappingType.ScaledFloat).getExprType()),
+        () -> assertEquals(TIMESTAMP, OpenSearchDataType.of(MappingType.Date).getExprType())
+    );
   }
 }
