[GitHub] [druid] paul-rogers commented on a change in pull request #11184: vectorize logical operators and boolean functions

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

paul-rogers commented on a change in pull request #11184:
URL: https://github.com/apache/druid/pull/11184#discussion_r758771121



##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -385,7 +322,59 @@ protected BinaryOpExprBase copy(Expr left, Expr right)
   public ExprEval eval(ObjectBinding bindings)
   {
     ExprEval leftVal = left.eval(bindings);
-    return leftVal.asBoolean() ? right.eval(bindings) : leftVal;
+    if (!ExpressionProcessing.useStrictBooleans()) {
+      return leftVal.asBoolean() ? right.eval(bindings) : leftVal;
+    }
+
+    // if left is false, always false
+    if (leftVal.value() != null && !leftVal.asBoolean()) {
+      return ExprEval.ofLongBoolean(false);

Review comment:
       This looks like a constant. Can it be defined as such? `static final ExprEval.FALSE = ExprEval.ofLongBoolean(false)` to avoid recomputing the value in an inner loop? Here and below.

##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -385,7 +322,59 @@ protected BinaryOpExprBase copy(Expr left, Expr right)
   public ExprEval eval(ObjectBinding bindings)
   {
     ExprEval leftVal = left.eval(bindings);
-    return leftVal.asBoolean() ? right.eval(bindings) : leftVal;
+    if (!ExpressionProcessing.useStrictBooleans()) {
+      return leftVal.asBoolean() ? right.eval(bindings) : leftVal;
+    }
+
+    // if left is false, always false
+    if (leftVal.value() != null && !leftVal.asBoolean()) {
+      return ExprEval.ofLongBoolean(false);
+    }
+    ExprEval rightVal;
+    if (NullHandling.sqlCompatible() || Types.is(leftVal.type(), ExprType.STRING)) {

Review comment:
       Same comment, can the `NullHandling.sqlCompatible()` be cached?
   
   Better, since the value never changes, can there be separate implementations for the two cases so we pick the one we want up front and never have to check again? ("Up-front" would be the start of this part of a query, if the value can change per query.)
   
   Picking the exact right code is not quite code gen, but is pretty close in terms of performance.

##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
##########
@@ -385,7 +322,59 @@ protected BinaryOpExprBase copy(Expr left, Expr right)
   public ExprEval eval(ObjectBinding bindings)
   {
     ExprEval leftVal = left.eval(bindings);
-    return leftVal.asBoolean() ? right.eval(bindings) : leftVal;
+    if (!ExpressionProcessing.useStrictBooleans()) {

Review comment:
       This is an inner loop. Can the `!ExpressionProcessing.useStrictBooleans()` be evaluated on setup and reused here rather than making this call every time? The value can't change. Maybe the JVM will inline the call, but better to just eliminate it in the inner-loop code path.

##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -213,20 +220,69 @@ default boolean areNumeric(List<Expr> args)
     }
 
     /**
-     * Check if all provided {@link Expr} can infer the output type as {@link ExpressionType#isNumeric} with a value of true.
+     * Check if all provided {@link Expr} can infer the output type as {@link ExpressionType#isNumeric} with a value
+     * of true (or null, which is not a type)
      *
      * There must be at least one expression with a computable numeric output type for this method to return true.
+     *
+     * This method should only be used if {@link #getType} produces accurate information for all bindings (no null
+     * value for type unless the input binding does not exist and so the input is always null)
+     *
+     * @see #getOutputType(InputBindingInspector)
      */
     default boolean areNumeric(Expr... args)
     {
       return areNumeric(Arrays.asList(args));
     }
 
     /**
-     * Check if all provided {@link Expr} can infer the output type as {@link ExpressionType#isPrimitive()} (non-array) with a
-     * value of true.
+     * Check if all arguments are the same type (or null, which is not a type)
+     *
+     * This method should only be used if {@link #getType} produces accurate information for all bindings (no null
+     * value for type unless the input binding does not exist and so the input is always null)
+     *
+     * @see #getOutputType(InputBindingInspector)
+     */
+    default boolean areSameTypes(List<Expr> args)
+    {
+      ExpressionType currentType = null;
+      boolean allSame = true;
+      for (Expr arg : args) {
+        ExpressionType argType = arg.getOutputType(this);
+        if (argType == null) {
+          continue;
+        }
+        if (currentType == null) {
+          currentType = argType;
+        }
+        allSame &= Objects.equals(argType, currentType);

Review comment:
       Simpler to do a short-circuit AND:
   
   ```java
       for ... {
         if (!Objects.equals(argType, currentType)) {
           return false;
         }
       }
       return true;
   ```

##########
File path: core/src/main/java/org/apache/druid/math/expr/BinaryOperatorExpr.java
##########
@@ -157,3 +158,67 @@ protected ExprEval evalString(@Nullable String left, @Nullable String right)
 
   protected abstract double evalDouble(double left, double right);
 }
+
+@SuppressWarnings("ClassName")
+abstract class BinaryBooleanOpExprBase extends BinaryOpExprBase
+{
+  BinaryBooleanOpExprBase(String op, Expr left, Expr right)
+  {
+    super(op, left, right);
+  }
+
+  @Override
+  public ExprEval eval(ObjectBinding bindings)
+  {
+    ExprEval leftVal = left.eval(bindings);
+    ExprEval rightVal = right.eval(bindings);
+
+    // Result of any Binary expressions is null if any of the argument is null.
+    // e.g "select null * 2 as c;" or "select null + 1 as c;" will return null as per Standard SQL spec.
+    if (NullHandling.sqlCompatible() && (leftVal.value() == null || rightVal.value() == null)) {
+      return ExprEval.of(null);
+    }
+
+    ExpressionType type = ExpressionTypeConversion.autoDetect(leftVal, rightVal);
+    boolean result;
+    switch (type.getType()) {

Review comment:
       Even better is to have different implementations for each type so the switch is done at setup time, not in the inner loop.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

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