fix expression post aggregator array handling when grouping wrapper t…

…ypes leak (apache#15543) * fix expression post aggregator array handling when grouping wrapper types leak * more consistent expression function error messaging
monetate · Dec 16, 2023 · e373f62 · e373f62
1 parent 90af71b
commit e373f62
Show file tree

Hide file tree

Showing 16 changed files with 521 additions and 404 deletions.
diff --git a/processing/src/main/java/org/apache/druid/math/expr/BinaryEvalOpExprBase.java b/processing/src/main/java/org/apache/druid/math/expr/BinaryEvalOpExprBase.java
@@ -207,7 +207,7 @@ public ExprEval eval(ObjectBinding bindings)
         break;
     }
     if (!ExpressionProcessing.useStrictBooleans() && !type.is(ExprType.STRING) && !type.isArray()) {
-      return ExprEval.ofBoolean(result, type.getType());
+      return ExprEval.ofBoolean(result, type);
     }
     return ExprEval.ofLongBoolean(result);
   }

diff --git a/processing/src/main/java/org/apache/druid/math/expr/ExprEval.java b/processing/src/main/java/org/apache/druid/math/expr/ExprEval.java
@@ -30,6 +30,9 @@
 import org.apache.druid.segment.column.NullableTypeStrategy;
 import org.apache.druid.segment.column.TypeStrategies;
 import org.apache.druid.segment.column.TypeStrategy;
+import org.apache.druid.segment.column.Types;
+import org.apache.druid.segment.data.ComparableList;
+import org.apache.druid.segment.data.ComparableStringArray;
 import org.apache.druid.segment.nested.StructuredData;
 
 import javax.annotation.Nullable;
@@ -357,17 +360,17 @@ public static ExprEval ofArray(ExpressionType outputType, @Nullable Object[] val
    * instead.
    */
   @Deprecated
-  public static ExprEval ofBoolean(boolean value, ExprType type)
+  public static ExprEval ofBoolean(boolean value, ExpressionType type)
   {
-    switch (type) {
+    switch (type.getType()) {
       case DOUBLE:
         return ExprEval.of(Evals.asDouble(value));
       case LONG:
         return ofLongBoolean(value);
       case STRING:
         return ExprEval.of(String.valueOf(value));
       default:
-        throw new IllegalArgumentException("Invalid type, cannot coerce [" + type + "] to boolean");
+        throw new Types.InvalidCastBooleanException(type);
     }
   }
 
@@ -502,6 +505,13 @@ public static ExprEval bestEffortOf(@Nullable Object val)
       final List<?> theList = val instanceof List ? ((List<?>) val) : Arrays.asList((Object[]) val);
       return bestEffortArray(theList);
     }
+    // handle leaky group by array types
+    if (val instanceof ComparableStringArray) {
+      return new ArrayExprEval(ExpressionType.STRING_ARRAY, ((ComparableStringArray) val).getDelegate());
+    }
+    if (val instanceof ComparableList) {
+      return bestEffortArray(((ComparableList) val).getDelegate());
+    }
 
     // in 'best effort' mode, we couldn't possibly use byte[] as a complex or anything else useful without type
     // knowledge, so lets turn it into a base64 encoded string so at least something downstream can use it by decoding
@@ -1569,8 +1579,8 @@ public Expr toExpr()
     }
   }
 
-  public static IAE invalidCast(ExpressionType fromType, ExpressionType toType)
+  public static Types.InvalidCastException invalidCast(ExpressionType fromType, ExpressionType toType)
   {
-    return new IAE("Invalid type, cannot cast [" + fromType + "] to [" + toType + "]");
+    return new Types.InvalidCastException(fromType, toType);
   }
 }
diff --git a/processing/src/main/java/org/apache/druid/math/expr/FunctionalExpr.java b/processing/src/main/java/org/apache/druid/math/expr/FunctionalExpr.java
@@ -24,6 +24,7 @@
 import org.apache.druid.error.DruidException;
 import org.apache.druid.java.util.common.StringUtils;
 import org.apache.druid.math.expr.vector.ExprVectorProcessor;
+import org.apache.druid.segment.column.Types;
 
 import javax.annotation.Nullable;
 import java.util.List;
@@ -190,11 +191,22 @@ public ExprEval eval(ObjectBinding bindings)
     try {
       return function.apply(args, bindings);
     }
-    catch (DruidException | ExpressionValidationException e) {
+    catch (ExpressionValidationException e) {
+      // ExpressionValidationException already contain function name
+      throw DruidException.forPersona(DruidException.Persona.USER)
+                          .ofCategory(DruidException.Category.INVALID_INPUT)
+                          .build(e, e.getMessage());
+    }
+    catch (Types.InvalidCastException | Types.InvalidCastBooleanException e) {
+      throw DruidException.forPersona(DruidException.Persona.USER)
+                          .ofCategory(DruidException.Category.INVALID_INPUT)
+                          .build(e, "Function[%s] encountered exception: %s", name, e.getMessage());
+    }
+    catch (DruidException e) {
       throw e;
     }
     catch (Exception e) {
-      throw DruidException.defensive().build(e, "Invocation of function '%s' encountered exception.", name);
+      throw DruidException.defensive().build(e, "Function[%s] encountered unknown exception.", name);
     }
   }
 

diff --git a/processing/src/main/java/org/apache/druid/math/expr/UnaryOperatorExpr.java b/processing/src/main/java/org/apache/druid/math/expr/UnaryOperatorExpr.java
@@ -184,7 +184,7 @@ public ExprEval eval(ObjectBinding bindings)
     if (!ExpressionProcessing.useStrictBooleans()) {
       // conforming to other boolean-returning binary operators
       ExpressionType retType = ret.type().is(ExprType.DOUBLE) ? ExpressionType.DOUBLE : ExpressionType.LONG;
-      return ExprEval.ofBoolean(!ret.asBoolean(), retType.getType());
+      return ExprEval.ofBoolean(!ret.asBoolean(), retType);
     }
     return ExprEval.ofLongBoolean(!ret.asBoolean());
   }

diff --git a/processing/src/main/java/org/apache/druid/segment/column/Types.java b/processing/src/main/java/org/apache/druid/segment/column/Types.java
@@ -141,4 +141,20 @@ public IncompatibleTypeException(TypeSignature<?> type, TypeSignature<?> other)
       super("Cannot implicitly cast [%s] to [%s]", type, other);
     }
   }
+
+  public static class InvalidCastException extends IAE
+  {
+    public InvalidCastException(TypeSignature<?> type, TypeSignature<?> other)
+    {
+      super("Invalid type, cannot cast [" + type + "] to [" + other + "]");
+    }
+  }
+
+  public static class InvalidCastBooleanException extends IAE
+  {
+    public InvalidCastBooleanException(TypeSignature<?> type)
+    {
+      super("Invalid type, cannot coerce [" + type + "] to boolean");
+    }
+  }
 }
diff --git a/processing/src/test/java/org/apache/druid/math/expr/EvalTest.java b/processing/src/test/java/org/apache/druid/math/expr/EvalTest.java
@@ -28,6 +28,8 @@
 import org.apache.druid.java.util.common.StringUtils;
 import org.apache.druid.segment.column.TypeStrategies;
 import org.apache.druid.segment.column.TypeStrategiesTest;
+import org.apache.druid.segment.data.ComparableList;
+import org.apache.druid.segment.data.ComparableStringArray;
 import org.apache.druid.segment.nested.StructuredData;
 import org.apache.druid.testing.InitializedNullHandlingTest;
 import org.junit.Assert;
@@ -1683,6 +1685,18 @@ public void testBestEffortOf()
         ExpressionType.UNKNOWN_COMPLEX,
         someOtherComplex
     );
+
+    assertBestEffortOf(
+        ComparableStringArray.of("a", "b", "c"),
+        ExpressionType.STRING_ARRAY,
+        new Object[]{"a", "b", "c"}
+    );
+
+    assertBestEffortOf(
+        new ComparableList<>(Arrays.asList(1L, 2L)),
+        ExpressionType.LONG_ARRAY,
+        new Object[]{1L, 2L}
+    );
   }
 
   private void assertBestEffortOf(@Nullable Object val, ExpressionType expectedType, @Nullable Object expectedValue)