[GitHub] [druid] clintropolis commented on a change in pull request #10605: bitwise math function expressions
clintropolis commented on a change in pull request #10605: URL: https://github.com/apache/druid/pull/10605#discussion_r560935104 ## 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: good catch :+1: fixed, and added tests 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] clintropolis commented on a change in pull request #10605: bitwise math function expressions
clintropolis commented on a change in pull request #10605: URL: https://github.com/apache/druid/pull/10605#discussion_r560934713 ## 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: heh no, when I split the functions i deleted the wrong half of the description .. and then wrote the one i meant to delete again a different way for the function i split out 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] clintropolis commented on a change in pull request #10605: bitwise math function expressions
clintropolis commented on a change in pull request #10605: URL: https://github.com/apache/druid/pull/10605#discussion_r556267351 ## 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: btw, testing `bitwiseConvertLongBitsToDouble(bitwiseConvertLongBitsToDouble(..))` uncovered an issue with the parser when trying to parse the output of `Expr.stringify` (because unit tests cover this round trip scenario, and when flatten is true it turns it into a constant), where large doubles with exponents, e.g. `1E10` or whatever, could not be correctly parsed, so I expanded the grammar to allow it [roughly according to these rules](https://docs.oracle.com/javase/8/docs/api/java/lang/Double.html#valueOf-java.lang.String-) 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] clintropolis commented on a change in pull request #10605: bitwise math function expressions
clintropolis commented on a change in pull request #10605:
URL: https://github.com/apache/druid/pull/10605#discussion_r553810003
##
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:
Trying to remember.. I think i was just trying to be consistent with the
other value conversion functions and put it here, but it probably could just
call it directly as it seems unlike we would change the function
##
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:
thinking back, I think i originally did this behavior before I added
`bitwiseConvertDouble`, so it was done as a way to do bitwise operations on
double values. After I added the explicit function, it isn't really necessary
anymore, so will revert to the behavior of casting and assuming long inputs.
##
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:
will split into `bitwiseConvertDoubleToLongBits` and
`bitwiseConvertLongBitsToDouble`
##
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:
more exhaustive coverage which should include these combinations is done
in `VectorExprSanityTest`, where non-vectorized and vectorized evaluation
results are asserted to be equal with a variety of combinations of inputs, but
I can add explicit tests here since those don't necessarily confirm
correctness, just self consistency between the two evaluation modes.
##
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:
>I'd think that `bitwiseConvertDouble(bitwiseConvertDouble(x))` would be
identical to `bitwiseConvertDouble(x)`.
hmm, should the conversion just pass through if the type is already the
output type or should it implicitly cast similar to the other bitwise functions?
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
[GitHub] [druid] clintropolis commented on a change in pull request #10605: bitwise math function expressions
clintropolis commented on a change in pull request #10605: URL: https://github.com/apache/druid/pull/10605#discussion_r554206473 ## 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: >I'd think that `bitwiseConvertDouble(bitwiseConvertDouble(x))` would be identical to `bitwiseConvertDouble(x)`. hmm, should the conversion just pass through if the type is already the output type or should it implicitly cast similar to the other bitwise functions? 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] clintropolis commented on a change in pull request #10605: bitwise math function expressions
clintropolis commented on a change in pull request #10605:
URL: https://github.com/apache/druid/pull/10605#discussion_r553811715
##
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:
more exhaustive coverage which should include these combinations is done
in `VectorExprSanityTest`, where non-vectorized and vectorized evaluation
results are asserted to be equal with a variety of combinations of inputs, but
I can add explicit tests here since those don't necessarily confirm
correctness, just self consistency between the two evaluation modes.
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] clintropolis commented on a change in pull request #10605: bitwise math function expressions
clintropolis commented on a change in pull request #10605: URL: https://github.com/apache/druid/pull/10605#discussion_r553810857 ## 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: will split into `bitwiseConvertDoubleToLongBits` and `bitwiseConvertLongBitsToDouble` 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] clintropolis commented on a change in pull request #10605: bitwise math function expressions
clintropolis commented on a change in pull request #10605: URL: https://github.com/apache/druid/pull/10605#discussion_r553810678 ## 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: thinking back, I think i originally did this behavior before I added `bitwiseConvertDouble`, so it was done as a way to do bitwise operations on double values. After I added the explicit function, it isn't really necessary anymore, so will revert to the behavior of casting and assuming long inputs. 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] clintropolis commented on a change in pull request #10605: bitwise math function expressions
clintropolis commented on a change in pull request #10605:
URL: https://github.com/apache/druid/pull/10605#discussion_r553810003
##
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:
Trying to remember.. I think i was just trying to be consistent with the
other value conversion functions and put it here, but it probably could just
call it directly as it seems unlike we would change the function
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
