You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/06/26 10:39:16 UTC
[GitHub] [druid] clintropolis opened a new pull request #10084: add bitwise and, or, negate expressions
clintropolis opened a new pull request #10084:
URL: https://github.com/apache/druid/pull/10084
### Description
This PR adds bitwise operators `&`, `|`, `~` to Druid native JSON query expressions.
`^` was already taken for power unfortunately, though I would be in favor of reworking it to a function so we can use `^` for xor and find an alternatively way to express the power operation.
SQL support appears to be not possible until https://issues.apache.org/jira/browse/CALCITE-3732 settles, so unfortunately only native JSON queries are supported at this time.
Related to #8560.
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added documentation for new or modified features or behaviors.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
<hr>
##### Key changed/added classes in this PR
* expression grammar file
* `Expr`
* `ExprListenerImpl`
----------------------------------------------------------------
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
[GitHub] [druid] suneet-s commented on a change in pull request #10084: add bitwise and, or, negate expressions
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10084:
URL: https://github.com/apache/druid/pull/10084#discussion_r446482630
##########
File path: core/src/test/java/org/apache/druid/math/expr/ParserTest.java
##########
@@ -172,6 +173,43 @@ public void testMixed()
validateFlatten("min(1, max(3, 4))", "(min [1, (max [3, 4])])", "1");
}
+ @Test
+ public void testBitwiseOps()
+ {
+ validateFlatten("3 & 1", "(& 3 1)", "1");
+ validateFlatten("3 & 1", "(& 3 1)", "1");
Review comment:
nit: duplicate test, did you intend to test the inverse of the previous line here?
```suggestion
validateFlatten("1 & 3", "(& 1 3)", "1");
```
##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -468,7 +468,7 @@ public void testGreatest()
{
// Same types
assertExpr("greatest(y, 0)", 2L);
- assertExpr("greatest(34.0, z, 5.0, 767.0", 767.0);
+ assertExpr("greatest(34.0, z, 5.0, 767.0)", 767.0);
Review comment:
👀 I'm surprised this didn't fail with a parse exception before
nice catch!
##########
File path: docs/misc/math-expr.md
##########
@@ -35,13 +35,15 @@ This expression language supports the following operators (listed in decreasing
|Operators|Description|
|---------|-----------|
-|!, -|Unary NOT and Minus|
+|!, -, ^|Unary NOT, Minus, and bitwise Negate|
Review comment:
```suggestion
|!, -, ~|Unary NOT, Minus, and bitwise Negate|
```
##########
File path: core/src/test/java/org/apache/druid/math/expr/ParserTest.java
##########
@@ -172,6 +173,43 @@ public void testMixed()
validateFlatten("min(1, max(3, 4))", "(min [1, (max [3, 4])])", "1");
}
+ @Test
+ public void testBitwiseOps()
+ {
+ validateFlatten("3 & 1", "(& 3 1)", "1");
+ validateFlatten("3 & 1", "(& 3 1)", "1");
+ validateFlatten("2 & 1", "(& 2 1)", "0");
+ validateFlatten("3 | 1", "(| 3 1)", "3");
+ validateFlatten("2 | 1", "(| 2 1)", "3");
+ validateFlatten("(~1) & 7", "(& ~1 7)", "6");
+
+ validateFlatten("'2' & '1'", "(& 2 1)", "0");
+ validateFlatten("'3' | '1'", "(| 3 1)", "3");
+ validateFlatten("(~'1') & 7", "(& ~1 7)", "6");
+
+ validateFlatten("'notanumber' & '1'", "(& notanumber 1)", null);
+ validateFlatten("'3' | 'notanumber'", "(| 3 notanumber)", null);
+ validateFlatten("~'notanumber'", "~notanumber", null);
+ validateFlatten("(~'notanumber') & '7'", "(& ~notanumber 7)", null);
+ validateFlatten("(~'notanumber') | '7'", "(| ~notanumber 7)", null);
Review comment:
could you add some tests with nulls for the bitwise operators
----------------------------------------------------------------
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
[GitHub] [druid] clintropolis closed pull request #10084: add bitwise and, or, negate expressions
Posted by GitBox <gi...@apache.org>.
clintropolis closed pull request #10084:
URL: https://github.com/apache/druid/pull/10084
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on pull request #10084: add bitwise and, or, negate expressions
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #10084:
URL: https://github.com/apache/druid/pull/10084#issuecomment-670274341
> SQL support appears to be not possible until https://issues.apache.org/jira/browse/CALCITE-3732 settles, so unfortunately only native JSON queries are supported at this time.
Fwiw, we don't need this to add support in SQL; we can add our own functions without Calcite being aware of them innately.
----------------------------------------------------------------
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
[GitHub] [druid] clintropolis commented on a change in pull request #10084: add bitwise and, or, negate expressions
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10084:
URL: https://github.com/apache/druid/pull/10084#discussion_r446485112
##########
File path: core/src/test/java/org/apache/druid/math/expr/ParserTest.java
##########
@@ -172,6 +173,43 @@ public void testMixed()
validateFlatten("min(1, max(3, 4))", "(min [1, (max [3, 4])])", "1");
}
+ @Test
+ public void testBitwiseOps()
+ {
+ validateFlatten("3 & 1", "(& 3 1)", "1");
+ validateFlatten("3 & 1", "(& 3 1)", "1");
Review comment:
Ah, it was just an accidental dupe I think
----------------------------------------------------------------
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
[GitHub] [druid] clintropolis commented on a change in pull request #10084: add bitwise and, or, negate expressions
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10084:
URL: https://github.com/apache/druid/pull/10084#discussion_r446486836
##########
File path: core/src/test/java/org/apache/druid/math/expr/ParserTest.java
##########
@@ -172,6 +173,43 @@ public void testMixed()
validateFlatten("min(1, max(3, 4))", "(min [1, (max [3, 4])])", "1");
}
+ @Test
+ public void testBitwiseOps()
+ {
+ validateFlatten("3 & 1", "(& 3 1)", "1");
+ validateFlatten("3 & 1", "(& 3 1)", "1");
+ validateFlatten("2 & 1", "(& 2 1)", "0");
+ validateFlatten("3 | 1", "(| 3 1)", "3");
+ validateFlatten("2 | 1", "(| 2 1)", "3");
+ validateFlatten("(~1) & 7", "(& ~1 7)", "6");
+
+ validateFlatten("'2' & '1'", "(& 2 1)", "0");
+ validateFlatten("'3' | '1'", "(| 3 1)", "3");
+ validateFlatten("(~'1') & 7", "(& ~1 7)", "6");
+
+ validateFlatten("'notanumber' & '1'", "(& notanumber 1)", null);
+ validateFlatten("'3' | 'notanumber'", "(| 3 notanumber)", null);
+ validateFlatten("~'notanumber'", "~notanumber", null);
+ validateFlatten("(~'notanumber') & '7'", "(& ~notanumber 7)", null);
+ validateFlatten("(~'notanumber') | '7'", "(| ~notanumber 7)", null);
Review comment:
Hmm, this actually brings up something worth thinking about actually, and I'm not exactly certain what the most correct behavior is to have here.
A test (at least here) would use a `null` constant, which `BinaryEvalOpExprBase` currently is _not_ short circuited to return `null` unless `NullHandling.sqlCompatible()` is set.
The "default" evaluator in `BinaryEvalOpExprBase` is `evalDoubles` if none of the other conditions match, and the other evaluators (`evalString` and `evalLong`) both require left and right expressions to be the same type, which the `null` constant makes not be true unless the other expression is `ExprType.STRING`.
This means tests using a `null` constant and a number on the other side end up in `evalDoubles`, which for these bitwise expressions will
```
throw new IllegalArgumentException("unsupported type " + ExprType.DOUBLE);
```
which seems sort of funny, and not really correct.
I'm not really certain that `evalDouble` being the default makes sense, but even if it does, i'm not certain what the correct behavior in default mode if a null constant is involved (there would never be a null number from a column in default mode).
I think it really comes down to what should be the behavior in default mode of the `null` constant when used in a number expression. The `'notanumber'` string case evaluates to null eventually, even in default mode, while `null` does not, which seems inconsistent.
##########
File path: docs/misc/math-expr.md
##########
@@ -35,13 +35,15 @@ This expression language supports the following operators (listed in decreasing
|Operators|Description|
|---------|-----------|
-|!, -|Unary NOT and Minus|
+|!, -, ^|Unary NOT, Minus, and bitwise Negate|
Review comment:
oops, thanks :+1:
----------------------------------------------------------------
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
[GitHub] [druid] stale[bot] commented on pull request #10084: add bitwise and, or, negate expressions
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10084:
URL: https://github.com/apache/druid/pull/10084#issuecomment-707626913
This issue is no longer marked as stale.
----------------------------------------------------------------
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
[GitHub] [druid] stale[bot] commented on pull request #10084: add bitwise and, or, negate expressions
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10084:
URL: https://github.com/apache/druid/pull/10084#issuecomment-706786010
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.
----------------------------------------------------------------
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
[GitHub] [druid] teyeheimans commented on pull request #10084: add bitwise and, or, negate expressions
Posted by GitBox <gi...@apache.org>.
teyeheimans commented on pull request #10084:
URL: https://github.com/apache/druid/pull/10084#issuecomment-707626890
I would love to see this functionality in druid. Any chance this could be fixed/merged?
----------------------------------------------------------------
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
[GitHub] [druid] clintropolis commented on pull request #10084: add bitwise and, or, negate expressions
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #10084:
URL: https://github.com/apache/druid/pull/10084#issuecomment-734090073
closing in favor of #10605
----------------------------------------------------------------
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
[GitHub] [druid] clintropolis commented on a change in pull request #10084: add bitwise and, or, negate expressions
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10084:
URL: https://github.com/apache/druid/pull/10084#discussion_r446485042
##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -468,7 +468,7 @@ public void testGreatest()
{
// Same types
assertExpr("greatest(y, 0)", 2L);
- assertExpr("greatest(34.0, z, 5.0, 767.0", 767.0);
+ assertExpr("greatest(34.0, z, 5.0, 767.0)", 767.0);
Review comment:
it just writes parse error stuff to stderr but still ends up as a valid expression which is how the test passes I guess
```
line 1:28 missing ')' at '<EOF>'
line 1:28 missing ')' at '<EOF>'
```
----------------------------------------------------------------
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
[GitHub] [druid] clintropolis commented on a change in pull request #10084: add bitwise and, or, negate expressions
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10084:
URL: https://github.com/apache/druid/pull/10084#discussion_r446487510
##########
File path: core/src/test/java/org/apache/druid/math/expr/ParserTest.java
##########
@@ -172,6 +173,43 @@ public void testMixed()
validateFlatten("min(1, max(3, 4))", "(min [1, (max [3, 4])])", "1");
}
+ @Test
+ public void testBitwiseOps()
+ {
+ validateFlatten("3 & 1", "(& 3 1)", "1");
+ validateFlatten("3 & 1", "(& 3 1)", "1");
+ validateFlatten("2 & 1", "(& 2 1)", "0");
+ validateFlatten("3 | 1", "(| 3 1)", "3");
+ validateFlatten("2 | 1", "(| 2 1)", "3");
+ validateFlatten("(~1) & 7", "(& ~1 7)", "6");
+
+ validateFlatten("'2' & '1'", "(& 2 1)", "0");
+ validateFlatten("'3' | '1'", "(| 3 1)", "3");
+ validateFlatten("(~'1') & 7", "(& ~1 7)", "6");
+
+ validateFlatten("'notanumber' & '1'", "(& notanumber 1)", null);
+ validateFlatten("'3' | 'notanumber'", "(| 3 notanumber)", null);
+ validateFlatten("~'notanumber'", "~notanumber", null);
+ validateFlatten("(~'notanumber') & '7'", "(& ~notanumber 7)", null);
+ validateFlatten("(~'notanumber') | '7'", "(| ~notanumber 7)", null);
Review comment:
```'notanumber' & 1``` actually has the same problem of ending up as `evalDouble` and exploding, so it's not _strictly_ the null constant, but `'notanumber'` ends up as a `null` so it's the same-ish problem.
----------------------------------------------------------------
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
[GitHub] [druid] gianm commented on a change in pull request #10084: add bitwise and, or, negate expressions
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10084:
URL: https://github.com/apache/druid/pull/10084#discussion_r447331582
##########
File path: core/src/test/java/org/apache/druid/math/expr/ParserTest.java
##########
@@ -172,6 +173,43 @@ public void testMixed()
validateFlatten("min(1, max(3, 4))", "(min [1, (max [3, 4])])", "1");
}
+ @Test
+ public void testBitwiseOps()
+ {
+ validateFlatten("3 & 1", "(& 3 1)", "1");
+ validateFlatten("3 & 1", "(& 3 1)", "1");
+ validateFlatten("2 & 1", "(& 2 1)", "0");
+ validateFlatten("3 | 1", "(| 3 1)", "3");
+ validateFlatten("2 | 1", "(| 2 1)", "3");
+ validateFlatten("(~1) & 7", "(& ~1 7)", "6");
+
+ validateFlatten("'2' & '1'", "(& 2 1)", "0");
+ validateFlatten("'3' | '1'", "(| 3 1)", "3");
+ validateFlatten("(~'1') & 7", "(& ~1 7)", "6");
+
+ validateFlatten("'notanumber' & '1'", "(& notanumber 1)", null);
+ validateFlatten("'3' | 'notanumber'", "(| 3 notanumber)", null);
+ validateFlatten("~'notanumber'", "~notanumber", null);
+ validateFlatten("(~'notanumber') & '7'", "(& ~notanumber 7)", null);
+ validateFlatten("(~'notanumber') | '7'", "(| ~notanumber 7)", null);
Review comment:
Maybe `BinaryEvalOpExprBase` should have a special case for `null` literals?
----------------------------------------------------------------
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