[GitHub] [druid] clintropolis commented on a change in pull request #10370: add computed Expr output types

[GitBox <gi...@apache.org>]

clintropolis commented on a change in pull request #10370:
URL: https://github.com/apache/druid/pull/10370#discussion_r485989681



##########
File path: core/src/main/java/org/apache/druid/math/expr/ExprType.java
##########
@@ -29,5 +35,116 @@
   STRING,
   DOUBLE_ARRAY,
   LONG_ARRAY,
-  STRING_ARRAY
+  STRING_ARRAY;
+
+  public boolean isNumeric()
+  {
+    return isNumeric(this);
+  }
+
+  /**
+   * The expression system does not distinguish between {@link ValueType#FLOAT} and {@link ValueType#DOUBLE}, and
+   * cannot currently handle {@link ValueType#COMPLEX} inputs. This method will convert {@link ValueType#FLOAT} to
+   * {@link #DOUBLE}, or throw an exception if a {@link ValueType#COMPLEX} is encountered.
+   *
+   * @throws IllegalStateException
+   */
+  public static ExprType fromValueType(@Nullable ValueType valueType)
+  {
+    if (valueType == null) {
+      throw new IllegalStateException("Unsupported unknown value type");
+    }
+    switch (valueType) {
+      case LONG:
+        return LONG;
+      case LONG_ARRAY:
+        return LONG_ARRAY;
+      case FLOAT:
+      case DOUBLE:
+        return DOUBLE;
+      case DOUBLE_ARRAY:
+        return DOUBLE_ARRAY;
+      case STRING:
+        return STRING;
+      case STRING_ARRAY:
+        return STRING_ARRAY;
+      case COMPLEX:
+      default:
+        throw new ISE("Unsupported value type[%s]", valueType);
+    }
+  }
+
+  public static boolean isNumeric(ExprType type)
+  {
+    return LONG.equals(type) || DOUBLE.equals(type);
+  }
+
+  public static boolean isArray(@Nullable ExprType type)
+  {
+    return LONG_ARRAY.equals(type) || DOUBLE_ARRAY.equals(type) || STRING_ARRAY.equals(type);
+  }
+
+  @Nullable
+  public static ExprType elementType(@Nullable ExprType type)
+  {
+    if (type != null && isArray(type)) {
+      switch (type) {
+        case STRING_ARRAY:
+          return STRING;
+        case LONG_ARRAY:
+          return LONG;
+        case DOUBLE_ARRAY:
+          return DOUBLE;
+      }
+    }
+    return type;
+  }
+
+  @Nullable
+  public static ExprType asArrayType(@Nullable ExprType elementType)
+  {
+    if (elementType != null && !isArray(elementType)) {
+      switch (elementType) {
+        case STRING:
+          return STRING_ARRAY;
+        case LONG:
+          return LONG_ARRAY;
+        case DOUBLE:
+          return DOUBLE_ARRAY;
+      }
+    }
+    return elementType;
+  }
+
+  @Nullable
+  public static ExprType implicitCast(@Nullable ExprType type, @Nullable ExprType other)
+  {
+    if (type == null || other == null) {
+      // cannot implicitly cast unknown types
+      return null;
+    }
+    // arrays cannot be implicitly cast
+    if (isArray(type)) {
+      if (!type.equals(other)) {
+        throw new IAE("Cannot implicitly cast %s to %s", type, other);
+      }
+      return type;
+    }
+    // if either argument is a string, type becomes a string
+    if (STRING.equals(type) || STRING.equals(other)) {
+      return STRING;
+    }
+
+    if (isNumeric(type) && isNumeric(other)) {
+      // all numbers win over longs
+      if (LONG.equals(type)) {
+        return other;
+      }
+      // floats vs longs would be handled here, but we currently only support doubles...
+      return type;
+    }
+
+    // unhandled is unknown
+    return null;

Review comment:
       Maybe? This case can't actually happen right now, all cases are actually covered in previous clauses so this line should currently never be hit. I was sort of starting to imagine ahead to a world where `ExprType` and `ValueType` are unified and things like complex types could appear in a type conversion logic block similar to this. I sort of expect this area of the code to be somewhat volatile in the near future as I work out some things about type handling.
   
   Usage wise, this method is basically only used to compute the output type given 2 input types, so it at least partially makes sense to return null if we can't determine the output type. I can see an argument for this being an exception as well (as well as either argument being null). `implicitCast` isn't a great name for this method either, I should probably call it something else, maybe something like `implicitTypeConversion` or .. something.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org