Simplify datetime-string compare logic with implict coercion

Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>

Author: yuancu

core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java

@@ -11,8 +11,6 @@
 import static org.opensearch.sql.ast.expression.SpanUnit.NONE;
 import static org.opensearch.sql.ast.expression.SpanUnit.UNKNOWN;
 import static org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.TYPE_FACTORY;
-import static org.opensearch.sql.utils.DateTimeUtils.findCastType;
-import static org.opensearch.sql.utils.DateTimeUtils.transferCompareForDateRelated;
 
 import java.math.BigDecimal;
 import java.util.ArrayList;
@@ -215,11 +213,8 @@ public RexNode visitIn(In node, CalcitePlanContext context) {
 
   @Override
   public RexNode visitCompare(Compare node, CalcitePlanContext context) {
-    RexNode leftCandidate = analyze(node.getLeft(), context);
-    RexNode rightCandidate = analyze(node.getRight(), context);
-    SqlTypeName castTarget = findCastType(leftCandidate, rightCandidate);
-    final RexNode left = transferCompareForDateRelated(leftCandidate, context, castTarget);
-    final RexNode right = transferCompareForDateRelated(rightCandidate, context, castTarget);
+    RexNode left = analyze(node.getLeft(), context);
+    RexNode right = analyze(node.getRight(), context);
     return PPLFuncImpTable.INSTANCE.resolve(context.rexBuilder, node.getOperator(), left, right);
   }
 

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

@@ -11,6 +11,7 @@
 import org.apache.calcite.rex.RexBuilder;
 import org.apache.calcite.rex.RexNode;
 import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory;
+import org.opensearch.sql.data.type.ExprCoreType;
 import org.opensearch.sql.data.type.ExprType;
 import org.opensearch.sql.data.type.WideningTypeRule;
 import org.opensearch.sql.exception.ExpressionEvaluationException;
@@ -121,12 +122,22 @@ public class CoercionUtils {
     for (int i = 1; i < arguments.size(); i++) {
       var type = OpenSearchTypeFactory.convertRelDataTypeToExprType(arguments.get(i).getType());
       try {
-        widestType = WideningTypeRule.max(widestType, type);
+        if (areDateAndTime(widestType, type)) {
+          // If one is date and the other is time, we consider timestamp as the widest type
+          widestType = ExprCoreType.TIMESTAMP;
+        } else {
+          widestType = WideningTypeRule.max(widestType, type);
+        }
       } catch (ExpressionEvaluationException e) {
         // the two types are not compatible, return null
         return null;
       }
     }
     return widestType;
   }
+
+  private static boolean areDateAndTime(ExprType type1, ExprType type2) {
+    return (type1 == ExprCoreType.DATE && type2 == ExprCoreType.TIME)
+        || (type1 == ExprCoreType.TIME && type2 == ExprCoreType.DATE);
+  }
 }

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

@@ -15,6 +15,7 @@
 import java.util.stream.IntStream;
 import lombok.RequiredArgsConstructor;
 import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeField;
 import org.apache.calcite.sql.SqlIntervalQualifier;
 import org.apache.calcite.sql.parser.SqlParserPos;
 import org.apache.calcite.sql.type.CompositeOperandTypeChecker;
@@ -25,6 +26,7 @@
 import org.apache.calcite.sql.type.SqlTypeFamily;
 import org.apache.calcite.sql.type.SqlTypeName;
 import org.apache.calcite.sql.type.SqlTypeUtil;
+import org.apache.calcite.util.Pair;
 import org.opensearch.sql.calcite.type.ExprIPType;
 import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory;
 import org.opensearch.sql.calcite.utils.UserDefinedFunctionUtils;
@@ -257,28 +259,56 @@ public boolean checkOperandTypes(List<RelDataType> types) {
       for (int i = 0; i < types.size() - 1; i++) {
         // TODO: Binary, Array UDT?
         // DATETIME, NUMERIC, BOOLEAN will be regarded as comparable
-        // with strings in SqlTypeUtil.isComparable
+        // with strings in isComparable
         RelDataType type_l = types.get(i);
         RelDataType type_r = types.get(i + 1);
-        if (!SqlTypeUtil.isComparable(type_l, type_r)) {
-          return false;
-        }
-        // Disallow coercing between strings and numeric, boolean
-        if ((type_l.getFamily() == SqlTypeFamily.CHARACTER
-                && cannotConvertStringInCompare((SqlTypeFamily) type_r.getFamily()))
-            || (type_r.getFamily() == SqlTypeFamily.CHARACTER
-                && cannotConvertStringInCompare((SqlTypeFamily) type_l.getFamily()))) {
+        if (!isComparable(type_l, type_r)) {
           return false;
         }
       }
       return true;
     }
 
-    private static boolean cannotConvertStringInCompare(SqlTypeFamily typeFamily) {
-      return switch (typeFamily) {
-        case BOOLEAN, INTEGER, NUMERIC, EXACT_NUMERIC, APPROXIMATE_NUMERIC -> true;
-        default -> false;
-      };
+    /**
+     * Modified from {@link SqlTypeUtil#isComparable(RelDataType, RelDataType)} to
+     *
+     * @param type1 first type
+     * @param type2 second type
+     * @return true if the two types are comparable, false otherwise
+     */
+    private static boolean isComparable(RelDataType type1, RelDataType type2) {
+      if (type1.isStruct() != type2.isStruct()) {
+        return false;
+      }
+
+      if (type1.isStruct()) {
+        int n = type1.getFieldCount();
+        if (n != type2.getFieldCount()) {
+          return false;
+        }
+        for (Pair<RelDataTypeField, RelDataTypeField> pair :
+            Pair.zip(type1.getFieldList(), type2.getFieldList())) {
+          if (!isComparable(pair.left.getType(), pair.right.getType())) {
+            return false;
+          }
+        }
+        return true;
+      }
+
+      // Numeric types are comparable without the need to cast
+      if (SqlTypeUtil.isNumeric(type1) && SqlTypeUtil.isNumeric(type2)) {
+        return true;
+      }
+
+      ExprType exprType1 = OpenSearchTypeFactory.convertRelDataTypeToExprType(type1);
+      ExprType exprType2 = OpenSearchTypeFactory.convertRelDataTypeToExprType(type2);
+
+      if (!exprType1.shouldCast(exprType2)) {
+        return true;
+      }
+
+      // If one of the arguments is of type 'ANY', return true.
+      return type1.getFamily() == SqlTypeFamily.ANY || type2.getFamily() == SqlTypeFamily.ANY;
     }
 
     @Override

core/src/main/java/org/opensearch/sql/utils/DateTimeUtils.java

@@ -5,10 +5,6 @@
 
 package org.opensearch.sql.utils;
 
-import static org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.ExprUDT.EXPR_DATE;
-import static org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.ExprUDT.EXPR_TIME;
-import static org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.ExprUDT.EXPR_TIMESTAMP;
-
 import java.time.Instant;
 import java.time.LocalDate;
 import java.time.LocalDateTime;
@@ -19,18 +15,11 @@
 import java.time.format.DateTimeParseException;
 import java.time.temporal.ChronoUnit;
 import java.util.Locale;
-import java.util.Objects;
 import java.util.regex.Pattern;
 import lombok.experimental.UtilityClass;
-import org.apache.calcite.rex.RexNode;
-import org.apache.calcite.sql.type.SqlTypeName;
-import org.opensearch.sql.calcite.CalcitePlanContext;
-import org.opensearch.sql.calcite.type.ExprSqlType;
-import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory;
 import org.opensearch.sql.data.model.ExprTimeValue;
 import org.opensearch.sql.data.model.ExprValue;
 import org.opensearch.sql.expression.function.FunctionProperties;
-import org.opensearch.sql.expression.function.PPLBuiltinOperators;
 
 @UtilityClass
 public class DateTimeUtils {
@@ -305,78 +294,4 @@ private static String normalizeUnit(String rawUnit) {
       }
     }
   }
-
-  /**
-   * The function add cast for date-related target node
-   *
-   * @param candidate The candidate node
-   * @param context calcite context
-   * @param castTarget the target cast type
-   * @return the rexnode after casting
-   */
-  public static RexNode transferCompareForDateRelated(
-      RexNode candidate, CalcitePlanContext context, SqlTypeName castTarget) {
-    if (!(Objects.isNull(castTarget))) {
-      switch (castTarget) {
-        case DATE:
-          if (!(candidate.getType() instanceof ExprSqlType
-              && ((ExprSqlType) candidate.getType()).getUdt() == EXPR_DATE)) {
-            return context.rexBuilder.makeCall(PPLBuiltinOperators.DATE, candidate);
-          }
-          break;
-        case TIME:
-          if (!(candidate.getType() instanceof ExprSqlType
-              && ((ExprSqlType) candidate.getType()).getUdt() == EXPR_TIME)) {
-            return context.rexBuilder.makeCall(PPLBuiltinOperators.TIME, candidate);
-          }
-          break;
-        case TIMESTAMP:
-          if (!(candidate.getType() instanceof ExprSqlType
-              && ((ExprSqlType) candidate.getType()).getUdt() == EXPR_TIMESTAMP)) {
-            return context.rexBuilder.makeCall(PPLBuiltinOperators.TIMESTAMP, candidate);
-          }
-          break;
-        default:
-          return candidate;
-      }
-    }
-    return candidate;
-  }
-
-  /**
-   * The function find the target cast type according to the left and right node. When the two node
-   * are both related to date with different type, cast to timestamp
-   *
-   * @param left
-   * @param right
-   * @return
-   */
-  public static SqlTypeName findCastType(RexNode left, RexNode right) {
-    SqlTypeName leftType = returnCorrespondingSqlType(left);
-    SqlTypeName rightType = returnCorrespondingSqlType(right);
-    if (leftType != null && rightType != null && rightType != leftType) {
-      return SqlTypeName.TIMESTAMP;
-    }
-    return leftType == null ? rightType : leftType;
-  }
-
-  /**
-   * Find corresponding cast type according to the node's type. If they're not related to the date,
-   * return null
-   *
-   * @param node the candidate node
-   * @return the sql type name
-   */
-  public static SqlTypeName returnCorrespondingSqlType(RexNode node) {
-    if (node.getType() instanceof ExprSqlType) {
-      OpenSearchTypeFactory.ExprUDT udt = ((ExprSqlType) node.getType()).getUdt();
-      return switch (udt) {
-        case EXPR_DATE -> SqlTypeName.DATE;
-        case EXPR_TIME -> SqlTypeName.TIME;
-        case EXPR_TIMESTAMP -> SqlTypeName.TIMESTAMP;
-        default -> null;
-      };
-    }
-    return null;
-  }
 }