[GitHub] [druid] gianm commented on a change in pull request #10605: bitwise math function expressions

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

gianm commented on a change in pull request #10605:
URL: https://github.com/apache/druid/pull/10605#discussion_r553775571



##########
File path: core/src/main/java/org/apache/druid/math/expr/Evals.java
##########
@@ -71,4 +71,14 @@ public static boolean asBoolean(@Nullable String x)
   {
     return !NullHandling.isNullOrEquivalent(x) && Boolean.parseBoolean(x);
   }
+
+  public static long doubleToLongBits(double x)
+  {
+    return Double.doubleToLongBits(x);

Review comment:
       Why add this instead of calling the thing in `Double`?

##########
File path: docs/misc/math-expr.md
##########
@@ -119,6 +119,13 @@ See javadoc of java.lang.Math for detailed explanation for each function.
 |acos|acos(x) would return the arc cosine of x|
 |asin|asin(x) would return the arc sine of x|
 |atan|atan(x) would return the arc tangent of x|
+|bitwiseAnd|bitwiseAnd(x,y) would return the result of x & y. Double values will be converted to their bit representation|

Review comment:
       Why convert doubles to their bit representations instead of casting them to longs? Casting to long would, I think, make more sense since we can think of it as an implicit cast of double-typed arguments to a function that only accepts longs.

##########
File path: docs/misc/math-expr.md
##########
@@ -119,6 +119,13 @@ See javadoc of java.lang.Math for detailed explanation for each function.
 |acos|acos(x) would return the arc cosine of x|
 |asin|asin(x) would return the arc sine of x|
 |atan|atan(x) would return the arc tangent of x|
+|bitwiseAnd|bitwiseAnd(x,y) would return the result of x & y. Double values will be converted to their bit representation|
+|bitwiseComplement|bitwiseComplement(x) would return the result of ~x. Double values will be converted to their bit representation|
+|bitwiseConvertDouble|bitwiseConvertDouble(x) would convert the IEEE 754 floating-point "double" bits stored in a long into a double value if the input is a long, or the copy bits of a double value into a long if the input is a double.|

Review comment:
       This function is kind of weird because it doesn't have a fixpoint. I'd think that `bitwiseConvertDouble(bitwiseConvertDouble(x))` would be identical to `bitwiseConvertDouble(x)`. The lack of fixpoint makes it hard to reason about what the result of this function is going to be. Is there a specific reason it's designed this way? If not, I'd suggest splitting into two functions for each direction of the conversion.

##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -519,6 +519,31 @@ public void testLeast()
     assertExpr("least(1, null, 'A')", "1");
   }
 
+  @Test
+  public void testBitwise()
+  {
+    assertExpr("bitwiseAnd(3, 1)", 1L);
+    assertExpr("bitwiseAnd(2, 1)", 0L);
+    assertExpr("bitwiseOr(3, 1)", 3L);
+    assertExpr("bitwiseOr(2, 1)", 3L);
+    assertExpr("bitwiseXor(3, 1)", 2L);
+    assertExpr("bitwiseXor(2, 1)", 3L);
+    assertExpr("bitwiseShiftLeft(2, 1)", 4L);
+    assertExpr("bitwiseShiftRight(2, 1)", 1L);
+    assertExpr("bitwiseAnd(bitwiseComplement(1), 7)", 6L);
+    assertExpr("bitwiseAnd('2', '1')", null);
+    assertExpr("bitwiseAnd(2, '1')", 0L);
+
+    assertExpr("bitwiseOr(2.345, 1)", 4612462889363109315L);

Review comment:
       Please include (double, double) and (long, double) in addition to (double, long) args.




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