[GitHub] [druid] gianm commented on a change in pull request #10605: bitwise math function expressions
gianm commented on a change in pull request #10605: URL: https://github.com/apache/druid/pull/10605#discussion_r561147445 ## File path: docs/misc/math-expr.md ## @@ -115,53 +115,53 @@ See javadoc of java.lang.Math for detailed explanation for each function. |name|description| ||---| -|abs|abs(x) would return the absolute value of x| -|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 implicitly cast to longs, use `bitwiseConvertDoubleToLongBits` to perform bitwise operations directly with doubles| -|bitwiseComplement|bitwiseComplement(x) would return the result of ~x. Double values will be implicitly cast to longs, use `bitwiseConvertDoubleToLongBits` to perform bitwise operations directly with doubles| -|bitwiseConvertDoubleToLongBits|bitwiseConvertDoubleToLongBits(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 implicitly cast the value to a long if the input is a double| -|bitwiseConvertLongBitsToDouble|bitwiseConvertLongBitsToDouble(x) would convert a long to the IEEE 754 floating-point "double" specified by the bits stored in the long. A double input will be implicitly cast to a long| -|bitwiseOr|bitwiseOr(x,y) would return the result of x [PIPE] y. Double values will be implicitly cast to longs, use `bitwiseConvertDoubleToLongBits` to perform bitwise operations directly with doubles| -|bitwiseShiftLeft|bitwiseShiftLeft(x,y) would return the result of x << y. Double values will be implicitly cast to longs, use `bitwiseConvertDoubleToLongBits` to perform bitwise operations directly with doubles| -|bitwiseShiftRight|bitwiseShiftRight(x,y) would return the result of x >> y. Double values will be implicitly cast to longs, use `bitwiseConvertDoubleToLongBits` to perform bitwise operations directly with doubles| -|bitwiseXor|bitwiseXor(x,y) would return the result of x ^ y. Double values will be implicitly cast to longs, use `bitwiseConvertDoubleToLongBits` to perform bitwise operations directly with doubles| -|atan2|atan2(y, x) would return the angle theta from the conversion of rectangular coordinates (x, y) to polar * coordinates (r, theta)| -|cbrt|cbrt(x) would return the cube root of x| -|ceil|ceil(x) would return the smallest (closest to negative infinity) double value that is greater than or equal to x and is equal to a mathematical integer| -|copysign|copysign(x) would return the first floating-point argument with the sign of the second floating-point argument| -|cos|cos(x) would return the trigonometric cosine of x| -|cosh|cosh(x) would return the hyperbolic cosine of x| -|cot|cot(x) would return the trigonometric cotangent of an angle x| +|abs|abs(x) returns the absolute value of x| +|acos|acos(x) returns the arc cosine of x| +|asin|asin(x) returns the arc sine of x| +|atan|atan(x) returns the arc tangent of x| +|bitwiseAnd|bitwiseAnd(x,y) returns the result of x & y. Double values will be implicitly cast to longs, use `bitwiseConvertDoubleToLongBits` to perform bitwise operations directly with doubles| +|bitwiseComplement|bitwiseComplement(x) returns the result of ~x. Double values will be implicitly cast to longs, use `bitwiseConvertDoubleToLongBits` to perform bitwise operations directly with doubles| +|bitwiseConvertDoubleToLongBits|bitwiseConvertDoubleToLongBits(x) will convert the IEEE 754 floating-point "double" bits of a double value into a long, or implicitly cast the value to a double if the input is a long.| Review comment: This still doesn't seem right; surely, if the input is a long, the function will cast the value to a double and _then_ convert those double bits back to a long. The description doesn't make it sound like that's what happens. 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: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10605: bitwise math function expressions
gianm commented on a change in pull request #10605: URL: https://github.com/apache/druid/pull/10605#discussion_r558098809 ## File path: core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 ## @@ -51,9 +51,12 @@ numericElement : (LONG | DOUBLE | NULL); literalElement : (STRING | LONG | DOUBLE | NULL); NULL : 'null'; +LONG : [0-9]+; +EXP: [eE] [-]? LONG; +// DOUBLE provides partial support for java double format +// see: https://docs.oracle.com/javase/8/docs/api/java/lang/Double.html#valueOf-java.lang.String- +DOUBLE : 'NaN' | 'Infinity' | (LONG '.' LONG) | (LONG EXP) | (LONG '.' LONG EXP); Review comment: This used to allow `10.` as a double, but now it doesn't. I think we should add that back. (with tests ) ## File path: docs/misc/math-expr.md ## @@ -119,13 +119,14 @@ 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.| -|bitwiseOr|bitwiseOr(x,y) would return the result of x [PIPE] y. Double values will be converted to their bit representation | -|bitwiseShiftLeft|bitwiseShiftLeft(x,y) would return the result of x << y. Double values will be converted to their bit representation| -|bitwiseShiftRight|bitwiseShiftRight(x,y) would return the result of x >> y. Double values will be converted to their bit representation| -|bitwiseXor|bitwiseXor(x,y) would return the result of x ^ y. Double values will be converted to their bit representation| +|bitwiseAnd|bitwiseAnd(x,y) would return the result of x & y. Double values will be implicitly cast to longs, use `bitwiseConvertDoubleToLongBits` to perform bitwise operations directly with doubles| +|bitwiseComplement|bitwiseComplement(x) would return the result of ~x. Double values will be implicitly cast to longs, use `bitwiseConvertDoubleToLongBits` to perform bitwise operations directly with doubles| +|bitwiseConvertDoubleToLongBits|bitwiseConvertDoubleToLongBits(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 implicitly cast the value to a long if the input is a double| Review comment: Are these docs for `bitwiseConvertDoubleToLongBits` correct? It doesn't sound like something that the function should do. 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: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10605: bitwise math function expressions
gianm commented on a change in pull request #10605:
URL: https://github.com/apache/druid/pull/10605#discussion_r554224182
##
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:
It should implicitly cast, I think.
Generally I think function behavior is easier to understand if the function
implicitly casts its inputs to the type that it expects, vs. changing behavior
based on its input type.
##
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:
I see, I missed that test. Sounds good.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10605: bitwise math function expressions
gianm commented on a change in pull request #10605:
URL: https://github.com/apache/druid/pull/10605#discussion_r554224361
##
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:
I see, I missed that test. Sounds good.
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10605: bitwise math function expressions
gianm commented on a change in pull request #10605: URL: https://github.com/apache/druid/pull/10605#discussion_r554224182 ## 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: It should implicitly cast, I think. Generally I think function behavior is easier to understand if the function implicitly casts its inputs to the type that it expects, vs. changing behavior based on its input type. 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: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10605: bitwise math function expressions
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
