Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@
grammar Expr;

expr : NULL # null
| ('-'|'!') expr # unaryOpExpr
| ('-'|'!'|'~') expr # unaryOpExpr
|<assoc=right> expr '^' expr # powOpExpr
| expr ('&'|'|') expr # bitwiseOpExpr
| expr ('*'|'/'|'%') expr # mulDivModuloExpr
| expr ('+'|'-') expr # addSubExpr
| expr ('<'|'<='|'>'|'>='|'=='|'!=') expr # logicalOpExpr
Expand Down Expand Up @@ -76,3 +77,6 @@ EQ : '==' ;
NEQ : '!=' ;
AND : '&&' ;
OR : '||' ;
BITAND : '&' ;
BITOR : '|' ;
BITNEG : '~' ;
109 changes: 109 additions & 0 deletions core/src/main/java/org/apache/druid/math/expr/Expr.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.apache.commons.lang.StringEscapeUtils;
import org.apache.druid.annotations.SubclassesMustOverrideEqualsAndHashCode;
import org.apache.druid.common.config.NullHandling;
import org.apache.druid.common.guava.GuavaUtils;
import org.apache.druid.java.util.common.IAE;
import org.apache.druid.java.util.common.ISE;
import org.apache.druid.java.util.common.StringUtils;
Expand Down Expand Up @@ -1441,6 +1442,42 @@ public String toString()
}
}

class UnaryBitwiseNegateExpr extends UnaryExpr
{
UnaryBitwiseNegateExpr(Expr expr)
{
super(expr);
}

@Override
UnaryExpr copy(Expr expr)
{
return new UnaryBitwiseNegateExpr(expr);
}

@Override
public ExprEval eval(ObjectBinding bindings)
{
ExprEval ret = expr.eval(bindings);
if ((NullHandling.sqlCompatible() && (ret.value() == null)) || ret.isNumericNull()) {
return ExprEval.of(null);
}
return ExprEval.of(~ret.asLong());
}

@Override
public String stringify()
{
return StringUtils.format("~%s", expr.stringify());
}

@Override
public String toString()
{
return StringUtils.format("~%s", expr);
}
}

/**
* Base type for all binary operators, this {@link Expr} has two children {@link Expr} for the left and right side
* operands.
Expand Down Expand Up @@ -1971,6 +2008,78 @@ public ExprEval eval(ObjectBinding bindings)
ExprEval leftVal = left.eval(bindings);
return leftVal.asBoolean() ? leftVal : right.eval(bindings);
}
}

class BinBitwiseAndExpr extends BinaryEvalOpExprBase
{
BinBitwiseAndExpr(String op, Expr left, Expr right)
{
super(op, left, right);
}

@Override
protected BinaryOpExprBase copy(Expr left, Expr right)
{
return new BinBitwiseAndExpr(op, left, right);
}

@Override
protected final long evalLong(long left, long right)
{
return left & right;
}

@Override
protected final double evalDouble(double left, double right)
{
throw new IllegalArgumentException("unsupported type " + ExprType.DOUBLE);
}

@Override
protected ExprEval evalString(@Nullable String left, @Nullable String right)
{
Long l1 = GuavaUtils.tryParseLong(left);
Long l2 = GuavaUtils.tryParseLong(right);
if (l1 == null || l2 == null) {
return ExprEval.of(null);
}
return ExprEval.of(l1 & l2);
}
}

class BinBitwiseOrExpr extends BinaryEvalOpExprBase
{
BinBitwiseOrExpr(String op, Expr left, Expr right)
{
super(op, left, right);
}

@Override
protected BinaryOpExprBase copy(Expr left, Expr right)
{
return new BinBitwiseOrExpr(op, left, right);
}

@Override
protected final long evalLong(long left, long right)
{
return left | right;
}

@Override
protected final double evalDouble(double left, double right)
{
throw new IllegalArgumentException("unsupported type " + ExprType.DOUBLE);
}

@Override
protected ExprEval evalString(@Nullable String left, @Nullable String right)
{
Long l1 = GuavaUtils.tryParseLong(left);
Long l2 = GuavaUtils.tryParseLong(right);
if (l1 == null || l2 == null) {
return ExprEval.of(null);
}
return ExprEval.of(l1 | l2);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ public void exitUnaryOpExpr(ExprParser.UnaryOpExprContext ctx)
case ExprParser.NOT:
nodes.put(ctx, new UnaryNotExpr((Expr) nodes.get(ctx.getChild(1))));
break;
case ExprParser.BITNEG:
nodes.put(ctx, new UnaryBitwiseNegateExpr((Expr) nodes.get(ctx.getChild(1))));
break;
default:
throw new RE("Unrecognized unary operator %s", ctx.getChild(0).getText());
}
Expand Down Expand Up @@ -301,6 +304,36 @@ public void exitMulDivModuloExpr(ExprParser.MulDivModuloExprContext ctx)
}
}

@Override
public void exitBitwiseOpExpr(ExprParser.BitwiseOpExprContext ctx)
{
int opCode = ((TerminalNode) ctx.getChild(1)).getSymbol().getType();
switch (opCode) {
case ExprParser.BITAND:
nodes.put(
ctx,
new BinBitwiseAndExpr(
ctx.getChild(1).getText(),
(Expr) nodes.get(ctx.getChild(0)),
(Expr) nodes.get(ctx.getChild(2))
)
);
break;
case ExprParser.BITOR:
nodes.put(
ctx,
new BinBitwiseOrExpr(
ctx.getChild(1).getText(),
(Expr) nodes.get(ctx.getChild(0)),
(Expr) nodes.get(ctx.getChild(2))
)
);
break;
default:
throw new RE("Unrecognized bitwise operator %s", ctx.getChild(1).getText());
}
}

@Override
public void exitPowOpExpr(ExprParser.PowOpExprContext ctx)
{
Expand Down
8 changes: 8 additions & 0 deletions core/src/test/java/org/apache/druid/math/expr/ExprTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,14 @@ public void testEqualsContractForBinAndExpr()
EqualsVerifier.forClass(BinAndExpr.class).usingGetClass().verify();
}

@Test
public void testEqualsContractForBitwiseExpr()
{
EqualsVerifier.forClass(BinBitwiseAndExpr.class).usingGetClass().verify();
EqualsVerifier.forClass(BinBitwiseOrExpr.class).usingGetClass().verify();
EqualsVerifier.forClass(UnaryBitwiseNegateExpr.class).usingGetClass().verify();
}

@Test
public void testEqualsContractForFunctionExpr()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 I'm surprised this didn't fail with a parse exception before

nice catch!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assertExpr("greatest('B', x, 'A')", "foo");

// Different types
Expand Down Expand Up @@ -496,7 +496,7 @@ public void testLeast()
{
// Same types
assertExpr("least(y, 0)", 0L);
assertExpr("least(34.0, z, 5.0, 767.0", 3.1);
assertExpr("least(34.0, z, 5.0, 767.0)", 3.1);
assertExpr("least('B', x, 'A')", "A");

// Different types
Expand Down
40 changes: 39 additions & 1 deletion core/src/test/java/org/apache/druid/math/expr/ParserTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.junit.Test;
import org.junit.rules.ExpectedException;

import javax.annotation.Nullable;
import java.util.Collections;
import java.util.List;
import java.util.Set;
Expand Down Expand Up @@ -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");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: duplicate test, did you intend to test the inverse of the previous line here?

Suggested change
validateFlatten("3 & 1", "(& 3 1)", "1");
validateFlatten("1 & 3", "(& 1 3)", "1");

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it was just an accidental dupe I think

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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add some tests with nulls for the bitwise operators

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe BinaryEvalOpExprBase should have a special case for null literals?

}

@Test
public void testBitwiseDoubleAndExplosion()
{
expectedException.expect(IllegalArgumentException.class);
expectedException.expectMessage("DOUBLE");
validateFlatten("3.0 & 1", "(& 3.0 1)", "1");
}

@Test
public void testBitwiseDoubleOrExplosion()
{
expectedException.expect(IllegalArgumentException.class);
expectedException.expectMessage("DOUBLE");
validateFlatten("3.0 | 1", "(| 3.0 1)", "1");
}

@Test
public void testIdentifiers()
{
Expand Down Expand Up @@ -543,7 +581,7 @@ public void testUniquify()
}


private void validateFlatten(String expression, String withoutFlatten, String withFlatten)
private void validateFlatten(String expression, String withoutFlatten, @Nullable String withFlatten)
{
Expr notFlat = Parser.parse(expression, ExprMacroTable.nil(), false);
Expr flat = Parser.parse(expression, ExprMacroTable.nil(), true);
Expand Down
4 changes: 3 additions & 1 deletion docs/misc/math-expr.md
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
|!, -, ^|Unary NOT, Minus, and bitwise Negate|
|!, -, ~|Unary NOT, Minus, and bitwise Negate|

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, thanks 👍

|^|Binary power op|
|&, &#124;|Binary bitwise AND, OR|
|*, /, %|Binary multiplicative|
|+, -|Binary additive|
|<, <=, >, >=, ==, !=|Binary Comparison|
|&&, &#124;&#124;|Binary Logical AND, OR|


Long, double, and string data types are supported. If a number contains a dot, it is interpreted as a double, otherwise it is interpreted as a long. That means, always add a '.' to your number if you want it interpreted as a double value. String literals should be quoted by single quotation marks.

Additionally, the expression language supports long, double, and string arrays. Array literals are created by wrapping square brackets around a list of scalar literals values delimited by a comma or space character. All values in an array literal must be the same type, however null values are accepted. Typed empty arrays may be defined by prefixing with their type in angle brackets: `<STRING>[]`, `<DOUBLE>[]`, or `<LONG>[]`.
Expand Down
1 change: 1 addition & 0 deletions website/.spelling
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ backfills
backpressure
base64
big-endian
bitwise
blobstore
boolean
breakpoint
Expand Down