Refactor boolean cast code, add tests (#3016)

common/src/main/java/io/druid/math/expr/Evals.java

@@ -19,6 +19,7 @@
 
 package io.druid.math.expr;
 
+import com.google.common.base.Strings;
 import io.druid.common.guava.GuavaUtils;
 import io.druid.java.util.common.logger.Logger;
 
@@ -81,4 +82,34 @@ public class Evals
       return binary;  // best effort.. keep it working
     }
   }
+
+  public static long asLong(boolean x)
+  {
+    return x ? 1L : 0L;
+  }
+
+  public static double asDouble(boolean x)
+  {
+    return x ? 1D : 0D;
+  }
+
+  public static String asString(boolean x)
+  {
+    return String.valueOf(x);
+  }
+
+  public static boolean asBoolean(long x)
+  {
+    return x > 0;
+  }
+
+  public static boolean asBoolean(double x)
+  {
+    return x > 0;
+  }
+
+  public static boolean asBoolean(String x)
+  {
+    return !Strings.isNullOrEmpty(x) && Boolean.valueOf(x);
+  }
 }

common/src/main/java/io/druid/math/expr/Expr.java

@@ -23,7 +23,6 @@ import com.google.common.math.LongMath;
 import io.druid.java.util.common.IAE;
 
 import java.util.List;
-import java.util.Objects;
 
 /**
  */
@@ -242,13 +241,9 @@ class UnaryNotExpr extends UnaryExpr
   public ExprEval eval(ObjectBinding bindings)
   {
     ExprEval ret = expr.eval(bindings);
-    if (ret.type() == ExprType.LONG) {
-      return ExprEval.of(ret.asBoolean() ? 0L : 1L);
-    }
-    if (ret.type() == ExprType.DOUBLE) {
-      return ExprEval.of(ret.asBoolean() ? 0.0d :1.0d);
-    }
-    throw new IllegalArgumentException("unsupported type " + ret.type());
+    // conforming to other boolean-returning binary operators
+    ExprType retType = ret.type() == ExprType.DOUBLE ? ExprType.DOUBLE : ExprType.LONG;
+    return ExprEval.of(!ret.asBoolean(), retType);
   }
 
   @Override
@@ -458,19 +453,19 @@ class BinLtExpr extends BinaryEvalOpExprBase
   @Override
   protected ExprEval evalString(String left, String right)
   {
-    return ExprEval.of(left.compareTo(right) < 0 ? 1L : 0L);
+    return ExprEval.of(left.compareTo(right) < 0, ExprType.LONG);
   }
 
   @Override
   protected final long evalLong(long left, long right)
   {
-    return left < right ? 1L : 0L;
+    return Evals.asLong(left < right);
   }
 
   @Override
   protected final double evalDouble(double left, double right)
   {
-    return left < right ? 1.0d : 0.0d;
+    return Evals.asDouble(left < right);
   }
 }
 
@@ -484,19 +479,19 @@ class BinLeqExpr extends BinaryEvalOpExprBase
   @Override
   protected ExprEval evalString(String left, String right)
   {
-    return ExprEval.of(left.compareTo(right) <= 0 ? 1L : 0L);
+    return ExprEval.of(left.compareTo(right) <= 0, ExprType.LONG);
   }
 
   @Override
   protected final long evalLong(long left, long right)
   {
-    return left <= right ? 1L : 0L;
+    return Evals.asLong(left <= right);
   }
 
   @Override
   protected final double evalDouble(double left, double right)
   {
-    return left <= right ? 1.0d : 0.0d;
+    return Evals.asDouble(left <= right);
   }
 }
 
@@ -510,19 +505,19 @@ class BinGtExpr extends BinaryEvalOpExprBase
   @Override
   protected ExprEval evalString(String left, String right)
   {
-    return ExprEval.of(left.compareTo(right) > 0 ? 1L : 0L);
+    return ExprEval.of(left.compareTo(right) > 0, ExprType.LONG);
   }
 
   @Override
   protected final long evalLong(long left, long right)
   {
-    return left > right ? 1L : 0L;
+    return Evals.asLong(left > right);
   }
 
   @Override
   protected final double evalDouble(double left, double right)
   {
-    return left > right ? 1.0d : 0.0d;
+    return Evals.asDouble(left > right);
   }
 }
 
@@ -536,19 +531,19 @@ class BinGeqExpr extends BinaryEvalOpExprBase
   @Override
   protected ExprEval evalString(String left, String right)
   {
-    return ExprEval.of(left.compareTo(right) >= 0 ? 1L : 0L);
+    return ExprEval.of(left.compareTo(right) >= 0, ExprType.LONG);
   }
 
   @Override
   protected final long evalLong(long left, long right)
   {
-    return left >= right ? 1L : 0L;
+    return Evals.asLong(left >= right);
   }
 
   @Override
   protected final double evalDouble(double left, double right)
   {
-    return left >= right ? 1.0d : 0.0d;
+    return Evals.asDouble(left >= right);
   }
 }
 
@@ -562,19 +557,19 @@ class BinEqExpr extends BinaryEvalOpExprBase
   @Override
   protected ExprEval evalString(String left, String right)
   {
-    return ExprEval.of(left.equals(right) ? 1L : 0L);
+    return ExprEval.of(left.equals(right), ExprType.LONG);
   }
 
   @Override
   protected final long evalLong(long left, long right)
   {
-    return left == right ? 1L : 0L;
+    return Evals.asLong(left == right);
   }
 
   @Override
   protected final double evalDouble(double left, double right)
   {
-    return left == right ? 1.0d : 0.0d;
+    return Evals.asDouble(left == right);
   }
 }
 
@@ -588,19 +583,19 @@ class BinNeqExpr extends BinaryEvalOpExprBase
   @Override
   protected ExprEval evalString(String left, String right)
   {
-    return ExprEval.of(!Objects.equals(left, right) ? 1L : 0L);
+    return ExprEval.of(!left.equals(right), ExprType.LONG);
   }
 
   @Override
   protected final long evalLong(long left, long right)
   {
-    return left != right ? 1L : 0L;
+    return Evals.asLong(left != right);
   }
 
   @Override
   protected final double evalDouble(double left, double right)
   {
-    return left != right ? 1.0d : 0.0d;
+    return Evals.asDouble(left != right);
   }
 }
 

common/src/main/java/io/druid/math/expr/ExprEval.java

@@ -55,9 +55,9 @@ public abstract class ExprEval<T>
   {
     switch (type) {
       case DOUBLE:
-        return ExprEval.of(value ? 1D : 0D);
+        return ExprEval.of(Evals.asDouble(value));
       case LONG:
-        return ExprEval.of(value ? 1L : 0L);
+        return ExprEval.of(Evals.asLong(value));
       case STRING:
         return ExprEval.of(String.valueOf(value));
       default:
@@ -162,7 +162,7 @@ public abstract class ExprEval<T>
     @Override
     public final boolean asBoolean()
     {
-      return asDouble() > 0;
+      return Evals.asBoolean(asDouble());
     }
 
     @Override
@@ -202,7 +202,7 @@ public abstract class ExprEval<T>
     @Override
     public final boolean asBoolean()
     {
-      return asLong() > 0;
+      return Evals.asBoolean(asLong());
     }
 
     @Override
@@ -266,7 +266,7 @@ public abstract class ExprEval<T>
     @Override
     public final boolean asBoolean()
     {
-      return Boolean.valueOf(value);
+      return Evals.asBoolean(value);
     }
 
     @Override

common/src/test/java/io/druid/math/expr/EvalTest.java

@@ -143,4 +143,35 @@ public class EvalTest
     Assert.assertEquals("NULL", eval("nvl(if(x == 9223372036854775807, '', 'x'), 'NULL')", bindings).asString());
     Assert.assertEquals("x", eval("nvl(if(x == 9223372036854775806, '', 'x'), 'NULL')", bindings).asString());
   }
+
+  @Test
+  public void testBooleanReturn()
+  {
+    Expr.ObjectBinding bindings = Parser.withMap(
+        ImmutableMap.of("x", 100L, "y", 100L, "z", 100D, "w", 100D)
+    );
+    ExprEval eval = Parser.parse("x==y").eval(bindings);
+    Assert.assertTrue(eval.asBoolean());
+    Assert.assertEquals(ExprType.LONG, eval.type());
+
+    eval = Parser.parse("x!=y").eval(bindings);
+    Assert.assertFalse(eval.asBoolean());
+    Assert.assertEquals(ExprType.LONG, eval.type());
+
+    eval = Parser.parse("x==z").eval(bindings);
+    Assert.assertTrue(eval.asBoolean());
+    Assert.assertEquals(ExprType.DOUBLE, eval.type());
+
+    eval = Parser.parse("x!=z").eval(bindings);
+    Assert.assertFalse(eval.asBoolean());
+    Assert.assertEquals(ExprType.DOUBLE, eval.type());
+
+    eval = Parser.parse("z==w").eval(bindings);
+    Assert.assertTrue(eval.asBoolean());
+    Assert.assertEquals(ExprType.DOUBLE, eval.type());
+
+    eval = Parser.parse("z!=w").eval(bindings);
+    Assert.assertFalse(eval.asBoolean());
+    Assert.assertEquals(ExprType.DOUBLE, eval.type());
+  }
 }