add bitwise and, or, negate expressions#10084
add bitwise and, or, negate expressions#10084clintropolis wants to merge 4 commits intoapache:masterfrom
Conversation
suneet-s
left a comment
suneet-s
left a comment
There was a problem hiding this comment.
Looks cool 🤘
1 small doc typo and some null tests, then LGTM
| // 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); |
There was a problem hiding this comment.
👀 I'm surprised this didn't fail with a parse exception before
nice catch!
There was a problem hiding this 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>'
| public void testBitwiseOps() | ||
| { | ||
| validateFlatten("3 & 1", "(& 3 1)", "1"); | ||
| validateFlatten("3 & 1", "(& 3 1)", "1"); |
There was a problem hiding this comment.
nit: duplicate test, did you intend to test the inverse of the previous line here?
| validateFlatten("3 & 1", "(& 3 1)", "1"); | |
| validateFlatten("1 & 3", "(& 1 3)", "1"); |
There was a problem hiding this comment.
Ah, it was just an accidental dupe I think
| |Operators|Description| | ||
| |---------|-----------| | ||
| |!, -|Unary NOT and Minus| | ||
| |!, -, ^|Unary NOT, Minus, and bitwise Negate| |
There was a problem hiding this comment.
| |!, -, ^|Unary NOT, Minus, and bitwise Negate| | |
| |!, -, ~|Unary NOT, Minus, and bitwise Negate| |
| validateFlatten("'3' | 'notanumber'", "(| 3 notanumber)", null); | ||
| validateFlatten("~'notanumber'", "~notanumber", null); | ||
| validateFlatten("(~'notanumber') & '7'", "(& ~notanumber 7)", null); | ||
| validateFlatten("(~'notanumber') | '7'", "(| ~notanumber 7)", null); |
There was a problem hiding this comment.
could you add some tests with nulls for the bitwise operators
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
Maybe BinaryEvalOpExprBase should have a special case for null literals?
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 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. |
|
I would love to see this functionality in druid. Any chance this could be fixed/merged? |
|
This issue is no longer marked as stale. |
|
closing in favor of #10605 |
4 participants
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.
This PR has:
Key changed/added classes in this PR
ExprExprListenerImpl