From bbc5800ea8be5726e06ba29f3e6bc691289e953c Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Sat, 15 Feb 2020 02:48:45 -0800 Subject: [PATCH 01/10] add Expr.stringify which produces parseable expression strings, parser support for null values in arrays, and parser support for empty numeric arrays --- .../org/apache/druid/math/expr/antlr/Expr.g4 | 15 ++- .../java/org/apache/druid/math/expr/Expr.java | 108 +++++++++++++++++- .../druid/math/expr/ExprListenerImpl.java | 41 ++++++- .../druid/math/expr/ApplyFunctionTest.java | 46 +++++++- .../apache/druid/math/expr/FunctionTest.java | 25 +++- .../apache/druid/math/expr/ParserTest.java | 65 ++++++++++- docs/misc/math-expr.md | 28 +---- 7 files changed, 289 insertions(+), 39 deletions(-) diff --git a/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 b/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 index aacbbe9d4290..10a9f6f9989f 100644 --- a/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 +++ b/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 @@ -29,10 +29,13 @@ expr : 'null' # null | DOUBLE # doubleExpr | LONG # longExpr | STRING # string - | '[' DOUBLE (',' DOUBLE)* ']' # doubleArray - | '[' LONG (',' LONG)* ']' # longArray - | '[' STRING (',' STRING)* ']' # stringArray + | '[' doubleElement (',' doubleElement)* ']' # doubleArray + | '[' longElement (',' longElement)* ']' # longArray + | '[' stringElement (',' stringElement)* ']' # stringArray | '[]' # emptyArray + | '[]' # emptyStringArray + | '[]' # emptyDoubleArray + | '[]' # emptyLongArray ; lambda : (IDENTIFIER | '(' ')' | '(' IDENTIFIER (',' IDENTIFIER)* ')') '->' expr @@ -41,6 +44,12 @@ lambda : (IDENTIFIER | '(' ')' | '(' IDENTIFIER (',' IDENTIFIER)* ')') '->' expr fnArgs : expr (',' expr)* # functionArgs ; +longElement : (LONG | 'null'); + +doubleElement : (DOUBLE | 'null'); + +stringElement : (STRING | 'null'); + IDENTIFIER : [_$a-zA-Z][_$a-zA-Z0-9]* | '"' (ESC | ~ [\"\\])* '"'; LONG : [0-9]+ ; DOUBLE : [0-9]+ '.' [0-9]* ; diff --git a/core/src/main/java/org/apache/druid/math/expr/Expr.java b/core/src/main/java/org/apache/druid/math/expr/Expr.java index ea7838294064..dbbf6d4ec52e 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Expr.java +++ b/core/src/main/java/org/apache/druid/math/expr/Expr.java @@ -19,12 +19,14 @@ package org.apache.druid.math.expr; +import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.common.math.LongMath; import com.google.common.primitives.Ints; +import org.apache.commons.lang.StringEscapeUtils; import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.ISE; @@ -46,6 +48,8 @@ */ public interface Expr { + String NULL_LITERAL = "null"; + Joiner ARG_JOINER = Joiner.on(", "); /** * Indicates expression is a constant whose literal value can be extracted by {@link Expr#getLiteralValue()}, * making evaluating with arguments and bindings unecessary @@ -106,6 +110,12 @@ default String getBindingIfIdentifier() */ ExprEval eval(ObjectBinding bindings); + /** + * Convert the {@link Expr} back into parseable string that when parsed with + * {@link Parser#parse(String, ExprMacroTable)} will produce an equivalent {@link Expr}. + */ + String stringify(); + /** * Programmatically inspect the {@link Expr} tree with a {@link Visitor}. Each {@link Expr} is responsible for * ensuring the {@link Visitor} can visit all of its {@link Expr} children before visiting itself @@ -460,6 +470,12 @@ public BindingDetails analyzeInputs() { return new BindingDetails(); } + + @Override + public String stringify() + { + return toString(); + } } abstract class NullNumericConstantExpr extends ConstantExpr @@ -473,7 +489,7 @@ public Object getLiteralValue() @Override public String toString() { - return "null"; + return NULL_LITERAL; } } @@ -541,6 +557,15 @@ public ExprEval eval(ObjectBinding bindings) { return ExprEval.ofLongArray(value); } + + @Override + public String stringify() + { + if (value.length == 0) { + return "[]"; + } + return toString(); + } } class StringExpr extends ConstantExpr @@ -571,6 +596,12 @@ public ExprEval eval(ObjectBinding bindings) { return ExprEval.of(value); } + + @Override + public String stringify() + { + return value == null ? NULL_LITERAL : StringUtils.format("'%s'", StringEscapeUtils.escapeJavaScript(value)); + } } class StringArrayExpr extends ConstantExpr @@ -599,6 +630,25 @@ public ExprEval eval(ObjectBinding bindings) { return ExprEval.ofStringArray(value); } + + @Override + public String stringify() + { + if (value.length == 0) { + return "[]"; + } + return StringUtils.format( + "[%s]", + ARG_JOINER.join( + Arrays.stream(value) + .map(s -> s == null + ? NULL_LITERAL + : StringUtils.format("'%s'", StringEscapeUtils.escapeJavaScript(s)) + ) + .iterator() + ) + ); + } } class DoubleExpr extends ConstantExpr @@ -664,6 +714,15 @@ public ExprEval eval(ObjectBinding bindings) { return ExprEval.ofDoubleArray(value); } + + @Override + public String stringify() + { + if (value.length == 0) { + return "[]"; + } + return toString(); + } } /** @@ -754,6 +813,12 @@ public ExprEval eval(ObjectBinding bindings) return ExprEval.bestEffortOf(bindings.get(binding)); } + @Override + public String stringify() + { + return StringUtils.format("\"%s\"", StringEscapeUtils.escapeJava(binding)); + } + @Override public void visit(Visitor visitor) { @@ -820,6 +885,12 @@ public ExprEval eval(ObjectBinding bindings) return expr.eval(bindings); } + @Override + public String stringify() + { + return StringUtils.format("(%s) -> %s", ARG_JOINER.join(getIdentifiers()), expr.stringify()); + } + @Override public void visit(Visitor visitor) { @@ -876,6 +947,12 @@ public ExprEval eval(ObjectBinding bindings) return function.apply(args, bindings); } + @Override + public String stringify() + { + return StringUtils.format("%s(%s)", name, ARG_JOINER.join(args.stream().map(Expr::stringify).iterator())); + } + @Override public void visit(Visitor visitor) { @@ -962,6 +1039,17 @@ public ExprEval eval(ObjectBinding bindings) return function.apply(lambdaExpr, argsExpr, bindings); } + @Override + public String stringify() + { + return StringUtils.format( + "%s(%s, %s)", + name, + lambdaExpr.stringify(), + ARG_JOINER.join(argsExpr.stream().map(Expr::stringify).iterator()) + ); + } + @Override public void visit(Visitor visitor) { @@ -1056,6 +1144,12 @@ public ExprEval eval(ObjectBinding bindings) throw new IAE("unsupported type " + ret.type()); } + @Override + public String stringify() + { + return StringUtils.format("-%s", expr.stringify()); + } + @Override public String toString() { @@ -1088,6 +1182,12 @@ public ExprEval eval(ObjectBinding bindings) return ExprEval.of(!ret.asBoolean(), retType); } + @Override + public String stringify() + { + return StringUtils.format("!%s", expr.stringify()); + } + @Override public String toString() { @@ -1141,6 +1241,12 @@ public String toString() return StringUtils.format("(%s %s %s)", op, left, right); } + @Override + public String stringify() + { + return StringUtils.format("(%s %s %s)", left.stringify(), op, right.stringify()); + } + protected abstract BinaryOpExprBase copy(Expr left, Expr right); @Override diff --git a/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java b/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java index 825c5ed9f98d..7d92105007b5 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java +++ b/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java @@ -109,9 +109,13 @@ public void exitDoubleExpr(ExprParser.DoubleExprContext ctx) @Override public void exitDoubleArray(ExprParser.DoubleArrayContext ctx) { - Double[] values = new Double[ctx.DOUBLE().size()]; + Double[] values = new Double[ctx.doubleElement().size()]; for (int i = 0; i < values.length; i++) { - values[i] = Double.parseDouble(ctx.DOUBLE(i).getText()); + if (ctx.doubleElement(i).getText().equalsIgnoreCase("null")) { + values[i] = null; + } else { + values[i] = Double.parseDouble(ctx.doubleElement(i).getText()); + } } nodes.put(ctx, new DoubleArrayExpr(values)); } @@ -188,9 +192,13 @@ public void exitLogicalAndOrExpr(ExprParser.LogicalAndOrExprContext ctx) @Override public void exitLongArray(ExprParser.LongArrayContext ctx) { - Long[] values = new Long[ctx.LONG().size()]; + Long[] values = new Long[ctx.longElement().size()]; for (int i = 0; i < values.length; i++) { - values[i] = Long.parseLong(ctx.LONG(i).getText()); + if (ctx.longElement(i).getText().equalsIgnoreCase("null")) { + values[i] = null; + } else { + values[i] = Long.parseLong(ctx.longElement(i).getText()); + } } nodes.put(ctx, new LongArrayExpr(values)); } @@ -406,9 +414,9 @@ public void exitNull(ExprParser.NullContext ctx) @Override public void exitStringArray(ExprParser.StringArrayContext ctx) { - String[] values = new String[ctx.STRING().size()]; + String[] values = new String[ctx.stringElement().size()]; for (int i = 0; i < values.length; i++) { - values[i] = escapeStringLiteral(ctx.STRING(i).getText()); + values[i] = escapeStringLiteral(ctx.stringElement(i).getText()); } nodes.put(ctx, new StringArrayExpr(values)); } @@ -419,6 +427,24 @@ public void exitEmptyArray(ExprParser.EmptyArrayContext ctx) nodes.put(ctx, new StringArrayExpr(new String[0])); } + @Override + public void exitEmptyStringArray(ExprParser.EmptyStringArrayContext ctx) + { + nodes.put(ctx, new StringArrayExpr(new String[0])); + } + + @Override + public void exitEmptyDoubleArray(ExprParser.EmptyDoubleArrayContext ctx) + { + nodes.put(ctx, new DoubleArrayExpr(new Double[0])); + } + + @Override + public void exitEmptyLongArray(ExprParser.EmptyLongArrayContext ctx) + { + nodes.put(ctx, new LongArrayExpr(new Long[0])); + } + /** * All {@link IdentifierExpr} that are *not* bound to a {@link LambdaExpr} identifier, will recieve a unique * {@link IdentifierExpr#identifier} value which may or may not be the same as the @@ -457,6 +483,9 @@ private static String sanitizeIdentifierString(String text) */ private static String escapeStringLiteral(String text) { + if (text.equalsIgnoreCase("null")) { + return null; + } String unquoted = text.substring(1, text.length() - 1); return unquoted.indexOf('\\') >= 0 ? StringEscapeUtils.unescapeJava(unquoted) : unquoted; } diff --git a/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java b/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java index 80d0410eeae1..6baa3afe7c2f 100644 --- a/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java @@ -102,7 +102,7 @@ public void testFold() assertExpr("fold((b, acc) -> b * acc, map((b) -> b * 2, filter(b -> b > 3, b)), 1)", 80L); assertExpr("fold((a, acc) -> concat(a, acc), a, '')", "foobarbazbarfoo"); assertExpr("fold((a, acc) -> array_append(acc, a), a, [])", new String[]{"foo", "bar", "baz", "foobar"}); - assertExpr("fold((a, acc) -> array_append(acc, a), b, cast([], 'LONG_ARRAY'))", new Long[]{1L, 2L, 3L, 4L, 5L}); + assertExpr("fold((a, acc) -> array_append(acc, a), b, [])", new Long[]{1L, 2L, 3L, 4L, 5L}); } @Test @@ -161,6 +161,16 @@ private void assertExpr(final String expression, final Object expectedResult) { final Expr expr = Parser.parse(expression, ExprMacroTable.nil()); Assert.assertEquals(expression, expectedResult, expr.eval(bindings).value()); + + final Expr exprNoFlatten = Parser.parse(expression, ExprMacroTable.nil(), false); + final Expr roundTrip = Parser.parse(exprNoFlatten.stringify(), ExprMacroTable.nil()); + Assert.assertEquals(expr.stringify(), expectedResult, roundTrip.eval(bindings).value()); + + final Expr roundTripFlatten = Parser.parse(expr.stringify(), ExprMacroTable.nil()); + Assert.assertEquals(expr.stringify(), expectedResult, roundTripFlatten.eval(bindings).value()); + + Assert.assertEquals(expr.stringify(), roundTrip.stringify()); + Assert.assertEquals(expr.stringify(), roundTripFlatten.stringify()); } private void assertExpr(final String expression, final Object[] expectedResult) @@ -170,6 +180,22 @@ private void assertExpr(final String expression, final Object[] expectedResult) if (expectedResult.length != 0 || result == null || result.length != 0) { Assert.assertArrayEquals(expression, expectedResult, result); } + + final Expr exprNoFlatten = Parser.parse(expression, ExprMacroTable.nil(), false); + final Expr roundTrip = Parser.parse(exprNoFlatten.stringify(), ExprMacroTable.nil()); + final Object[] resultRoundTrip = roundTrip.eval(bindings).asArray(); + if (expectedResult.length != 0 || resultRoundTrip == null || resultRoundTrip.length != 0) { + Assert.assertArrayEquals(expr.stringify(), expectedResult, resultRoundTrip); + } + + final Expr roundTripFlatten = Parser.parse(expr.stringify(), ExprMacroTable.nil()); + final Object[] resultRoundTripFlatten = roundTripFlatten.eval(bindings).asArray(); + if (expectedResult.length != 0 || resultRoundTripFlatten == null || resultRoundTripFlatten.length != 0) { + Assert.assertArrayEquals(expr.stringify(), expectedResult, resultRoundTripFlatten); + } + + Assert.assertEquals(expr.stringify(), roundTrip.stringify()); + Assert.assertEquals(expr.stringify(), roundTripFlatten.stringify()); } private void assertExpr(final String expression, final Double[] expectedResult) @@ -180,5 +206,23 @@ private void assertExpr(final String expression, final Double[] expectedResult) for (int i = 0; i < result.length; i++) { Assert.assertEquals(expression, expectedResult[i], result[i], 0.00001); // something is lame somewhere.. } + + final Expr exprNoFlatten = Parser.parse(expression, ExprMacroTable.nil(), false); + final Expr roundTrip = Parser.parse(exprNoFlatten.stringify(), ExprMacroTable.nil()); + Double[] resultRoundTrip = (Double[]) roundTrip.eval(bindings).value(); + Assert.assertEquals(expectedResult.length, resultRoundTrip.length); + for (int i = 0; i < resultRoundTrip.length; i++) { + Assert.assertEquals(expression, expectedResult[i], resultRoundTrip[i], 0.00001); + } + + final Expr roundTripFlatten = Parser.parse(expr.stringify(), ExprMacroTable.nil()); + Double[] resultRoundTripFlatten= (Double[]) roundTripFlatten.eval(bindings).value(); + Assert.assertEquals(expectedResult.length, resultRoundTripFlatten.length); + for (int i = 0; i < resultRoundTripFlatten.length; i++) { + Assert.assertEquals(expression, expectedResult[i], resultRoundTripFlatten[i], 0.00001); + } + + Assert.assertEquals(expr.stringify(), roundTrip.stringify()); + Assert.assertEquals(expr.stringify(), roundTripFlatten.stringify()); } } diff --git a/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java b/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java index 5571096ccb4f..c2b84ffaa7d2 100644 --- a/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/FunctionTest.java @@ -231,7 +231,7 @@ public void testArrayAppend() assertExpr("array_append([1, 2, 3], 4)", new Long[]{1L, 2L, 3L, 4L}); assertExpr("array_append([1, 2, 3], 'bar')", new Long[]{1L, 2L, 3L, null}); assertExpr("array_append([], 1)", new String[]{"1"}); - assertExpr("array_append(cast([], 'LONG_ARRAY'), 1)", new Long[]{1L}); + assertExpr("array_append([], 1)", new Long[]{1L}); } @Test @@ -287,18 +287,39 @@ public void testArrayPrepend() assertExpr("array_prepend(4, [1, 2, 3])", new Long[]{4L, 1L, 2L, 3L}); assertExpr("array_prepend('bar', [1, 2, 3])", new Long[]{null, 1L, 2L, 3L}); assertExpr("array_prepend(1, [])", new String[]{"1"}); - assertExpr("array_prepend(1, cast([], 'LONG_ARRAY'))", new Long[]{1L}); + assertExpr("array_prepend(1, [])", new Long[]{1L}); + assertExpr("array_prepend(1, [])", new Double[]{1.0}); } private void assertExpr(final String expression, final Object expectedResult) { final Expr expr = Parser.parse(expression, ExprMacroTable.nil()); Assert.assertEquals(expression, expectedResult, expr.eval(bindings).value()); + + final Expr exprNoFlatten = Parser.parse(expression, ExprMacroTable.nil(), false); + final Expr roundTrip = Parser.parse(exprNoFlatten.stringify(), ExprMacroTable.nil()); + Assert.assertEquals(expr.stringify(), expectedResult, roundTrip.eval(bindings).value()); + + final Expr roundTripFlatten = Parser.parse(expr.stringify(), ExprMacroTable.nil()); + Assert.assertEquals(expr.stringify(), expectedResult, roundTripFlatten.eval(bindings).value()); + + Assert.assertEquals(expr.stringify(), roundTrip.stringify()); + Assert.assertEquals(expr.stringify(), roundTripFlatten.stringify()); } private void assertExpr(final String expression, final Object[] expectedResult) { final Expr expr = Parser.parse(expression, ExprMacroTable.nil()); Assert.assertArrayEquals(expression, expectedResult, expr.eval(bindings).asArray()); + + final Expr exprNoFlatten = Parser.parse(expression, ExprMacroTable.nil(), false); + final Expr roundTrip = Parser.parse(exprNoFlatten.stringify(), ExprMacroTable.nil()); + Assert.assertArrayEquals(expression, expectedResult, roundTrip.eval(bindings).asArray()); + + final Expr roundTripFlatten = Parser.parse(expr.stringify(), ExprMacroTable.nil()); + Assert.assertArrayEquals(expression, expectedResult, roundTripFlatten.eval(bindings).asArray()); + + Assert.assertEquals(expr.stringify(), roundTrip.stringify()); + Assert.assertEquals(expr.stringify(), roundTripFlatten.stringify()); } } diff --git a/core/src/test/java/org/apache/druid/math/expr/ParserTest.java b/core/src/test/java/org/apache/druid/math/expr/ParserTest.java index d4361b6e8228..6b706b4f1b87 100644 --- a/core/src/test/java/org/apache/druid/math/expr/ParserTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/ParserTest.java @@ -191,6 +191,15 @@ public void testLiteralArrays() validateConstantExpression("[1.0, 2.345]", new Double[]{1.0, 2.345}); validateConstantExpression("[1, 3]", new Long[]{1L, 3L}); validateConstantExpression("[\'hello\', \'world\']", new String[]{"hello", "world"}); + + validateConstantExpression("[1.0, null, 2.345]", new Double[]{1.0, null, 2.345}); + validateConstantExpression("[null, 1, 3]", new Long[]{null, 1L, 3L}); + validateConstantExpression("[\'hello\', \'world\', null]", new String[]{"hello", "world", null}); + + validateConstantExpression("[]", new String[0]); + validateConstantExpression("[]", new String[0]); + validateConstantExpression("[]", new Double[0]); + validateConstantExpression("[]", new Long[0]); } @Test @@ -454,8 +463,17 @@ public void testUniquify() private void validateFlatten(String expression, String withoutFlatten, String withFlatten) { - Assert.assertEquals(expression, withoutFlatten, Parser.parse(expression, ExprMacroTable.nil(), false).toString()); - Assert.assertEquals(expression, withFlatten, Parser.parse(expression, ExprMacroTable.nil(), true).toString()); + Expr notFlat = Parser.parse(expression, ExprMacroTable.nil(), false); + Expr flat = Parser.parse(expression, ExprMacroTable.nil(), true); + Assert.assertEquals(expression, withoutFlatten, notFlat.toString()); + Assert.assertEquals(expression, withFlatten, flat.toString()); + + Expr notFlatRoundTrip = Parser.parse(notFlat.stringify(), ExprMacroTable.nil(), false); + Expr flatRoundTrip = Parser.parse(flat.stringify(), ExprMacroTable.nil(), true); + Assert.assertEquals(expression, withoutFlatten, notFlatRoundTrip.toString()); + Assert.assertEquals(expression, withFlatten, flatRoundTrip.toString()); + Assert.assertEquals(notFlat.stringify(), notFlatRoundTrip.stringify()); + Assert.assertEquals(flat.stringify(), flatRoundTrip.stringify()); } private void validateParser(String expression, String expected, List identifiers) @@ -482,6 +500,14 @@ private void validateParser( Assert.assertEquals(expression, identifiers, deets.getRequiredBindingsList()); Assert.assertEquals(expression, scalars, deets.getScalarVariables()); Assert.assertEquals(expression, arrays, deets.getArrayVariables()); + + final Expr parsedNoFlatten = Parser.parse(expression, ExprMacroTable.nil(), false); + final Expr roundTrip = Parser.parse(parsedNoFlatten.stringify(), ExprMacroTable.nil()); + Assert.assertEquals(parsed.stringify(), roundTrip.stringify()); + final Expr.BindingDetails roundTripDeets = roundTrip.analyzeInputs(); + Assert.assertEquals(expression, identifiers, roundTripDeets.getRequiredBindingsList()); + Assert.assertEquals(expression, scalars, roundTripDeets.getScalarVariables()); + Assert.assertEquals(expression, arrays, roundTripDeets.getArrayVariables()); } private void validateApplyUnapplied( @@ -497,23 +523,54 @@ private void validateApplyUnapplied( final Expr transformed = Parser.applyUnappliedBindings(parsed, deets, identifiers); Assert.assertEquals(expression, unapplied, parsed.toString()); Assert.assertEquals(applied, applied, transformed.toString()); + + final Expr parsedNoFlatten = Parser.parse(expression, ExprMacroTable.nil(), false); + final Expr parsedRoundTrip = Parser.parse(parsedNoFlatten.stringify(), ExprMacroTable.nil()); + Expr.BindingDetails roundTripDeets = parsedRoundTrip.analyzeInputs(); + Parser.validateExpr(parsedRoundTrip, roundTripDeets); + final Expr transformedRoundTrip = Parser.applyUnappliedBindings(parsedRoundTrip, roundTripDeets, identifiers); + Assert.assertEquals(expression, unapplied, parsedRoundTrip.toString()); + Assert.assertEquals(applied, applied, transformedRoundTrip.toString()); + + Assert.assertEquals(parsed.stringify(), parsedRoundTrip.stringify()); + Assert.assertEquals(transformed.stringify(), transformedRoundTrip.stringify()); } private void validateConstantExpression(String expression, Object expected) { + Expr parsed = Parser.parse(expression, ExprMacroTable.nil()); Assert.assertEquals( expression, expected, - Parser.parse(expression, ExprMacroTable.nil()).eval(Parser.withMap(ImmutableMap.of())).value() + parsed.eval(Parser.withMap(ImmutableMap.of())).value() ); + + final Expr parsedNoFlatten = Parser.parse(expression, ExprMacroTable.nil(), false); + Expr parsedRoundTrip = Parser.parse(parsedNoFlatten.stringify(), ExprMacroTable.nil()); + Assert.assertEquals( + expression, + expected, + parsedRoundTrip.eval(Parser.withMap(ImmutableMap.of())).value() + ); + Assert.assertEquals(parsed.stringify(), parsedRoundTrip.stringify()); } private void validateConstantExpression(String expression, Object[] expected) { + Expr parsed = Parser.parse(expression, ExprMacroTable.nil()); + Assert.assertArrayEquals( + expression, + expected, + (Object[]) parsed.eval(Parser.withMap(ImmutableMap.of())).value() + ); + + final Expr parsedNoFlatten = Parser.parse(expression, ExprMacroTable.nil(), false); + Expr roundTrip = Parser.parse(parsedNoFlatten.stringify(), ExprMacroTable.nil()); Assert.assertArrayEquals( expression, expected, - (Object[]) Parser.parse(expression, ExprMacroTable.nil()).eval(Parser.withMap(ImmutableMap.of())).value() + (Object[]) roundTrip.eval(Parser.withMap(ImmutableMap.of())).value() ); + Assert.assertEquals(parsed.stringify(), roundTrip.stringify()); } } diff --git a/docs/misc/math-expr.md b/docs/misc/math-expr.md index 27b799d0c72d..6407433982e0 100644 --- a/docs/misc/math-expr.md +++ b/docs/misc/math-expr.md @@ -37,28 +37,15 @@ This expression language supports the following operators (listed in decreasing |<, <=, >, >=, ==, !=|Binary Comparison| |&&, |||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. +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. +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: `[]`, `[]`, or `[]`. -Expressions can contain variables. Variable names may contain letters, digits, '\_' and '$'. Variable names must not -begin with a digit. To escape other special characters, you can quote it with double quotation marks. +Expressions can contain variables. Variable names may contain letters, digits, '\_' and '$'. Variable names must not begin with a digit. To escape other special characters, you can quote it with double quotation marks. -For logical operators, a number is true if and only if it is positive (0 or negative value means false). For string -type, it's the evaluation result of 'Boolean.valueOf(string)'. +For logical operators, a number is true if and only if it is positive (0 or negative value means false). For string type, it's the evaluation result of 'Boolean.valueOf(string)'. -[Multi-value string dimensions](../querying/multi-value-dimensions.html) are supported and may be treated as either -scalar or array typed values. When treated as a scalar type, an expression will automatically be transformed to apply -the scalar operation across all values of the multi-valued type, to mimic Druid's native behavior. Values that result in -arrays will be coerced back into the native Druid string type for aggregation. Druid aggregations on multi-value string -dimensions on the individual values, _not_ the 'array', behaving similar to the `UNNEST` operator available in many SQL -dialects. However, by using the `array_to_string` function, aggregations may be done on a stringified version of the -complete array, allowing the complete row to be preserved. Using `string_to_array` in an expression post-aggregator, -allows transforming the stringified dimension back into the true native array type. +[Multi-value string dimensions](../querying/multi-value-dimensions.html) are supported and may be treated as either scalar or array typed values. When treated as a scalar type, an expression will automatically be transformed to apply the scalar operation across all values of the multi-valued type, to mimic Druid's native behavior. Values that result in arrays will be coerced back into the native Druid string type for aggregation. Druid aggregations on multi-value string dimensions on the individual values, _not_ the 'array', behaving similar to the `UNNEST` operator available in many SQL dialects. However, by using the `array_to_string` function, aggregations may be done on a stringified version of the complete array, allowing the complete row to be preserved. Using `string_to_array` in an expression post-aggregator, allows transforming the stringified dimension back into the true native array type. The following built-in functions are available. @@ -196,10 +183,7 @@ See javadoc of java.lang.Math for detailed explanation for each function. ## IP address functions -For the IPv4 address functions, the `address` argument can either be an IPv4 dotted-decimal string -(e.g., "192.168.0.1") or an IP address represented as a long (e.g., 3232235521). The `subnet` -argument should be a string formatted as an IPv4 address subnet in CIDR notation (e.g., -"192.168.0.0/16"). +For the IPv4 address functions, the `address` argument can either be an IPv4 dotted-decimal string (e.g., "192.168.0.1") or an IP address represented as a long (e.g., 3232235521). The `subnet` argument should be a string formatted as an IPv4 address subnet in CIDR notation (e.g., "192.168.0.0/16"). | function | description | | --- | --- | From 7f35b8f6025f2d3f583393f82d1a90d20bd23966 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Sat, 15 Feb 2020 05:40:51 -0800 Subject: [PATCH 02/10] oops, macros are expressions too --- .../druid/math/expr/ExprMacroTable.java | 25 ++++++++- .../org/apache/druid/math/expr/Parser.java | 2 +- .../expressions/BloomFilterExprMacro.java | 2 +- .../expression/IPv4AddressMatchExprMacro.java | 13 ++++- .../expression/IPv4AddressParseExprMacro.java | 6 +- .../IPv4AddressStringifyExprMacro.java | 6 +- .../druid/query/expression/LikeExprMacro.java | 21 ++++++- .../query/expression/LookupExprMacro.java | 12 +++- .../expression/RegexpExtractExprMacro.java | 22 +++++++- .../expression/TimestampCeilExprMacro.java | 8 ++- .../expression/TimestampExtractExprMacro.java | 21 ++++++- .../expression/TimestampFloorExprMacro.java | 7 ++- .../expression/TimestampFormatExprMacro.java | 25 ++++++++- .../expression/TimestampParseExprMacro.java | 25 ++++++++- .../expression/TimestampShiftExprMacro.java | 8 ++- .../druid/query/expression/TrimExprMacro.java | 55 +++++++++++++------ .../IPv4AddressMatchExprMacroTest.java | 2 +- .../druid/query/expression/ExprMacroTest.java | 8 +++ .../IPv4AddressMatchOperatorConversion.java | 4 +- .../IPv4AddressParseOperatorConversion.java | 4 +- ...Pv4AddressStringifyOperatorConversion.java | 4 +- 21 files changed, 223 insertions(+), 57 deletions(-) diff --git a/core/src/main/java/org/apache/druid/math/expr/ExprMacroTable.java b/core/src/main/java/org/apache/druid/math/expr/ExprMacroTable.java index 9f8a7bf51bae..b69197e35216 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ExprMacroTable.java +++ b/core/src/main/java/org/apache/druid/math/expr/ExprMacroTable.java @@ -19,6 +19,7 @@ package org.apache.druid.math.expr; +import com.google.common.base.Joiner; import com.google.common.base.Supplier; import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableList; @@ -97,13 +98,15 @@ public interface ExprMacro */ public abstract static class BaseScalarUnivariateMacroFunctionExpr implements Expr { + protected final String name; protected final Expr arg; // Use Supplier to memoize values as ExpressionSelectors#makeExprEvalSelector() can make repeated calls for them private final Supplier analyzeInputsSupplier; - public BaseScalarUnivariateMacroFunctionExpr(Expr arg) + public BaseScalarUnivariateMacroFunctionExpr(String name, Expr arg) { + this.name = name; this.arg = arg; analyzeInputsSupplier = Suppliers.memoize(this::supplyAnalyzeInputs); } @@ -121,6 +124,12 @@ public BindingDetails analyzeInputs() return analyzeInputsSupplier.get(); } + @Override + public String stringify() + { + return StringUtils.format("%s(%s)", name, arg.stringify()); + } + private BindingDetails supplyAnalyzeInputs() { return arg.analyzeInputs().withScalarArguments(ImmutableSet.of(arg)); @@ -132,17 +141,29 @@ private BindingDetails supplyAnalyzeInputs() */ public abstract static class BaseScalarMacroFunctionExpr implements Expr { + protected final String name; protected final List args; // Use Supplier to memoize values as ExpressionSelectors#makeExprEvalSelector() can make repeated calls for them private final Supplier analyzeInputsSupplier; - public BaseScalarMacroFunctionExpr(final List args) + public BaseScalarMacroFunctionExpr(String name, final List args) { + this.name = name; this.args = args; analyzeInputsSupplier = Suppliers.memoize(this::supplyAnalyzeInputs); } + @Override + public String stringify() + { + return StringUtils.format( + "%s(%s)", + name, + Joiner.on(", ").join(args.stream().map(Expr::stringify).iterator()) + ); + } + @Override public void visit(final Visitor visitor) { diff --git a/core/src/main/java/org/apache/druid/math/expr/Parser.java b/core/src/main/java/org/apache/druid/math/expr/Parser.java index 64d5440497e7..b816324ee8c0 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Parser.java +++ b/core/src/main/java/org/apache/druid/math/expr/Parser.java @@ -108,7 +108,7 @@ public static Expr parse(String in, ExprMacroTable macroTable) } @VisibleForTesting - static Expr parse(String in, ExprMacroTable macroTable, boolean withFlatten) + public static Expr parse(String in, ExprMacroTable macroTable, boolean withFlatten) { ExprLexer lexer = new ExprLexer(new ANTLRInputStream(in)); CommonTokenStream tokens = new CommonTokenStream(lexer); diff --git a/extensions-core/druid-bloom-filter/src/main/java/org/apache/druid/query/expressions/BloomFilterExprMacro.java b/extensions-core/druid-bloom-filter/src/main/java/org/apache/druid/query/expressions/BloomFilterExprMacro.java index 829df48cc48a..dcc3a16ccad6 100644 --- a/extensions-core/druid-bloom-filter/src/main/java/org/apache/druid/query/expressions/BloomFilterExprMacro.java +++ b/extensions-core/druid-bloom-filter/src/main/java/org/apache/druid/query/expressions/BloomFilterExprMacro.java @@ -71,7 +71,7 @@ class BloomExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr { private BloomExpr(Expr arg) { - super(arg); + super(FN_NAME, arg); } @Nonnull diff --git a/processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java index 6cc94ac61133..5e9cc85fe545 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java @@ -21,6 +21,7 @@ import org.apache.commons.net.util.SubnetUtils; import org.apache.druid.java.util.common.IAE; +import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprEval; import org.apache.druid.math.expr.ExprMacroTable; @@ -52,13 +53,13 @@ */ public class IPv4AddressMatchExprMacro implements ExprMacroTable.ExprMacro { - public static final String NAME = "ipv4_match"; + public static final String FN_NAME = "ipv4_match"; private static final int ARG_SUBNET = 1; @Override public String name() { - return NAME; + return FN_NAME; } @Override @@ -77,7 +78,7 @@ class IPv4AddressMatchExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunct private IPv4AddressMatchExpr(Expr arg, SubnetUtils.SubnetInfo subnetInfo) { - super(arg); + super(FN_NAME, arg); this.subnetInfo = subnetInfo; } @@ -116,6 +117,12 @@ public Expr visit(Shuttle shuttle) Expr newArg = arg.visit(shuttle); return shuttle.visit(new IPv4AddressMatchExpr(newArg, subnetInfo)); } + + @Override + public String stringify() + { + return StringUtils.format("%s(%s, %s)", FN_NAME, arg.stringify(), args.get(ARG_SUBNET).stringify()); + } } return new IPv4AddressMatchExpr(arg, subnetInfo); diff --git a/processing/src/main/java/org/apache/druid/query/expression/IPv4AddressParseExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/IPv4AddressParseExprMacro.java index 569c037148ee..fdf67b4cc5b1 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/IPv4AddressParseExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/IPv4AddressParseExprMacro.java @@ -47,12 +47,12 @@ */ public class IPv4AddressParseExprMacro implements ExprMacroTable.ExprMacro { - public static final String NAME = "ipv4_parse"; + public static final String FN_NAME = "ipv4_parse"; @Override public String name() { - return NAME; + return FN_NAME; } @Override @@ -68,7 +68,7 @@ class IPv4AddressParseExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunct { private IPv4AddressParseExpr(Expr arg) { - super(arg); + super(FN_NAME, arg); } @Nonnull diff --git a/processing/src/main/java/org/apache/druid/query/expression/IPv4AddressStringifyExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/IPv4AddressStringifyExprMacro.java index d9afa5872777..4aea0aa37182 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/IPv4AddressStringifyExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/IPv4AddressStringifyExprMacro.java @@ -46,12 +46,12 @@ */ public class IPv4AddressStringifyExprMacro implements ExprMacroTable.ExprMacro { - public static final String NAME = "ipv4_stringify"; + public static final String FN_NAME = "ipv4_stringify"; @Override public String name() { - return NAME; + return FN_NAME; } @Override @@ -67,7 +67,7 @@ class IPv4AddressStringifyExpr extends ExprMacroTable.BaseScalarUnivariateMacroF { private IPv4AddressStringifyExpr(Expr arg) { - super(arg); + super(FN_NAME, arg); } @Nonnull diff --git a/processing/src/main/java/org/apache/druid/query/expression/LikeExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/LikeExprMacro.java index a1c980465eff..bc0ce4e36f5d 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/LikeExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/LikeExprMacro.java @@ -21,6 +21,7 @@ import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.IAE; +import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprEval; import org.apache.druid.math.expr.ExprMacroTable; @@ -32,10 +33,11 @@ public class LikeExprMacro implements ExprMacroTable.ExprMacro { + private static final String FN_NAME = "like"; @Override public String name() { - return "like"; + return FN_NAME; } @Override @@ -71,7 +73,7 @@ class LikeExtractExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionEx { private LikeExtractExpr(Expr arg) { - super(arg); + super(FN_NAME, arg); } @Nonnull @@ -87,6 +89,21 @@ public Expr visit(Shuttle shuttle) Expr newArg = arg.visit(shuttle); return shuttle.visit(new LikeExtractExpr(newArg)); } + + @Override + public String stringify() + { + if (escapeExpr != null) { + return StringUtils.format( + "%s(%s, %s, %s)", + FN_NAME, + arg.stringify(), + patternExpr.stringify(), + escapeExpr.stringify() + ); + } + return StringUtils.format("%s(%s, %s)", FN_NAME, arg.stringify(), patternExpr.stringify()); + } } return new LikeExtractExpr(arg); } diff --git a/processing/src/main/java/org/apache/druid/query/expression/LookupExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/LookupExprMacro.java index 88e3ce44e578..a827aea2601d 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/LookupExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/LookupExprMacro.java @@ -22,6 +22,7 @@ import com.google.inject.Inject; import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.IAE; +import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprEval; import org.apache.druid.math.expr.ExprMacroTable; @@ -33,6 +34,7 @@ public class LookupExprMacro implements ExprMacroTable.ExprMacro { + private static final String FN_NAME = "lookup"; private final LookupExtractorFactoryContainerProvider lookupExtractorFactoryContainerProvider; @Inject @@ -44,7 +46,7 @@ public LookupExprMacro(final LookupExtractorFactoryContainerProvider lookupExtra @Override public String name() { - return "lookup"; + return FN_NAME; } @Override @@ -75,7 +77,7 @@ class LookupExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr { private LookupExpr(Expr arg) { - super(arg); + super(FN_NAME, arg); } @Nonnull @@ -91,6 +93,12 @@ public Expr visit(Shuttle shuttle) Expr newArg = arg.visit(shuttle); return shuttle.visit(new LookupExpr(newArg)); } + + @Override + public String stringify() + { + return StringUtils.format("%s(%s, %s)", FN_NAME, arg.stringify(), lookupExpr.stringify()); + } } return new LookupExpr(arg); diff --git a/processing/src/main/java/org/apache/druid/query/expression/RegexpExtractExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/RegexpExtractExprMacro.java index df8f1f955a4a..7428ede84647 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/RegexpExtractExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/RegexpExtractExprMacro.java @@ -21,6 +21,7 @@ import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.IAE; +import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprEval; import org.apache.druid.math.expr.ExprMacroTable; @@ -32,10 +33,12 @@ public class RegexpExtractExprMacro implements ExprMacroTable.ExprMacro { + private static final String FN_NAME = "regexp_extract"; + @Override public String name() { - return "regexp_extract"; + return FN_NAME; } @Override @@ -62,7 +65,7 @@ class RegexpExtractExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunction { private RegexpExtractExpr(Expr arg) { - super(arg); + super(FN_NAME, arg); } @Nonnull @@ -81,6 +84,21 @@ public Expr visit(Shuttle shuttle) Expr newArg = arg.visit(shuttle); return shuttle.visit(new RegexpExtractExpr(newArg)); } + + @Override + public String stringify() + { + if (indexExpr != null) { + return StringUtils.format( + "%s(%s, %s, %s)", + FN_NAME, + arg.stringify(), + patternExpr.stringify(), + indexExpr.stringify() + ); + } + return StringUtils.format("%s(%s, %s)", FN_NAME, arg.stringify(), patternExpr.stringify()); + } } return new RegexpExtractExpr(arg); } diff --git a/processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java index bb2f5af5e1e9..d50c5005fbfc 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java @@ -34,10 +34,12 @@ public class TimestampCeilExprMacro implements ExprMacroTable.ExprMacro { + private static final String FN_NAME = "timestamp_ceil"; + @Override public String name() { - return "timestamp_ceil"; + return FN_NAME; } @Override @@ -60,7 +62,7 @@ private static class TimestampCeilExpr extends ExprMacroTable.BaseScalarMacroFun TimestampCeilExpr(final List args) { - super(args); + super(FN_NAME, args); this.granularity = getGranularity(args, ExprUtils.nilBindings()); } @@ -103,7 +105,7 @@ private static class TimestampCeilDynamicExpr extends ExprMacroTable.BaseScalarM { TimestampCeilDynamicExpr(final List args) { - super(args); + super(FN_NAME, args); } @Nonnull diff --git a/processing/src/main/java/org/apache/druid/query/expression/TimestampExtractExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/TimestampExtractExprMacro.java index af840dd817ef..d3184dd5ee38 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/TimestampExtractExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/TimestampExtractExprMacro.java @@ -34,6 +34,8 @@ public class TimestampExtractExprMacro implements ExprMacroTable.ExprMacro { + private static final String FN_NAME = "timestamp_extract"; + public enum Unit { EPOCH, @@ -59,7 +61,7 @@ public enum Unit @Override public String name() { - return "timestamp_extract"; + return FN_NAME; } @Override @@ -93,7 +95,7 @@ class TimestampExtractExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunct { private TimestampExtractExpr(Expr arg) { - super(arg); + super(FN_NAME, arg); } @Nonnull @@ -159,6 +161,21 @@ public Expr visit(Shuttle shuttle) Expr newArg = arg.visit(shuttle); return shuttle.visit(new TimestampExtractExpr(newArg)); } + + @Override + public String stringify() + { + if (args.size() > 2) { + return StringUtils.format( + "%s(%s, %s, %s)", + FN_NAME, + arg.stringify(), + args.get(1).stringify(), + args.get(2).stringify() + ); + } + return StringUtils.format("%s(%s, %s)", FN_NAME, arg.stringify(), args.get(1).stringify()); + } } return new TimestampExtractExpr(arg); diff --git a/processing/src/main/java/org/apache/druid/query/expression/TimestampFloorExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/TimestampFloorExprMacro.java index 00cbb547ac64..c6b7e7ea66e5 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/TimestampFloorExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/TimestampFloorExprMacro.java @@ -32,10 +32,11 @@ public class TimestampFloorExprMacro implements ExprMacroTable.ExprMacro { + private static final String FN_NAME = "timestamp_floor"; @Override public String name() { - return "timestamp_floor"; + return FN_NAME; } @Override @@ -68,7 +69,7 @@ public static class TimestampFloorExpr extends ExprMacroTable.BaseScalarMacroFun TimestampFloorExpr(final List args) { - super(args); + super(FN_NAME, args); this.granularity = computeGranularity(args, ExprUtils.nilBindings()); } @@ -113,7 +114,7 @@ public static class TimestampFloorDynamicExpr extends ExprMacroTable.BaseScalarM { TimestampFloorDynamicExpr(final List args) { - super(args); + super(FN_NAME, args); } @Nonnull diff --git a/processing/src/main/java/org/apache/druid/query/expression/TimestampFormatExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/TimestampFormatExprMacro.java index 3f222bfd39c1..e7f469666854 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/TimestampFormatExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/TimestampFormatExprMacro.java @@ -21,6 +21,7 @@ import com.google.common.base.Preconditions; import org.apache.druid.java.util.common.IAE; +import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprEval; import org.apache.druid.math.expr.ExprMacroTable; @@ -34,10 +35,12 @@ public class TimestampFormatExprMacro implements ExprMacroTable.ExprMacro { + private static final String FN_NAME = "timestamp_format"; + @Override public String name() { - return "timestamp_format"; + return FN_NAME; } @Override @@ -72,7 +75,7 @@ class TimestampFormatExpr extends ExprMacroTable.BaseScalarUnivariateMacroFuncti { private TimestampFormatExpr(Expr arg) { - super(arg); + super(FN_NAME, arg); } @Nonnull @@ -93,6 +96,24 @@ public Expr visit(Shuttle shuttle) Expr newArg = arg.visit(shuttle); return shuttle.visit(new TimestampFormatExpr(newArg)); } + + @Override + public String stringify() + { + if (args.size() > 2) { + return StringUtils.format( + "%s(%s, %s, %s)", + FN_NAME, + arg.stringify(), + args.get(1).stringify(), + args.get(2).stringify() + ); + } + if (args.size() > 1) { + return StringUtils.format("%s(%s, %s)", FN_NAME, arg.stringify(), args.get(1).stringify()); + } + return super.stringify(); + } } return new TimestampFormatExpr(arg); diff --git a/processing/src/main/java/org/apache/druid/query/expression/TimestampParseExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/TimestampParseExprMacro.java index b2079f4ac194..535c7332554b 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/TimestampParseExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/TimestampParseExprMacro.java @@ -21,6 +21,7 @@ import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.IAE; +import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprEval; import org.apache.druid.math.expr.ExprMacroTable; @@ -36,10 +37,12 @@ public class TimestampParseExprMacro implements ExprMacroTable.ExprMacro { + private static final String FN_NAME = "timestamp_parse"; + @Override public String name() { - return "timestamp_parse"; + return FN_NAME; } @Override @@ -68,7 +71,7 @@ class TimestampParseExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctio { private TimestampParseExpr(Expr arg) { - super(arg); + super(FN_NAME, arg); } @Nonnull @@ -96,6 +99,24 @@ public Expr visit(Shuttle shuttle) Expr newArg = arg.visit(shuttle); return shuttle.visit(new TimestampParseExpr(newArg)); } + + @Override + public String stringify() + { + if (args.size() > 2) { + return StringUtils.format( + "%s(%s, %s, %s)", + FN_NAME, + arg.stringify(), + args.get(1).stringify(), + args.get(2).stringify() + ); + } + if (args.size() > 1) { + return StringUtils.format("%s(%s, %s)", FN_NAME, arg.stringify(), args.get(1).stringify()); + } + return super.stringify(); + } } return new TimestampParseExpr(arg); diff --git a/processing/src/main/java/org/apache/druid/query/expression/TimestampShiftExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/TimestampShiftExprMacro.java index 872c89fcc00b..b3f8d1e767f2 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/TimestampShiftExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/TimestampShiftExprMacro.java @@ -34,10 +34,12 @@ public class TimestampShiftExprMacro implements ExprMacroTable.ExprMacro { + private static final String FN_NAME = "timestamp_shift"; + @Override public String name() { - return "timestamp_shift"; + return FN_NAME; } @Override @@ -79,7 +81,7 @@ private static class TimestampShiftExpr extends ExprMacroTable.BaseScalarMacroFu TimestampShiftExpr(final List args) { - super(args); + super(FN_NAME, args); final PeriodGranularity granularity = getGranularity(args, ExprUtils.nilBindings()); period = granularity.getPeriod(); chronology = ISOChronology.getInstance(granularity.getTimeZone()); @@ -105,7 +107,7 @@ private static class TimestampShiftDynamicExpr extends ExprMacroTable.BaseScalar { TimestampShiftDynamicExpr(final List args) { - super(args); + super(FN_NAME, args); } @Nonnull diff --git a/processing/src/main/java/org/apache/druid/query/expression/TrimExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/TrimExprMacro.java index c7546a5e823e..90cde1c91d77 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/TrimExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/TrimExprMacro.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableSet; import org.apache.druid.java.util.common.IAE; +import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprEval; import org.apache.druid.math.expr.ExprMacroTable; @@ -35,19 +36,26 @@ public abstract class TrimExprMacro implements ExprMacroTable.ExprMacro enum TrimMode { - BOTH(true, true), - LEFT(true, false), - RIGHT(false, true); + BOTH("trim", true, true), + LEFT("ltrim", true, false), + RIGHT("rtrim", false, true); + private final String name; private final boolean left; private final boolean right; - TrimMode(final boolean left, final boolean right) + TrimMode(final String name, final boolean left, final boolean right) { + this.name = name; this.left = left; this.right = right; } + public String getFnName() + { + return name; + } + public boolean isLeft() { return left; @@ -60,18 +68,16 @@ public boolean isRight() } private final TrimMode mode; - private final String name; - public TrimExprMacro(final String name, final TrimMode mode) + public TrimExprMacro(final TrimMode mode) { - this.name = name; this.mode = mode; } @Override public String name() { - return name; + return mode.getFnName(); } @Override @@ -82,13 +88,13 @@ public Expr apply(final List args) } if (args.size() == 1) { - return new TrimStaticCharsExpr(mode, args.get(0), DEFAULT_CHARS); + return new TrimStaticCharsExpr(mode, args.get(0), DEFAULT_CHARS, null); } else { final Expr charsArg = args.get(1); if (charsArg.isLiteral()) { final String charsString = charsArg.eval(ExprUtils.nilBindings()).asString(); final char[] chars = charsString == null ? EMPTY_CHARS : charsString.toCharArray(); - return new TrimStaticCharsExpr(mode, args.get(0), chars); + return new TrimStaticCharsExpr(mode, args.get(0), chars, charsArg); } else { return new TrimDynamicCharsExpr(mode, args.get(0), args.get(1)); } @@ -99,12 +105,14 @@ private static class TrimStaticCharsExpr extends ExprMacroTable.BaseScalarUnivar { private final TrimMode mode; private final char[] chars; + private final Expr charsExpr; - public TrimStaticCharsExpr(final TrimMode mode, final Expr stringExpr, final char[] chars) + public TrimStaticCharsExpr(final TrimMode mode, final Expr stringExpr, final char[] chars, final Expr charsExpr) { - super(stringExpr); + super(mode.getFnName(), stringExpr); this.mode = mode; this.chars = chars; + this.charsExpr = charsExpr; } @Nonnull @@ -153,7 +161,16 @@ public ExprEval eval(final ObjectBinding bindings) public Expr visit(Shuttle shuttle) { Expr newStringExpr = arg.visit(shuttle); - return shuttle.visit(new TrimStaticCharsExpr(mode, newStringExpr, chars)); + return shuttle.visit(new TrimStaticCharsExpr(mode, newStringExpr, chars, charsExpr)); + } + + @Override + public String stringify() + { + if (charsExpr != null) { + return StringUtils.format("%s(%s, %s)", mode.getFnName(), arg.stringify(), charsExpr.stringify()); + } + return super.stringify(); } } @@ -219,6 +236,12 @@ public ExprEval eval(final ObjectBinding bindings) } } + @Override + public String stringify() + { + return StringUtils.format("%s(%s, %s)", mode.getFnName(), stringExpr.stringify(), charsExpr.stringify()); + } + @Override public void visit(final Visitor visitor) { @@ -270,7 +293,7 @@ public static class BothTrimExprMacro extends TrimExprMacro { public BothTrimExprMacro() { - super("trim", TrimMode.BOTH); + super(TrimMode.BOTH); } } @@ -278,7 +301,7 @@ public static class LeftTrimExprMacro extends TrimExprMacro { public LeftTrimExprMacro() { - super("ltrim", TrimMode.LEFT); + super(TrimMode.LEFT); } } @@ -286,7 +309,7 @@ public static class RightTrimExprMacro extends TrimExprMacro { public RightTrimExprMacro() { - super("rtrim", TrimMode.RIGHT); + super(TrimMode.RIGHT); } } } diff --git a/processing/src/test/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacroTest.java b/processing/src/test/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacroTest.java index c4f7d9c12a2c..0b63a720b840 100644 --- a/processing/src/test/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacroTest.java +++ b/processing/src/test/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacroTest.java @@ -193,7 +193,7 @@ private static class NotLiteralExpr extends ExprMacroTable.BaseScalarUnivariateM { NotLiteralExpr(Expr arg) { - super(arg); + super("not", arg); } @Override diff --git a/server/src/test/java/org/apache/druid/query/expression/ExprMacroTest.java b/server/src/test/java/org/apache/druid/query/expression/ExprMacroTest.java index abe34d0a2eb9..441cce0a9a93 100644 --- a/server/src/test/java/org/apache/druid/query/expression/ExprMacroTest.java +++ b/server/src/test/java/org/apache/druid/query/expression/ExprMacroTest.java @@ -232,5 +232,13 @@ private void assertExpr(final String expression, final Object expectedResult) { final Expr expr = Parser.parse(expression, LookupEnabledTestExprMacroTable.INSTANCE); Assert.assertEquals(expression, expectedResult, expr.eval(BINDINGS).value()); + + final Expr exprNotFlattened = Parser.parse(expression, LookupEnabledTestExprMacroTable.INSTANCE, false); + final Expr roundTripNotFlattened = + Parser.parse(exprNotFlattened.stringify(), LookupEnabledTestExprMacroTable.INSTANCE); + Assert.assertEquals(exprNotFlattened.stringify(), expectedResult, roundTripNotFlattened.eval(BINDINGS).value()); + + final Expr roundTrip = Parser.parse(expr.stringify(), LookupEnabledTestExprMacroTable.INSTANCE); + Assert.assertEquals(exprNotFlattened.stringify(), expectedResult, roundTrip.eval(BINDINGS).value()); } } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv4AddressMatchOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv4AddressMatchOperatorConversion.java index b9a39b427b25..2ecee46c02f6 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv4AddressMatchOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv4AddressMatchOperatorConversion.java @@ -41,7 +41,7 @@ public class IPv4AddressMatchOperatorConversion extends DirectOperatorConversion private static final SqlSingleOperandTypeChecker SUBNET_OPERAND = OperandTypes.family(SqlTypeFamily.STRING); private static final SqlFunction SQL_FUNCTION = OperatorConversions - .operatorBuilder(StringUtils.toUpperCase(IPv4AddressMatchExprMacro.NAME)) + .operatorBuilder(StringUtils.toUpperCase(IPv4AddressMatchExprMacro.FN_NAME)) .operandTypeChecker(OperandTypes.sequence("(expr,string)", ADDRESS_OPERAND, SUBNET_OPERAND)) .returnTypeInference(ReturnTypes.BOOLEAN_NULLABLE) .functionCategory(SqlFunctionCategory.USER_DEFINED_FUNCTION) @@ -49,7 +49,7 @@ public class IPv4AddressMatchOperatorConversion extends DirectOperatorConversion public IPv4AddressMatchOperatorConversion() { - super(SQL_FUNCTION, IPv4AddressMatchExprMacro.NAME); + super(SQL_FUNCTION, IPv4AddressMatchExprMacro.FN_NAME); } @Override diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv4AddressParseOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv4AddressParseOperatorConversion.java index 9658bff28af6..1981a91b4bb2 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv4AddressParseOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv4AddressParseOperatorConversion.java @@ -33,7 +33,7 @@ public class IPv4AddressParseOperatorConversion extends DirectOperatorConversion { private static final SqlFunction SQL_FUNCTION = OperatorConversions - .operatorBuilder(StringUtils.toUpperCase(IPv4AddressParseExprMacro.NAME)) + .operatorBuilder(StringUtils.toUpperCase(IPv4AddressParseExprMacro.FN_NAME)) .operandTypeChecker( OperandTypes.or( OperandTypes.family(SqlTypeFamily.STRING), @@ -45,7 +45,7 @@ public class IPv4AddressParseOperatorConversion extends DirectOperatorConversion public IPv4AddressParseOperatorConversion() { - super(SQL_FUNCTION, IPv4AddressParseExprMacro.NAME); + super(SQL_FUNCTION, IPv4AddressParseExprMacro.FN_NAME); } @Override diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv4AddressStringifyOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv4AddressStringifyOperatorConversion.java index eb48189b2320..6bd02b41221e 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv4AddressStringifyOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/IPv4AddressStringifyOperatorConversion.java @@ -33,7 +33,7 @@ public class IPv4AddressStringifyOperatorConversion extends DirectOperatorConversion { private static final SqlFunction SQL_FUNCTION = OperatorConversions - .operatorBuilder(StringUtils.toUpperCase(IPv4AddressStringifyExprMacro.NAME)) + .operatorBuilder(StringUtils.toUpperCase(IPv4AddressStringifyExprMacro.FN_NAME)) .operandTypeChecker( OperandTypes.or( OperandTypes.family(SqlTypeFamily.INTEGER), @@ -45,7 +45,7 @@ public class IPv4AddressStringifyOperatorConversion extends DirectOperatorConver public IPv4AddressStringifyOperatorConversion() { - super(SQL_FUNCTION, IPv4AddressStringifyExprMacro.NAME); + super(SQL_FUNCTION, IPv4AddressStringifyExprMacro.FN_NAME); } From 6745c34f3b875e4c9be82e9d2ab2e6b9bcd9f6df Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Sat, 15 Feb 2020 16:16:25 -0800 Subject: [PATCH 03/10] style --- .../test/java/org/apache/druid/math/expr/ApplyFunctionTest.java | 2 +- .../java/org/apache/druid/query/expression/LikeExprMacro.java | 1 + .../apache/druid/query/expression/TimestampFloorExprMacro.java | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java b/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java index 6baa3afe7c2f..8dea860b33cb 100644 --- a/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java @@ -216,7 +216,7 @@ private void assertExpr(final String expression, final Double[] expectedResult) } final Expr roundTripFlatten = Parser.parse(expr.stringify(), ExprMacroTable.nil()); - Double[] resultRoundTripFlatten= (Double[]) roundTripFlatten.eval(bindings).value(); + Double[] resultRoundTripFlatten = (Double[]) roundTripFlatten.eval(bindings).value(); Assert.assertEquals(expectedResult.length, resultRoundTripFlatten.length); for (int i = 0; i < resultRoundTripFlatten.length; i++) { Assert.assertEquals(expression, expectedResult[i], resultRoundTripFlatten[i], 0.00001); diff --git a/processing/src/main/java/org/apache/druid/query/expression/LikeExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/LikeExprMacro.java index bc0ce4e36f5d..2332b2858eaf 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/LikeExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/LikeExprMacro.java @@ -34,6 +34,7 @@ public class LikeExprMacro implements ExprMacroTable.ExprMacro { private static final String FN_NAME = "like"; + @Override public String name() { diff --git a/processing/src/main/java/org/apache/druid/query/expression/TimestampFloorExprMacro.java b/processing/src/main/java/org/apache/druid/query/expression/TimestampFloorExprMacro.java index c6b7e7ea66e5..85603ce019a2 100644 --- a/processing/src/main/java/org/apache/druid/query/expression/TimestampFloorExprMacro.java +++ b/processing/src/main/java/org/apache/druid/query/expression/TimestampFloorExprMacro.java @@ -33,6 +33,7 @@ public class TimestampFloorExprMacro implements ExprMacroTable.ExprMacro { private static final String FN_NAME = "timestamp_floor"; + @Override public String name() { From 9a8e23e0549112c2628feef6c3b409f53940df4d Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Sat, 15 Feb 2020 20:48:17 -0800 Subject: [PATCH 04/10] spotbugs --- .../main/java/org/apache/druid/math/expr/ExprListenerImpl.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java b/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java index 7d92105007b5..e4a3be0b9d3e 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java +++ b/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java @@ -28,6 +28,7 @@ import org.apache.druid.math.expr.antlr.ExprBaseListener; import org.apache.druid.math.expr.antlr.ExprParser; +import javax.annotation.Nullable; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -481,6 +482,7 @@ private static String sanitizeIdentifierString(String text) /** * Remove single quote from a string literal, returning unquoted string value */ + @Nullable private static String escapeStringLiteral(String text) { if (text.equalsIgnoreCase("null")) { From 7389deddf9e138f3ca502498eaf95cdda7af453f Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Tue, 18 Feb 2020 14:53:05 -0800 Subject: [PATCH 05/10] qualified type arrays --- .../org/apache/druid/math/expr/antlr/Expr.g4 | 42 +++++++++---------- .../java/org/apache/druid/math/expr/Expr.java | 6 +-- .../apache/druid/math/expr/ParserTest.java | 13 +++++- 3 files changed, 36 insertions(+), 25 deletions(-) diff --git a/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 b/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 index 10a9f6f9989f..a75134a75669 100644 --- a/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 +++ b/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 @@ -15,27 +15,27 @@ grammar Expr; -expr : 'null' # null - | ('-'|'!') expr # unaryOpExpr - | expr '^' expr # powOpExpr - | expr ('*'|'/'|'%') expr # mulDivModuloExpr - | expr ('+'|'-') expr # addSubExpr - | expr ('<'|'<='|'>'|'>='|'=='|'!=') expr # logicalOpExpr - | expr ('&&'|'||') expr # logicalAndOrExpr - | '(' expr ')' # nestedExpr - | IDENTIFIER '(' lambda ',' fnArgs ')' # applyFunctionExpr - | IDENTIFIER '(' fnArgs? ')' # functionExpr - | IDENTIFIER # identifierExpr - | DOUBLE # doubleExpr - | LONG # longExpr - | STRING # string - | '[' doubleElement (',' doubleElement)* ']' # doubleArray - | '[' longElement (',' longElement)* ']' # longArray - | '[' stringElement (',' stringElement)* ']' # stringArray - | '[]' # emptyArray - | '[]' # emptyStringArray - | '[]' # emptyDoubleArray - | '[]' # emptyLongArray +expr : 'null' # null + | ('-'|'!') expr # unaryOpExpr + | expr '^' expr # powOpExpr + | expr ('*'|'/'|'%') expr # mulDivModuloExpr + | expr ('+'|'-') expr # addSubExpr + | expr ('<'|'<='|'>'|'>='|'=='|'!=') expr # logicalOpExpr + | expr ('&&'|'||') expr # logicalAndOrExpr + | '(' expr ')' # nestedExpr + | IDENTIFIER '(' lambda ',' fnArgs ')' # applyFunctionExpr + | IDENTIFIER '(' fnArgs? ')' # functionExpr + | IDENTIFIER # identifierExpr + | DOUBLE # doubleExpr + | LONG # longExpr + | STRING # string + | ''? '[' stringElement (',' stringElement)* ']' # stringArray + | ''? '[' doubleElement (',' doubleElement)* ']' # doubleArray + | ''? '[' longElement (',' longElement)* ']' # longArray + | '[]' # emptyArray + | '[]' # emptyStringArray + | '[]' # emptyDoubleArray + | '[]' # emptyLongArray ; lambda : (IDENTIFIER | '(' ')' | '(' IDENTIFIER (',' IDENTIFIER)* ')') '->' expr diff --git a/core/src/main/java/org/apache/druid/math/expr/Expr.java b/core/src/main/java/org/apache/druid/math/expr/Expr.java index dbbf6d4ec52e..51e64248c2c7 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Expr.java +++ b/core/src/main/java/org/apache/druid/math/expr/Expr.java @@ -564,7 +564,7 @@ public String stringify() if (value.length == 0) { return "[]"; } - return toString(); + return StringUtils.format("%s", toString()); } } @@ -638,7 +638,7 @@ public String stringify() return "[]"; } return StringUtils.format( - "[%s]", + "[%s]", ARG_JOINER.join( Arrays.stream(value) .map(s -> s == null @@ -721,7 +721,7 @@ public String stringify() if (value.length == 0) { return "[]"; } - return toString(); + return StringUtils.format("%s", toString()); } } diff --git a/core/src/test/java/org/apache/druid/math/expr/ParserTest.java b/core/src/test/java/org/apache/druid/math/expr/ParserTest.java index 6b706b4f1b87..6a85aba332d8 100644 --- a/core/src/test/java/org/apache/druid/math/expr/ParserTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/ParserTest.java @@ -200,6 +200,15 @@ public void testLiteralArrays() validateConstantExpression("[]", new String[0]); validateConstantExpression("[]", new Double[0]); validateConstantExpression("[]", new Long[0]); + + validateConstantExpression("[null, null, null]", new String[]{null, null, null}); + validateConstantExpression("[null, null, null]", new Double[]{null, null, null}); + validateConstantExpression("[null, null, null]", new Long[]{null, null, null}); + validateConstantExpression("[null, null, null]", new String[]{null, null, null}); + + validateConstantExpression("[1, null, 2]", new Double[]{1.0, null, 2.0}); + validateConstantExpression("[3, null, 4]", new Long[]{3L, null, 4L}); + validateConstantExpression("['foo', 'bar', 'baz']", new String[]{"foo", "bar", "baz"}); } @Test @@ -558,12 +567,14 @@ private void validateConstantExpression(String expression, Object expected) private void validateConstantExpression(String expression, Object[] expected) { Expr parsed = Parser.parse(expression, ExprMacroTable.nil()); + Object evaluated = parsed.eval(Parser.withMap(ImmutableMap.of())).value(); Assert.assertArrayEquals( expression, expected, - (Object[]) parsed.eval(Parser.withMap(ImmutableMap.of())).value() + (Object[]) evaluated ); + Assert.assertEquals(expected.getClass(), evaluated.getClass()); final Expr parsedNoFlatten = Parser.parse(expression, ExprMacroTable.nil(), false); Expr roundTrip = Parser.parse(parsedNoFlatten.stringify(), ExprMacroTable.nil()); Assert.assertArrayEquals( From 8aafe2ea0bccfe16f459069adf9e8aae8792d2f6 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 19 Feb 2020 15:24:29 -0800 Subject: [PATCH 06/10] review stuffs --- core/src/main/java/org/apache/druid/math/expr/Expr.java | 4 ++++ .../java/org/apache/druid/math/expr/ExprListenerImpl.java | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/apache/druid/math/expr/Expr.java b/core/src/main/java/org/apache/druid/math/expr/Expr.java index 9e6193b9bbe0..7ec75f9e10d7 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Expr.java +++ b/core/src/main/java/org/apache/druid/math/expr/Expr.java @@ -603,6 +603,7 @@ public ExprEval eval(ObjectBinding bindings) @Override public String stringify() { + // escape as javascript string since string literals are wrapped in single quotes return value == null ? NULL_LITERAL : StringUtils.format("'%s'", StringEscapeUtils.escapeJavaScript(value)); } } @@ -640,12 +641,14 @@ public String stringify() if (value.length == 0) { return "[]"; } + return StringUtils.format( "[%s]", ARG_JOINER.join( Arrays.stream(value) .map(s -> s == null ? NULL_LITERAL + // escape as javascript string since string literals are wrapped in single quotes : StringUtils.format("'%s'", StringEscapeUtils.escapeJavaScript(s)) ) .iterator() @@ -819,6 +822,7 @@ public ExprEval eval(ObjectBinding bindings) @Override public String stringify() { + // escape as java strings since identifiers are wrapped in double quotes return StringUtils.format("\"%s\"", StringEscapeUtils.escapeJava(binding)); } diff --git a/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java b/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java index e4a3be0b9d3e..b4315794a399 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java +++ b/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java @@ -112,7 +112,7 @@ public void exitDoubleArray(ExprParser.DoubleArrayContext ctx) { Double[] values = new Double[ctx.doubleElement().size()]; for (int i = 0; i < values.length; i++) { - if (ctx.doubleElement(i).getText().equalsIgnoreCase("null")) { + if (ctx.doubleElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) { values[i] = null; } else { values[i] = Double.parseDouble(ctx.doubleElement(i).getText()); @@ -195,7 +195,7 @@ public void exitLongArray(ExprParser.LongArrayContext ctx) { Long[] values = new Long[ctx.longElement().size()]; for (int i = 0; i < values.length; i++) { - if (ctx.longElement(i).getText().equalsIgnoreCase("null")) { + if (ctx.longElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) { values[i] = null; } else { values[i] = Long.parseLong(ctx.longElement(i).getText()); @@ -485,7 +485,7 @@ private static String sanitizeIdentifierString(String text) @Nullable private static String escapeStringLiteral(String text) { - if (text.equalsIgnoreCase("null")) { + if (text.equalsIgnoreCase(Expr.NULL_LITERAL)) { return null; } String unquoted = text.substring(1, text.length() - 1); From ad0b01fa430a16cbfcc55ded91f9224252ec783e Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Thu, 20 Feb 2020 15:51:08 -0800 Subject: [PATCH 07/10] simplify grammar --- .../org/apache/druid/math/expr/antlr/Expr.g4 | 18 ++++---- .../druid/math/expr/ExprListenerImpl.java | 46 ++++++------------- .../druid/math/expr/ApplyFunctionTest.java | 8 ++-- .../apache/druid/math/expr/ParserTest.java | 2 +- 4 files changed, 29 insertions(+), 45 deletions(-) diff --git a/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 b/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 index a75134a75669..87bcee702f48 100644 --- a/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 +++ b/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 @@ -29,21 +29,23 @@ expr : 'null' # null | DOUBLE # doubleExpr | LONG # longExpr | STRING # string - | ''? '[' stringElement (',' stringElement)* ']' # stringArray - | ''? '[' doubleElement (',' doubleElement)* ']' # doubleArray - | ''? '[' longElement (',' longElement)* ']' # longArray - | '[]' # emptyArray - | '[]' # emptyStringArray - | '[]' # emptyDoubleArray - | '[]' # emptyLongArray + | ''? '[' stringArrayBody? ']' # stringArray + | ''? '[' doubleArrayBody? ']' # doubleArray + | ''? '[' longArrayBody? ']' # longArray ; lambda : (IDENTIFIER | '(' ')' | '(' IDENTIFIER (',' IDENTIFIER)* ')') '->' expr ; -fnArgs : expr (',' expr)* # functionArgs +fnArgs : expr (',' expr)* # functionArgs ; +stringArrayBody : stringElement (',' stringElement)*; + +doubleArrayBody : doubleElement (',' doubleElement)*; + +longArrayBody : longElement (',' longElement)*; + longElement : (LONG | 'null'); doubleElement : (DOUBLE | 'null'); diff --git a/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java b/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java index b4315794a399..a00a95a4e1fa 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java +++ b/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java @@ -110,12 +110,14 @@ public void exitDoubleExpr(ExprParser.DoubleExprContext ctx) @Override public void exitDoubleArray(ExprParser.DoubleArrayContext ctx) { - Double[] values = new Double[ctx.doubleElement().size()]; + Double[] values = ctx.doubleArrayBody() == null + ? new Double[0] + : new Double[ctx.doubleArrayBody().doubleElement().size()]; for (int i = 0; i < values.length; i++) { - if (ctx.doubleElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) { + if (ctx.doubleArrayBody().doubleElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) { values[i] = null; } else { - values[i] = Double.parseDouble(ctx.doubleElement(i).getText()); + values[i] = Double.parseDouble(ctx.doubleArrayBody().doubleElement(i).getText()); } } nodes.put(ctx, new DoubleArrayExpr(values)); @@ -193,12 +195,14 @@ public void exitLogicalAndOrExpr(ExprParser.LogicalAndOrExprContext ctx) @Override public void exitLongArray(ExprParser.LongArrayContext ctx) { - Long[] values = new Long[ctx.longElement().size()]; + Long[] values = ctx.longArrayBody() == null + ? new Long[0] + : new Long[ctx.longArrayBody().longElement().size()]; for (int i = 0; i < values.length; i++) { - if (ctx.longElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) { + if (ctx.longArrayBody().longElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) { values[i] = null; } else { - values[i] = Long.parseLong(ctx.longElement(i).getText()); + values[i] = Long.parseLong(ctx.longArrayBody().longElement(i).getText()); } } nodes.put(ctx, new LongArrayExpr(values)); @@ -415,37 +419,15 @@ public void exitNull(ExprParser.NullContext ctx) @Override public void exitStringArray(ExprParser.StringArrayContext ctx) { - String[] values = new String[ctx.stringElement().size()]; + String[] values = ctx.stringArrayBody() == null + ? new String[0] + : new String[ctx.stringArrayBody().stringElement().size()]; for (int i = 0; i < values.length; i++) { - values[i] = escapeStringLiteral(ctx.stringElement(i).getText()); + values[i] = escapeStringLiteral(ctx.stringArrayBody().stringElement(i).getText()); } nodes.put(ctx, new StringArrayExpr(values)); } - @Override - public void exitEmptyArray(ExprParser.EmptyArrayContext ctx) - { - nodes.put(ctx, new StringArrayExpr(new String[0])); - } - - @Override - public void exitEmptyStringArray(ExprParser.EmptyStringArrayContext ctx) - { - nodes.put(ctx, new StringArrayExpr(new String[0])); - } - - @Override - public void exitEmptyDoubleArray(ExprParser.EmptyDoubleArrayContext ctx) - { - nodes.put(ctx, new DoubleArrayExpr(new Double[0])); - } - - @Override - public void exitEmptyLongArray(ExprParser.EmptyLongArrayContext ctx) - { - nodes.put(ctx, new LongArrayExpr(new Long[0])); - } - /** * All {@link IdentifierExpr} that are *not* bound to a {@link LambdaExpr} identifier, will recieve a unique * {@link IdentifierExpr#identifier} value which may or may not be the same as the diff --git a/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java b/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java index 8dea860b33cb..0c0922d72171 100644 --- a/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java @@ -98,11 +98,11 @@ public void testFilter() @Test public void testFold() { - assertExpr("fold((x, y) -> x + y, [1, 1, 1, 1, 1], 0)", 5L); - assertExpr("fold((b, acc) -> b * acc, map((b) -> b * 2, filter(b -> b > 3, b)), 1)", 80L); - assertExpr("fold((a, acc) -> concat(a, acc), a, '')", "foobarbazbarfoo"); +// assertExpr("fold((x, y) -> x + y, [1, 1, 1, 1, 1], 0)", 5L); +// assertExpr("fold((b, acc) -> b * acc, map((b) -> b * 2, filter(b -> b > 3, b)), 1)", 80L); +// assertExpr("fold((a, acc) -> concat(a, acc), a, '')", "foobarbazbarfoo"); assertExpr("fold((a, acc) -> array_append(acc, a), a, [])", new String[]{"foo", "bar", "baz", "foobar"}); - assertExpr("fold((a, acc) -> array_append(acc, a), b, [])", new Long[]{1L, 2L, 3L, 4L, 5L}); +// assertExpr("fold((a, acc) -> array_append(acc, a), b, [])", new Long[]{1L, 2L, 3L, 4L, 5L}); } @Test diff --git a/core/src/test/java/org/apache/druid/math/expr/ParserTest.java b/core/src/test/java/org/apache/druid/math/expr/ParserTest.java index 6a85aba332d8..f644c0021726 100644 --- a/core/src/test/java/org/apache/druid/math/expr/ParserTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/ParserTest.java @@ -206,7 +206,7 @@ public void testLiteralArrays() validateConstantExpression("[null, null, null]", new Long[]{null, null, null}); validateConstantExpression("[null, null, null]", new String[]{null, null, null}); - validateConstantExpression("[1, null, 2]", new Double[]{1.0, null, 2.0}); + validateConstantExpression("[1.0, null, 2000.0]", new Double[]{1.0, null, 2000.0}); validateConstantExpression("[3, null, 4]", new Long[]{3L, null, 4L}); validateConstantExpression("['foo', 'bar', 'baz']", new String[]{"foo", "bar", "baz"}); } From 276a4c56b4743ebb1d24230ba0416d388b662623 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Fri, 21 Feb 2020 02:39:46 -0800 Subject: [PATCH 08/10] more permissive array parsing --- .../org/apache/druid/math/expr/antlr/Expr.g4 | 23 ++-- .../druid/java/util/common/Numbers.java | 40 ++++++ .../druid/math/expr/ExprListenerImpl.java | 127 ++++++++++++------ .../org/apache/druid/math/expr/Parser.java | 6 +- .../druid/java/util/common/NumbersTest.java | 32 +++++ .../druid/math/expr/ApplyFunctionTest.java | 8 +- .../apache/druid/math/expr/ParserTest.java | 81 ++++++++++- 7 files changed, 256 insertions(+), 61 deletions(-) diff --git a/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 b/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 index 87bcee702f48..db336a2d54e0 100644 --- a/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 +++ b/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 @@ -15,7 +15,7 @@ grammar Expr; -expr : 'null' # null +expr : NULL # null | ('-'|'!') expr # unaryOpExpr | expr '^' expr # powOpExpr | expr ('*'|'/'|'%') expr # mulDivModuloExpr @@ -29,9 +29,11 @@ expr : 'null' # null | DOUBLE # doubleExpr | LONG # longExpr | STRING # string - | ''? '[' stringArrayBody? ']' # stringArray - | ''? '[' doubleArrayBody? ']' # doubleArray - | ''? '[' longArrayBody? ']' # longArray + | '[' (stringElement (',' stringElement)*)? ']' # stringArray + | '[' longElement (',' longElement)*']' # longArray + | '' '[' (numericElement (',' numericElement)*)? ']' # explicitLongArray + | ''? '[' (numericElement (',' numericElement)*)? ']' # doubleArray + | '' '[' (literalElement (',' literalElement)*)? ']' # explicitStringArray ; lambda : (IDENTIFIER | '(' ')' | '(' IDENTIFIER (',' IDENTIFIER)* ')') '->' expr @@ -40,18 +42,15 @@ lambda : (IDENTIFIER | '(' ')' | '(' IDENTIFIER (',' IDENTIFIER)* ')') '->' expr fnArgs : expr (',' expr)* # functionArgs ; -stringArrayBody : stringElement (',' stringElement)*; +stringElement : (STRING | NULL); -doubleArrayBody : doubleElement (',' doubleElement)*; +longElement : (LONG | NULL); -longArrayBody : longElement (',' longElement)*; +numericElement : (LONG | DOUBLE | NULL); -longElement : (LONG | 'null'); - -doubleElement : (DOUBLE | 'null'); - -stringElement : (STRING | 'null'); +literalElement : (STRING | LONG | DOUBLE | NULL); +NULL : 'null'; IDENTIFIER : [_$a-zA-Z][_$a-zA-Z0-9]* | '"' (ESC | ~ [\"\\])* '"'; LONG : [0-9]+ ; DOUBLE : [0-9]+ '.' [0-9]* ; diff --git a/core/src/main/java/org/apache/druid/java/util/common/Numbers.java b/core/src/main/java/org/apache/druid/java/util/common/Numbers.java index 15a1b1f99049..d24d9799cca5 100644 --- a/core/src/main/java/org/apache/druid/java/util/common/Numbers.java +++ b/core/src/main/java/org/apache/druid/java/util/common/Numbers.java @@ -118,6 +118,7 @@ public static double tryParseDouble(@Nullable Object val, double nullValue) } } + /** * Try parsing the given Number or String object val as long. * @param val @@ -167,6 +168,45 @@ public static float tryParseFloat(@Nullable Object val, float nullValue) } } + /** + * Like {@link #tryParseDouble}, but does not produce a primitive and will explode if unable to produce a Double + * similar to {@link Double#parseDouble} + */ + @Nullable + public static Double parseDoubleObject(@Nullable String val) + { + if (val == null) { + return null; + } + Double d = Doubles.tryParse(val); + if (d != null) { + return d; + } + throw new NumberFormatException("Cannot parse string to double"); + } + + /** + * Like {@link #tryParseLong} but does not produce a primitive and will explode if unable to produce a Long + * similar to {@link Long#parseLong} + */ + @Nullable + public static Long parseLongObject(@Nullable String val) + { + if (val == null) { + return null; + } + Long lobj = Longs.tryParse(val); + if (lobj != null) { + return lobj; + } + // try as a double, for "ddd.dd" , Longs.tryParse(..) returns null + Double dobj = Doubles.tryParse((String) val); + if (dobj != null) { + return dobj.longValue(); + } + throw new NumberFormatException("Cannot parse string to long"); + } + public static int toIntExact(long value, String error) { if ((int) value != value) { diff --git a/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java b/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java index a00a95a4e1fa..efab221af188 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java +++ b/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java @@ -23,6 +23,7 @@ import org.antlr.v4.runtime.tree.TerminalNode; import org.apache.commons.lang.StringEscapeUtils; import org.apache.druid.annotations.UsedInGeneratedCode; +import org.apache.druid.java.util.common.Numbers; import org.apache.druid.java.util.common.RE; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.antlr.ExprBaseListener; @@ -78,7 +79,7 @@ public void exitUnaryOpExpr(ExprParser.UnaryOpExprContext ctx) nodes.put(ctx, new UnaryNotExpr((Expr) nodes.get(ctx.getChild(1)))); break; default: - throw new RuntimeException("Unrecognized unary operator " + ctx.getChild(0).getText()); + throw new RE("Unrecognized unary operator %s", ctx.getChild(0).getText()); } } @@ -98,6 +99,7 @@ public void exitApplyFunctionExpr(ExprParser.ApplyFunctionExprContext ctx) ); } + @Override public void exitDoubleExpr(ExprParser.DoubleExprContext ctx) { @@ -107,21 +109,6 @@ public void exitDoubleExpr(ExprParser.DoubleExprContext ctx) ); } - @Override - public void exitDoubleArray(ExprParser.DoubleArrayContext ctx) - { - Double[] values = ctx.doubleArrayBody() == null - ? new Double[0] - : new Double[ctx.doubleArrayBody().doubleElement().size()]; - for (int i = 0; i < values.length; i++) { - if (ctx.doubleArrayBody().doubleElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) { - values[i] = null; - } else { - values[i] = Double.parseDouble(ctx.doubleArrayBody().doubleElement(i).getText()); - } - } - nodes.put(ctx, new DoubleArrayExpr(values)); - } @Override public void exitAddSubExpr(ExprParser.AddSubExprContext ctx) @@ -149,7 +136,7 @@ public void exitAddSubExpr(ExprParser.AddSubExprContext ctx) ); break; default: - throw new RuntimeException("Unrecognized binary operator " + ctx.getChild(1).getText()); + throw new RE("Unrecognized binary operator %s", ctx.getChild(1).getText()); } } @@ -188,24 +175,8 @@ public void exitLogicalAndOrExpr(ExprParser.LogicalAndOrExprContext ctx) ); break; default: - throw new RuntimeException("Unrecognized binary operator " + ctx.getChild(1).getText()); - } - } - - @Override - public void exitLongArray(ExprParser.LongArrayContext ctx) - { - Long[] values = ctx.longArrayBody() == null - ? new Long[0] - : new Long[ctx.longArrayBody().longElement().size()]; - for (int i = 0; i < values.length; i++) { - if (ctx.longArrayBody().longElement(i).getText().equalsIgnoreCase(Expr.NULL_LITERAL)) { - values[i] = null; - } else { - values[i] = Long.parseLong(ctx.longArrayBody().longElement(i).getText()); - } + throw new RE("Unrecognized binary operator %s", ctx.getChild(1).getText()); } - nodes.put(ctx, new LongArrayExpr(values)); } @Override @@ -286,7 +257,7 @@ public void exitLogicalOpExpr(ExprParser.LogicalOpExprContext ctx) ); break; default: - throw new RuntimeException("Unrecognized binary operator " + ctx.getChild(1).getText()); + throw new RE("Unrecognized binary operator %s", ctx.getChild(1).getText()); } } @@ -326,7 +297,7 @@ public void exitMulDivModuloExpr(ExprParser.MulDivModuloExprContext ctx) ); break; default: - throw new RuntimeException("Unrecognized binary operator " + ctx.getChild(1).getText()); + throw new RE("Unrecognized binary operator ", ctx.getChild(1).getText()); } } @@ -416,14 +387,90 @@ public void exitNull(ExprParser.NullContext ctx) nodes.put(ctx, new StringExpr(null)); } + @Override + public void exitDoubleArray(ExprParser.DoubleArrayContext ctx) + { + Double[] values = new Double[ctx.numericElement().size()]; + for (int i = 0; i < values.length; i++) { + if (ctx.numericElement(i).NULL() != null) { + values[i] = null; + } else if (ctx.numericElement(i).LONG() != null) { + values[i] = Numbers.parseDoubleObject(ctx.numericElement(i).LONG().getText()); + } else if (ctx.numericElement(i).DOUBLE() != null) { + values[i] = Numbers.parseDoubleObject(ctx.numericElement(i).DOUBLE().getText()); + } else { + throw new RE("Failed to parse array element %s as a double", ctx.numericElement(i).getText()); + } + } + nodes.put(ctx, new DoubleArrayExpr(values)); + } + + @Override + public void exitLongArray(ExprParser.LongArrayContext ctx) + { + Long[] values = new Long[ctx.longElement().size()]; + for (int i = 0; i < values.length; i++) { + if (ctx.longElement(i).NULL() != null) { + values[i] = null; + } else if (ctx.longElement(i).LONG() != null) { + values[i] = Long.parseLong(ctx.longElement(i).LONG().getText()); + } else { + throw new RE("Failed to parse array element %s as a long", ctx.longElement(i).getText()); + } + } + nodes.put(ctx, new LongArrayExpr(values)); + } + + @Override + public void exitExplicitLongArray(ExprParser.ExplicitLongArrayContext ctx) + { + Long[] values = new Long[ctx.numericElement().size()]; + for (int i = 0; i < values.length; i++) { + if (ctx.numericElement(i).NULL() != null) { + values[i] = null; + } else if (ctx.numericElement(i).LONG() != null) { + values[i] = Numbers.parseLongObject(ctx.numericElement(i).LONG().getText()); + } else if (ctx.numericElement(i).DOUBLE() != null) { + values[i] = Numbers.parseLongObject(ctx.numericElement(i).DOUBLE().getText()); + } else { + throw new RE("Failed to parse array element %s as a long", ctx.numericElement(i).getText()); + } + } + nodes.put(ctx, new LongArrayExpr(values)); + } + @Override public void exitStringArray(ExprParser.StringArrayContext ctx) { - String[] values = ctx.stringArrayBody() == null - ? new String[0] - : new String[ctx.stringArrayBody().stringElement().size()]; + String[] values = new String[ctx.stringElement().size()]; + for (int i = 0; i < values.length; i++) { + if (ctx.stringElement(i).NULL() != null) { + values[i] = null; + } else if (ctx.stringElement(i).STRING() != null) { + values[i] = escapeStringLiteral(ctx.stringElement(i).STRING().getText()); + } else { + throw new RE("Failed to parse array: element %s is not a string", ctx.stringElement(i).getText()); + } + } + nodes.put(ctx, new StringArrayExpr(values)); + } + + @Override + public void exitExplicitStringArray(ExprParser.ExplicitStringArrayContext ctx) + { + String[] values = new String[ctx.literalElement().size()]; for (int i = 0; i < values.length; i++) { - values[i] = escapeStringLiteral(ctx.stringArrayBody().stringElement(i).getText()); + if (ctx.literalElement(i).NULL() != null) { + values[i] = null; + } else if (ctx.literalElement(i).STRING() != null) { + values[i] = escapeStringLiteral(ctx.literalElement(i).STRING().getText()); + } else if (ctx.literalElement(i).DOUBLE() != null) { + values[i] = ctx.literalElement(i).DOUBLE().getText(); + } else if (ctx.literalElement(i).LONG() != null) { + values[i] = ctx.literalElement(i).LONG().getText(); + } else { + throw new RE("Failed to parse array element %s as a string", ctx.literalElement(i).getText()); + } } nodes.put(ctx, new StringArrayExpr(values)); } diff --git a/core/src/main/java/org/apache/druid/math/expr/Parser.java b/core/src/main/java/org/apache/druid/math/expr/Parser.java index b816324ee8c0..d8fb564c4f2d 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Parser.java +++ b/core/src/main/java/org/apache/druid/math/expr/Parser.java @@ -118,7 +118,11 @@ public static Expr parse(String in, ExprMacroTable macroTable, boolean withFlatt ParseTreeWalker walker = new ParseTreeWalker(); ExprListenerImpl listener = new ExprListenerImpl(parseTree, macroTable); walker.walk(listener, parseTree); - return withFlatten ? flatten(listener.getAST()) : listener.getAST(); + Expr parsed = listener.getAST(); + if (parsed == null) { + throw new RE("Failed to parse expression: %s", in); + } + return withFlatten ? flatten(parsed) : parsed; } /** diff --git a/core/src/test/java/org/apache/druid/java/util/common/NumbersTest.java b/core/src/test/java/org/apache/druid/java/util/common/NumbersTest.java index dae51dbdd52f..0369930802d2 100644 --- a/core/src/test/java/org/apache/druid/java/util/common/NumbersTest.java +++ b/core/src/test/java/org/apache/druid/java/util/common/NumbersTest.java @@ -127,4 +127,36 @@ public void testParseBooleanWithUnparseableObject() expectedException.expectMessage(CoreMatchers.startsWith("Unknown type")); Numbers.parseBoolean(new Object()); } + + @Test + public void testParseLongObject() + { + Assert.assertEquals(null, Numbers.parseLongObject(null)); + Assert.assertEquals((Long) 1L, Numbers.parseLongObject("1")); + Assert.assertEquals((Long) 32L, Numbers.parseLongObject("32.1243")); + } + + @Test + public void testParseLongObjectUnparseable() + { + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Cannot parse string to long"); + Assert.assertEquals((Long) 1337L, Numbers.parseLongObject("'1'")); + } + + @Test + public void testParseDoubleObject() + { + Assert.assertEquals(null, Numbers.parseLongObject(null)); + Assert.assertEquals((Double) 1.0, Numbers.parseDoubleObject("1")); + Assert.assertEquals((Double) 32.1243, Numbers.parseDoubleObject("32.1243")); + } + + @Test + public void testParseDoubleObjectUnparseable() + { + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Cannot parse string to double"); + Assert.assertEquals((Double) 300.0, Numbers.parseDoubleObject("'1.1'")); + } } diff --git a/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java b/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java index 0c0922d72171..8dea860b33cb 100644 --- a/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/ApplyFunctionTest.java @@ -98,11 +98,11 @@ public void testFilter() @Test public void testFold() { -// assertExpr("fold((x, y) -> x + y, [1, 1, 1, 1, 1], 0)", 5L); -// assertExpr("fold((b, acc) -> b * acc, map((b) -> b * 2, filter(b -> b > 3, b)), 1)", 80L); -// assertExpr("fold((a, acc) -> concat(a, acc), a, '')", "foobarbazbarfoo"); + assertExpr("fold((x, y) -> x + y, [1, 1, 1, 1, 1], 0)", 5L); + assertExpr("fold((b, acc) -> b * acc, map((b) -> b * 2, filter(b -> b > 3, b)), 1)", 80L); + assertExpr("fold((a, acc) -> concat(a, acc), a, '')", "foobarbazbarfoo"); assertExpr("fold((a, acc) -> array_append(acc, a), a, [])", new String[]{"foo", "bar", "baz", "foobar"}); -// assertExpr("fold((a, acc) -> array_append(acc, a), b, [])", new Long[]{1L, 2L, 3L, 4L, 5L}); + assertExpr("fold((a, acc) -> array_append(acc, a), b, [])", new Long[]{1L, 2L, 3L, 4L, 5L}); } @Test diff --git a/core/src/test/java/org/apache/druid/math/expr/ParserTest.java b/core/src/test/java/org/apache/druid/math/expr/ParserTest.java index f644c0021726..b1ef6736ed5a 100644 --- a/core/src/test/java/org/apache/druid/math/expr/ParserTest.java +++ b/core/src/test/java/org/apache/druid/math/expr/ParserTest.java @@ -22,9 +22,12 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import org.apache.druid.java.util.common.RE; import org.apache.druid.testing.InitializedNullHandlingTest; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import java.util.Collections; import java.util.List; @@ -35,6 +38,9 @@ */ public class ParserTest extends InitializedNullHandlingTest { + @Rule + public ExpectedException expectedException = ExpectedException.none(); + @Test public void testSimple() { @@ -186,31 +192,98 @@ public void testLiterals() } @Test - public void testLiteralArrays() + public void testLiteralArraysHomogeneousElements() { validateConstantExpression("[1.0, 2.345]", new Double[]{1.0, 2.345}); validateConstantExpression("[1, 3]", new Long[]{1L, 3L}); - validateConstantExpression("[\'hello\', \'world\']", new String[]{"hello", "world"}); + validateConstantExpression("['hello', 'world']", new String[]{"hello", "world"}); + } + @Test + public void testLiteralArraysHomogeneousOrNullElements() + { validateConstantExpression("[1.0, null, 2.345]", new Double[]{1.0, null, 2.345}); validateConstantExpression("[null, 1, 3]", new Long[]{null, 1L, 3L}); - validateConstantExpression("[\'hello\', \'world\', null]", new String[]{"hello", "world", null}); + validateConstantExpression("['hello', 'world', null]", new String[]{"hello", "world", null}); + } + @Test + public void testLiteralArraysEmptyAndAllNullImplicitAreString() + { validateConstantExpression("[]", new String[0]); + validateConstantExpression("[null, null, null]", new String[]{null, null, null}); + } + + @Test + public void testLiteralArraysImplicitTypedNumericMixed() + { + // implicit typed numeric arrays with mixed elements are doubles + validateConstantExpression("[1, null, 2000.0]", new Double[]{1.0, null, 2000.0}); + validateConstantExpression("[1.0, null, 2000]", new Double[]{1.0, null, 2000.0}); + } + + @Test + public void testLiteralArraysExplicitTypedEmpties() + { validateConstantExpression("[]", new String[0]); validateConstantExpression("[]", new Double[0]); validateConstantExpression("[]", new Long[0]); + } - validateConstantExpression("[null, null, null]", new String[]{null, null, null}); + @Test + public void testLiteralArraysExplicitAllNull() + { validateConstantExpression("[null, null, null]", new Double[]{null, null, null}); validateConstantExpression("[null, null, null]", new Long[]{null, null, null}); validateConstantExpression("[null, null, null]", new String[]{null, null, null}); + } + @Test + public void testLiteralArraysExplicitTypes() + { validateConstantExpression("[1.0, null, 2000.0]", new Double[]{1.0, null, 2000.0}); validateConstantExpression("[3, null, 4]", new Long[]{3L, null, 4L}); validateConstantExpression("['foo', 'bar', 'baz']", new String[]{"foo", "bar", "baz"}); } + @Test + public void testLiteralArraysExplicitTypesMixedElements() + { + // explicit typed numeric arrays mixed numeric types should coerce to the correct explicit type + validateConstantExpression("[3, null, 4, 2.345]", new Double[]{3.0, null, 4.0, 2.345}); + validateConstantExpression("[1.0, null, 2000.0]", new Long[]{1L, null, 2000L}); + + // explicit typed string arrays should accept any literal and convert to string + validateConstantExpression("['1', null, 2000, 1.1]", new String[]{"1", null, "2000", "1.1"}); + } + + @Test + public void testLiteralArrayImplicitStringParseException() + { + // implicit typed string array cannot handle literals thate are not null or string + expectedException.expect(RE.class); + expectedException.expectMessage("Failed to parse array: element 2000 is not a string"); + validateConstantExpression("['1', null, 2000, 1.1]", new String[]{"1", null, "2000", "1.1"}); + } + + @Test + public void testLiteralArraysExplicitLongParseException() + { + // explicit typed long arrays only handle numeric types + expectedException.expect(RE.class); + expectedException.expectMessage("Failed to parse array element '2000' as a long"); + validateConstantExpression("[1, null, '2000']", new Long[]{1L, null, 2000L}); + } + + @Test + public void testLiteralArraysExplicitDoubleParseException() + { + // explicit typed double arrays only handle numeric types + expectedException.expect(RE.class); + expectedException.expectMessage("Failed to parse array element '2000.0' as a double"); + validateConstantExpression("[1.0, null, '2000.0']", new Double[]{1.0, null, 2000.0}); + } + @Test public void testFunctions() { From 7efa7d616fb97e0c67e254ff2a174205acf85ad5 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Fri, 21 Feb 2020 02:43:52 -0800 Subject: [PATCH 09/10] reuse expr joiner --- .../main/java/org/apache/druid/math/expr/ExprMacroTable.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/druid/math/expr/ExprMacroTable.java b/core/src/main/java/org/apache/druid/math/expr/ExprMacroTable.java index b69197e35216..a6fa4846cb12 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ExprMacroTable.java +++ b/core/src/main/java/org/apache/druid/math/expr/ExprMacroTable.java @@ -19,7 +19,6 @@ package org.apache.druid.math.expr; -import com.google.common.base.Joiner; import com.google.common.base.Supplier; import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableList; @@ -160,7 +159,7 @@ public String stringify() return StringUtils.format( "%s(%s)", name, - Joiner.on(", ").join(args.stream().map(Expr::stringify).iterator()) + Expr.ARG_JOINER.join(args.stream().map(Expr::stringify).iterator()) ); } From 56bf7c38762be0a26cbd7748d9d684ded93c1f38 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Fri, 21 Feb 2020 03:58:35 -0800 Subject: [PATCH 10/10] fix it --- .../main/java/org/apache/druid/math/expr/ExprListenerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java b/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java index efab221af188..ae41653950f9 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java +++ b/core/src/main/java/org/apache/druid/math/expr/ExprListenerImpl.java @@ -297,7 +297,7 @@ public void exitMulDivModuloExpr(ExprParser.MulDivModuloExprContext ctx) ); break; default: - throw new RE("Unrecognized binary operator ", ctx.getChild(1).getText()); + throw new RE("Unrecognized binary operator %s", ctx.getChild(1).getText()); } }