From dd466b9547f3a53cc39375bbb7b6991e3b9ade58 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 3 Jul 2019 05:21:23 -0700 Subject: [PATCH 1/8] multi-value string expression transformation fix --- .../java/org/apache/druid/math/expr/Expr.java | 265 ++++++++++++------ .../druid/math/expr/ExprListenerImpl.java | 63 ++++- .../druid/math/expr/ExprMacroTable.java | 17 +- .../org/apache/druid/math/expr/Parser.java | 92 +++--- .../apache/druid/math/expr/ParserTest.java | 53 +++- .../query/aggregation/AggregatorFactory.java | 2 +- .../SimpleDoubleAggregatorFactory.java | 2 +- .../SimpleFloatAggregatorFactory.java | 2 +- .../SimpleLongAggregatorFactory.java | 2 +- .../post/ExpressionPostAggregator.java | 2 +- .../druid/query/expression/TrimExprMacro.java | 16 +- .../query/filter/ExpressionDimFilter.java | 2 +- .../segment/filter/ExpressionFilter.java | 2 +- .../segment/virtual/ExpressionSelectors.java | 6 +- .../virtual/ExpressionVirtualColumn.java | 2 +- ...tCachingExpressionColumnValueSelector.java | 2 +- ...tCachingExpressionColumnValueSelector.java | 2 +- .../SingleStringInputDimensionSelector.java | 2 +- .../druid/query/MultiValuedDimensionTest.java | 52 +++- .../virtual/ExpressionVirtualColumnTest.java | 2 +- .../druid/sql/calcite/CalciteQueryTest.java | 115 +++++--- 21 files changed, 473 insertions(+), 230 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 271586b49e4d..e3062266aaf7 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 @@ -32,8 +32,8 @@ import org.apache.druid.java.util.common.guava.Comparators; import javax.annotation.Nullable; +import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Objects; @@ -70,6 +70,15 @@ default Object getLiteralValue() throw new ISE("Not a literal"); } + /** + * Returns an {@link IdentifierExpr} if it is one, else null + */ + @Nullable + default IdentifierExpr getIdentifierExprIfIdentifierExpr() + { + return null; + } + /** * Returns the string identifier of an {@link IdentifierExpr}, else null */ @@ -80,6 +89,17 @@ default String getIdentifierIfIdentifier() return null; } + /** + * Returns the string identifier to use to get a value from {@link Expr.ObjectBinding} of an {@link IdentifierExpr}, + * else null + */ + @Nullable + default String getIdentifierBindingIfIdentifier() + { + // overridden by things that are identifiers + return null; + } + /** * Evaluate the {@link Expr} with the bindings which supply {@link IdentifierExpr} with their values, producing an * {@link ExprEval} with the result. @@ -88,7 +108,7 @@ default String getIdentifierIfIdentifier() /** * 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. + * ensuring the {@link Visitor} can visit all of its {@link Expr} children before visiting itself */ void visit(Visitor visitor); @@ -147,39 +167,68 @@ interface Shuttle */ class BindingDetails { - private final ImmutableSet freeVariables; - private final ImmutableSet scalarVariables; - private final ImmutableSet arrayVariables; + private final ImmutableSet freeVariables; + private final ImmutableSet scalarVariables; + private final ImmutableSet arrayVariables; public BindingDetails() { - this(Collections.emptySet(), Collections.emptySet(), Collections.emptySet()); + this(ImmutableSet.of(), ImmutableSet.of(), ImmutableSet.of()); } - public BindingDetails(String identifier) + public BindingDetails(IdentifierExpr expr) { - this(ImmutableSet.of(identifier), Collections.emptySet(), Collections.emptySet()); + this(ImmutableSet.of(expr), ImmutableSet.of(), ImmutableSet.of()); } - public BindingDetails(Set freeVariables, Set scalarVariables, Set arrayVariables) + public BindingDetails( + ImmutableSet freeVariables, + ImmutableSet scalarVariables, + ImmutableSet arrayVariables + ) { - this.freeVariables = ImmutableSet.copyOf(freeVariables); - this.scalarVariables = ImmutableSet.copyOf(scalarVariables); - this.arrayVariables = ImmutableSet.copyOf(arrayVariables); + this.freeVariables = freeVariables; + this.scalarVariables = scalarVariables; + this.arrayVariables = arrayVariables; } /** * Get the list of required column inputs to evaluate an expression */ - public ImmutableList getRequiredColumns() + public List getRequiredColumnsList() { - return ImmutableList.copyOf(freeVariables); + return new ArrayList<>(freeVariables.stream().map(IdentifierExpr::getIdentifierBindingIfIdentifier).collect(Collectors.toSet())); + } + + /** + * Get the set of required column inputs to evaluate an expression + */ + public Set getRequiredColumns() + { + return freeVariables.stream().map(IdentifierExpr::getIdentifierBindingIfIdentifier).collect(Collectors.toSet()); + } + + /** + * Set of identifiers which are used with scalar operators and functions + */ + public Set getScalarColumns() + { + return scalarVariables.stream().map(IdentifierExpr::getIdentifierBindingIfIdentifier).collect(Collectors.toSet()); + } + + /** + * Set of identifiers which are used with array typed functions and apply functions. + */ + public Set getArrayColumns() + { + return arrayVariables.stream().map(IdentifierExpr::getIdentifierBindingIfIdentifier).collect(Collectors.toSet()); } /** * Total set of 'free' identifiers of an {@link Expr}, that are not supplied by a {@link LambdaExpr} binding + * @return */ - public ImmutableSet getFreeVariables() + public Set getFreeVariables() { return freeVariables; } @@ -187,52 +236,89 @@ public ImmutableSet getFreeVariables() /** * Set of identifiers which are used with scalar operators and functions */ - public ImmutableSet getScalarVariables() + public Set getScalarVariables() { - return scalarVariables; + return scalarVariables.stream().map(IdentifierExpr::getIdentifier).collect(Collectors.toSet()); } /** * Set of identifiers which are used with array typed functions and apply functions. */ - public ImmutableSet getArrayVariables() + public Set getArrayVariables() { - return arrayVariables; + return arrayVariables.stream().map(IdentifierExpr::getIdentifier).collect(Collectors.toSet()); } - public BindingDetails merge(BindingDetails other) + /** + * Combine with {@link BindingDetails} from {@link Expr#analyzeInputs()} + */ + public BindingDetails with(Expr other) { - return new BindingDetails( - Sets.union(freeVariables, other.freeVariables), - Sets.union(scalarVariables, other.scalarVariables), - Sets.union(arrayVariables, other.arrayVariables) - ); + return with(other.analyzeInputs()); } - public BindingDetails mergeWith(Set moreScalars, Set moreArrays) + /** + * Combine (union) another {@link BindingDetails} + */ + public BindingDetails with(BindingDetails other) { return new BindingDetails( - Sets.union(freeVariables, Sets.union(moreScalars, moreArrays)), - Sets.union(scalarVariables, moreScalars), - Sets.union(arrayVariables, moreArrays) + ImmutableSet.copyOf(Sets.union(freeVariables, other.freeVariables)), + ImmutableSet.copyOf(Sets.union(scalarVariables, other.scalarVariables)), + ImmutableSet.copyOf(Sets.union(arrayVariables, other.arrayVariables)) ); } - public BindingDetails mergeWithScalars(Set moreScalars) + /** + * Add set of arguments as {@link BindingDetails#scalarVariables} that are *directly* {@link IdentifierExpr}, + * else they are ignored. + */ + public BindingDetails withScalarArguments(Set scalarArguments) { + Set moreScalars = new HashSet<>(); + for (Expr expr : scalarArguments) { + final IdentifierExpr stringIdentifier = expr.getIdentifierExprIfIdentifierExpr(); + if (stringIdentifier != null) { + moreScalars.add((IdentifierExpr) expr); + } + } return new BindingDetails( - Sets.union(freeVariables, moreScalars), - Sets.union(scalarVariables, moreScalars), + ImmutableSet.copyOf(Sets.union(freeVariables, moreScalars)), + ImmutableSet.copyOf(Sets.union(scalarVariables, moreScalars)), arrayVariables ); } - public BindingDetails mergeWithArrays(Set moreArrays) + /** + * Add set of arguments as {@link BindingDetails#arrayVariables} that are *directly* {@link IdentifierExpr}, + * else they are ignored. + */ + public BindingDetails withArrayArguments(Set arrayArguments) { + Set arrayIdentifiers = new HashSet<>(); + for (Expr expr : arrayArguments) { + final IdentifierExpr isIdentifier = expr.getIdentifierExprIfIdentifierExpr(); + if (isIdentifier != null) { + arrayIdentifiers.add((IdentifierExpr) expr); + } + } return new BindingDetails( - Sets.union(freeVariables, moreArrays), + ImmutableSet.copyOf(Sets.union(freeVariables, arrayIdentifiers)), scalarVariables, - Sets.union(arrayVariables, moreArrays) + ImmutableSet.copyOf(Sets.union(arrayVariables, arrayIdentifiers)) + ); + } + + /** + * Remove any {@link IdentifierExpr} that are from a {@link LambdaExpr}, since the {@link ApplyFunction} will + * provide bindings for these variables. + */ + public BindingDetails removeLambdaArguments(Set lambda) + { + return new BindingDetails( + ImmutableSet.copyOf(freeVariables.stream().filter(x -> !lambda.contains(x.getIdentifier())).iterator()), + ImmutableSet.copyOf(scalarVariables.stream().filter(x -> !lambda.contains(x.getIdentifier())).iterator()), + ImmutableSet.copyOf(arrayVariables.stream().filter(x -> !lambda.contains(x.getIdentifier())).iterator()) ); } } @@ -482,10 +568,18 @@ public ExprEval eval(ObjectBinding bindings) class IdentifierExpr implements Expr { private final String identifier; + private final String bindingIdentifier; - IdentifierExpr(String value) + public IdentifierExpr(String value) { this.identifier = value; + this.bindingIdentifier = value; + } + + public IdentifierExpr(String identifier, String bindingIdentifier) + { + this.identifier = identifier; + this.bindingIdentifier = bindingIdentifier; } @Override @@ -494,6 +588,24 @@ public String toString() return identifier; } + /** + * Unique identifier + */ + @Nullable + public String getIdentifier() + { + return identifier; + } + + /** + * Value binding identifier + */ + @Nullable + public String getBindingIdentifier() + { + return bindingIdentifier; + } + @Nullable @Override public String getIdentifierIfIdentifier() @@ -501,16 +613,30 @@ public String getIdentifierIfIdentifier() return identifier; } + @Nullable + @Override + public String getIdentifierBindingIfIdentifier() + { + return bindingIdentifier; + } + + @Nullable + @Override + public IdentifierExpr getIdentifierExprIfIdentifierExpr() + { + return this; + } + @Override public BindingDetails analyzeInputs() { - return new BindingDetails(identifier); + return new BindingDetails(this); } @Override public ExprEval eval(ObjectBinding bindings) { - return ExprEval.bestEffortOf(bindings.get(identifier)); + return ExprEval.bestEffortOf(bindings.get(bindingIdentifier)); } @Override @@ -600,11 +726,7 @@ public BindingDetails analyzeInputs() { final Set lambdaArgs = args.stream().map(IdentifierExpr::toString).collect(Collectors.toSet()); BindingDetails bodyDetails = expr.analyzeInputs(); - return new BindingDetails( - Sets.difference(bodyDetails.getFreeVariables(), lambdaArgs), - Sets.difference(bodyDetails.getScalarVariables(), lambdaArgs), - Sets.difference(bodyDetails.getArrayVariables(), lambdaArgs) - ); + return bodyDetails.removeLambdaArguments(lambdaArgs); } } @@ -658,28 +780,13 @@ public Expr visit(Shuttle shuttle) @Override public BindingDetails analyzeInputs() { - final Set scalarVariables = new HashSet<>(); - final Set arrayVariables = new HashSet<>(); - final Set scalarArgs = function.getScalarInputs(args); - final Set arrayArgs = function.getArrayInputs(args); BindingDetails accumulator = new BindingDetails(); for (Expr arg : args) { - accumulator = accumulator.merge(arg.analyzeInputs()); - } - for (Expr arg : scalarArgs) { - String s = arg.getIdentifierIfIdentifier(); - if (s != null) { - scalarVariables.add(s); - } - } - for (Expr arg : arrayArgs) { - String s = arg.getIdentifierIfIdentifier(); - if (s != null) { - arrayVariables.add(s); - } + accumulator = accumulator.with(arg); } - return accumulator.mergeWith(scalarVariables, arrayVariables); + return accumulator.withScalarArguments(function.getScalarInputs(args)) + .withArrayArguments(function.getArrayInputs(args)); } } @@ -714,21 +821,11 @@ class ApplyFunctionExpr implements Expr for (Expr arg : argsExpr) { BindingDetails argDetails = arg.analyzeInputs(); argBindingDetailsBuilder.add(argDetails); - accumulator = accumulator.merge(argDetails); - } - - final Set arrayVariables = new HashSet<>(); - Set arrayArgs = function.getArrayInputs(argsExpr); - - for (Expr arg : arrayArgs) { - String s = arg.getIdentifierIfIdentifier(); - if (s != null) { - arrayVariables.add(s); - } + accumulator = accumulator.with(argDetails); } lambdaBindingDetails = lambdaExpr.analyzeInputs(); - bindingDetails = accumulator.merge(lambdaBindingDetails).mergeWithArrays(arrayVariables); + bindingDetails = accumulator.with(lambdaBindingDetails).withArrayArguments(function.getArrayInputs(argsExpr)); argsBindingDetails = argBindingDetailsBuilder.build(); } @@ -804,14 +901,7 @@ public Expr visit(Shuttle shuttle) public BindingDetails analyzeInputs() { // currently all unary operators only operate on scalar inputs - final Set scalars; - final String identifierMaybe = expr.getIdentifierIfIdentifier(); - if (identifierMaybe != null) { - scalars = ImmutableSet.of(identifierMaybe); - } else { - scalars = Collections.emptySet(); - } - return expr.analyzeInputs().mergeWithScalars(scalars); + return expr.analyzeInputs().withScalarArguments(ImmutableSet.of(expr)); } } @@ -934,18 +1024,7 @@ public String toString() public BindingDetails analyzeInputs() { // currently all binary operators operate on scalar inputs - final Set scalars = new HashSet<>(); - final String leftIdentifer = left.getIdentifierIfIdentifier(); - final String rightIdentifier = right.getIdentifierIfIdentifier(); - if (leftIdentifer != null) { - scalars.add(leftIdentifer); - } - if (rightIdentifier != null) { - scalars.add(rightIdentifier); - } - return left.analyzeInputs() - .merge(right.analyzeInputs()) - .mergeWithScalars(scalars); + return left.analyzeInputs().with(right).withScalarArguments(ImmutableSet.of(left, right)); } } 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 b4dc961e61f9..3373d7a9ce31 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 @@ -24,14 +24,17 @@ import org.apache.commons.lang.StringEscapeUtils; import org.apache.druid.annotations.UsedInGeneratedCode; import org.apache.druid.java.util.common.RE; +import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.antlr.ExprBaseListener; import org.apache.druid.math.expr.antlr.ExprParser; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; /** * Implementation of antlr parse tree listener, transforms {@link ParseTree} to {@link Expr}, based on the grammar @@ -44,11 +47,17 @@ public class ExprListenerImpl extends ExprBaseListener private final ExprMacroTable macroTable; private final ParseTree rootNodeKey; + private final Set lambadIdentifiers; + private final Set uniqueIdentifiers; + private int uniqueCounter = 0; + ExprListenerImpl(ParseTree rootNodeKey, ExprMacroTable macroTable) { this.rootNodeKey = rootNodeKey; this.macroTable = macroTable; this.nodes = new HashMap<>(); + this.lambadIdentifiers = new HashSet<>(); + this.uniqueIdentifiers = new HashSet<>(); } Expr getAST() @@ -347,14 +356,19 @@ public void exitFunctionExpr(ExprParser.FunctionExprContext ctx) @Override public void exitIdentifierExpr(ExprParser.IdentifierExprContext ctx) { - String text = ctx.getText(); - if (text.charAt(0) == '"' && text.charAt(text.length() - 1) == '"') { - text = StringEscapeUtils.unescapeJava(text.substring(1, text.length() - 1)); + final String text = sanitizeIdentifierString(ctx.getText()); + nodes.put(ctx, createIdentifierExpr(text)); + } + + @Override + public void enterLambda(ExprParser.LambdaContext ctx) + { + // mark lambda identifiers on enter, for reference later when creating the IdentifierExpr inside of the lambdas + for (int i = 0; i < ctx.IDENTIFIER().size(); i++) { + String text = ctx.IDENTIFIER(i).getText(); + text = sanitizeIdentifierString(text); + this.lambadIdentifiers.add(text); } - nodes.put( - ctx, - new IdentifierExpr(text) - ); } @Override @@ -363,10 +377,10 @@ public void exitLambda(ExprParser.LambdaContext ctx) List identifiers = new ArrayList<>(ctx.IDENTIFIER().size()); for (int i = 0; i < ctx.IDENTIFIER().size(); i++) { String text = ctx.IDENTIFIER(i).getText(); - if (text.charAt(0) == '"' && text.charAt(text.length() - 1) == '"') { - text = StringEscapeUtils.unescapeJava(text.substring(1, text.length() - 1)); - } - identifiers.add(i, new IdentifierExpr(text)); + text = sanitizeIdentifierString(text); + identifiers.add(i, createIdentifierExpr(text)); + // clean up lambda identifier references on exit + lambadIdentifiers.remove(text); } nodes.put(ctx, new LambdaExpr(identifiers, (Expr) nodes.get(ctx.expr()))); @@ -405,6 +419,33 @@ public void exitEmptyArray(ExprParser.EmptyArrayContext ctx) nodes.put(ctx, new StringArrayExpr(new String[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 + * {@link IdentifierExpr#bindingIdentifier} value. {@link LambdaExpr} identifiers however, will always have + * {@link IdentifierExpr#identifier} be the same as {@link IdentifierExpr#bindingIdentifier}. + */ + private IdentifierExpr createIdentifierExpr(String binding) + { + if (!lambadIdentifiers.contains(binding)) { + String uniqueIdentifier = binding; + while (uniqueIdentifiers.contains(uniqueIdentifier)) { + uniqueIdentifier = StringUtils.format("%s_%s", binding, uniqueCounter++); + } + uniqueIdentifiers.add(uniqueIdentifier); + return new IdentifierExpr(uniqueIdentifier, binding); + } + return new IdentifierExpr(binding); + } + + private static String sanitizeIdentifierString(String text) + { + if (text.charAt(0) == '"' && text.charAt(text.length() - 1) == '"') { + text = StringEscapeUtils.unescapeJava(text.substring(1, text.length() - 1)); + } + return text; + } + private static String escapeStringLiteral(String text) { String unquoted = text.substring(1, text.length() - 1); 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 370c5a1633ce..fce44f419e53 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 @@ -112,11 +112,7 @@ public void visit(final Visitor visitor) @Override public BindingDetails analyzeInputs() { - final String identifier = arg.getIdentifierIfIdentifier(); - if (identifier == null) { - return arg.analyzeInputs(); - } - return arg.analyzeInputs().mergeWithScalars(ImmutableSet.of(identifier)); + return arg.analyzeInputs().withScalarArguments(ImmutableSet.of(arg)); } } @@ -145,16 +141,13 @@ public void visit(final Visitor visitor) @Override public BindingDetails analyzeInputs() { - Set scalars = new HashSet<>(); + final Set argSet = new HashSet<>(args.size()); BindingDetails accumulator = new BindingDetails(); for (Expr arg : args) { - final String identifier = arg.getIdentifierIfIdentifier(); - if (identifier != null) { - scalars.add(identifier); - } - accumulator = accumulator.merge(arg.analyzeInputs()); + accumulator = accumulator.with(arg); + argSet.add(arg); } - return accumulator.mergeWithScalars(scalars); + return accumulator.withScalarArguments(argSet); } } } 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 59ed0dcbe8ab..2b07cbaa097b 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 @@ -171,13 +171,9 @@ public static Expr applyUnappliedIdentifiers(Expr expr, Expr.BindingDetails bind return expr; } List unapplied = toApply.stream() - .filter(x -> bindingDetails.getFreeVariables().contains(x)) + .filter(x -> bindingDetails.getRequiredColumnsList().contains(x)) .collect(Collectors.toList()); - ApplyFunction fn; - final LambdaExpr lambdaExpr; - final List args; - // any unapplied identifiers that are inside a lambda expression need that lambda expression to be rewritten Expr newExpr = expr.visit( childExpr -> { @@ -215,26 +211,45 @@ public static Expr applyUnappliedIdentifiers(Expr expr, Expr.BindingDetails bind return newExpr; } - // else, it *should be safe* to wrap in either map or cartesian_map because we still have missing bindings that - // were *not* referenced in a lambda body - if (remainingUnappliedArgs.size() == 1) { + return applyUnapplied(newExpr, remainingUnappliedArgs); + } + + private static Expr applyUnapplied(Expr expr, List unapplied) + { + // wrap an expression in either map or cartesian_map to apply any unapplied identifiers + + final Map toReplace = new HashMap<>(); + final List args = expr.analyzeInputs() + .getFreeVariables() + .stream() + .filter(x -> unapplied.contains(x.getBindingIdentifier())) + .collect(Collectors.toList()); + + final List lambdaArgs = new ArrayList<>(); + for (IdentifierExpr applyFnArg : args) { + IdentifierExpr lambdaRewrite = new IdentifierExpr(applyFnArg.getIdentifier(), applyFnArg.getIdentifier()); + lambdaArgs.add(lambdaRewrite); + toReplace.put(applyFnArg, lambdaRewrite); + } + + Expr newExpr = expr.visit(childExpr -> { + if (childExpr instanceof IdentifierExpr) { + if (toReplace.containsKey(childExpr)) { + return toReplace.get(childExpr); + } + } + return childExpr; + }); + + final LambdaExpr lambdaExpr = new LambdaExpr(lambdaArgs, newExpr); + final ApplyFunction fn; + if (args.size() == 1) { fn = new ApplyFunction.MapFunction(); - IdentifierExpr lambdaArg = new IdentifierExpr(remainingUnappliedArgs.iterator().next()); - lambdaExpr = new LambdaExpr(ImmutableList.of(lambdaArg), newExpr); - args = ImmutableList.of(lambdaArg); } else { fn = new ApplyFunction.CartesianMapFunction(); - List identifiers = new ArrayList<>(remainingUnappliedArgs.size()); - args = new ArrayList<>(remainingUnappliedArgs.size()); - for (String remainingUnappliedArg : remainingUnappliedArgs) { - IdentifierExpr arg = new IdentifierExpr(remainingUnappliedArg); - identifiers.add(arg); - args.add(arg); - } - lambdaExpr = new LambdaExpr(identifiers, newExpr); } - Expr magic = new ApplyFunctionExpr(fn, fn.name(), lambdaExpr, args); + final Expr magic = new ApplyFunctionExpr(fn, fn.name(), lambdaExpr, ImmutableList.copyOf(args)); return magic; } @@ -251,26 +266,37 @@ private static ApplyFunctionExpr liftApplyLambda(ApplyFunctionExpr expr, List unappliedInThisApply = + Set unappliedInThisApply = unappliedArgs.stream() - .filter(u -> !expr.bindingDetails.getArrayVariables().contains(u)) - .collect(Collectors.toList()); + .filter(u -> !expr.bindingDetails.getArrayColumns().contains(u)) + .collect(Collectors.toSet()); + + List unappliedIdentifiers = + expr.bindingDetails + .getFreeVariables() + .stream() + .filter(x -> unappliedInThisApply.contains(x.getIdentifierBindingIfIdentifier())) + .map(IdentifierExpr::getIdentifierIfIdentifier) + .collect(Collectors.toList()); List newArgs = new ArrayList<>(); for (int i = 0; i < expr.argsExpr.size(); i++) { - newArgs.add(applyUnappliedIdentifiers( - expr.argsExpr.get(i), - expr.argsBindingDetails.get(i), - unappliedInThisApply) + newArgs.add( + applyUnappliedIdentifiers( + expr.argsExpr.get(i), + expr.argsBindingDetails.get(i), + unappliedIdentifiers + ) ); } // this will _not_ include the lambda identifiers.. anything in this list needs to be applied - List unappliedLambdaBindings = expr.lambdaBindingDetails.getFreeVariables() - .stream() - .filter(unappliedArgs::contains) - .map(IdentifierExpr::new) - .collect(Collectors.toList()); + List unappliedLambdaBindings = + expr.lambdaBindingDetails.getFreeVariables() + .stream() + .filter(x -> unappliedArgs.contains(x.getIdentifierBindingIfIdentifier())) + .map(x -> new IdentifierExpr(x.getIdentifier(), x.getBindingIdentifier())) + .collect(Collectors.toList()); if (unappliedLambdaBindings.size() == 0) { return new ApplyFunctionExpr(expr.function, expr.name, expr.lambdaExpr, newArgs); @@ -353,7 +379,7 @@ private static ApplyFunctionExpr liftApplyLambda(ApplyFunctionExpr expr, List conflicted = - Sets.intersection(bindingDetails.getScalarVariables(), bindingDetails.getArrayVariables()); + Sets.intersection(bindingDetails.getScalarColumns(), bindingDetails.getArrayColumns()); if (conflicted.size() != 0) { throw new RE("Invalid expression: %s; %s used as both scalar and array variables", expression, conflicted); } 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 aa40eb51ccfa..03c488941889 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 @@ -75,6 +75,7 @@ public void testSimpleLogicalOps1() validateParser("x!=y", "(!= x y)", ImmutableList.of("x", "y")); validateParser("x && y", "(&& x y)", ImmutableList.of("x", "y")); validateParser("x || y", "(|| x y)", ImmutableList.of("x", "y")); + } @Test @@ -92,7 +93,7 @@ public void testSimpleAdditivityOp2() validateParser("x-y+z", "(+ (- x y) z)", ImmutableList.of("x", "y", "z")); validateParser("x-y-z", "(- (- x y) z)", ImmutableList.of("x", "y", "z")); - validateParser("x-y-x", "(- (- x y) x)", ImmutableList.of("x", "y")); + validateParser("x-y-x", "(- (- x y) x_0)", ImmutableList.of("x", "y"), ImmutableSet.of("x", "x_0", "y")); } @Test @@ -195,7 +196,7 @@ public void testLiteralArrays() public void testFunctions() { validateParser("sqrt(x)", "(sqrt [x])", ImmutableList.of("x")); - validateParser("if(cond,then,else)", "(if [cond, then, else])", ImmutableList.of("cond", "then", "else")); + validateParser("if(cond,then,else)", "(if [cond, then, else])", ImmutableList.of("else", "then", "cond")); validateParser("cast(x, 'STRING')", "(cast [x, STRING])", ImmutableList.of("x")); validateParser("cast(x, 'LONG')", "(cast [x, LONG])", ImmutableList.of("x")); validateParser("cast(x, 'DOUBLE')", "(cast [x, DOUBLE])", ImmutableList.of("x")); @@ -272,15 +273,15 @@ public void testApplyFunctions() ); validateParser( "x + map((x) -> x + 1, x)", - "(+ x (map ([x] -> (+ x 1)), [x]))", + "(+ x (map ([x] -> (+ x 1)), [x_0]))", ImmutableList.of("x"), ImmutableSet.of("x"), - ImmutableSet.of("x") + ImmutableSet.of("x_0") ); validateParser( "map((x) -> concat(x, y), z)", "(map ([x] -> (concat [x, y])), [z])", - ImmutableList.of("z", "y"), + ImmutableList.of("y", "z"), ImmutableSet.of("y"), ImmutableSet.of("z") ); @@ -303,14 +304,14 @@ public void testApplyFunctions() validateParser( "array_append(z, fold((x, acc) -> acc + x, map((x) -> x + 1, x), y))", "(array_append [z, (fold ([x, acc] -> (+ acc x)), [(map ([x] -> (+ x 1)), [x]), y])])", - ImmutableList.of("z", "x", "y"), + ImmutableList.of("x", "y", "z"), ImmutableSet.of(), ImmutableSet.of("x", "z") ); validateParser( "map(z -> z + 1, array_append(z, fold((x, acc) -> acc + x, map((x) -> x + 1, x), y)))", "(map ([z] -> (+ z 1)), [(array_append [z, (fold ([x, acc] -> (+ acc x)), [(map ([x] -> (+ x 1)), [x]), y])])])", - ImmutableList.of("z", "x", "y"), + ImmutableList.of("x", "y", "z"), ImmutableSet.of(), ImmutableSet.of("x", "z") ); @@ -318,7 +319,7 @@ public void testApplyFunctions() validateParser( "array_append(map(z -> z + 1, array_append(z, fold((x, acc) -> acc + x, map((x) -> x + 1, x), y))), a)", "(array_append [(map ([z] -> (+ z 1)), [(array_append [z, (fold ([x, acc] -> (+ acc x)), [(map ([x] -> (+ x 1)), [x]), y])])]), a])", - ImmutableList.of("z", "x", "y", "a"), + ImmutableList.of("a", "x", "y", "z"), ImmutableSet.of("a"), ImmutableSet.of("x", "z") ); @@ -400,6 +401,40 @@ public void testApplyUnapplied() ); } + @Test + public void testUniquify() + { + validateParser("x-x", "(- x x_0)", ImmutableList.of("x"), ImmutableSet.of("x", "x_0")); + validateParser( + "x - x + x", + "(+ (- x x_0) x_1)", + ImmutableList.of("x"), + ImmutableSet.of("x", "x_0", "x_1") + ); + + validateParser( + "map((x) -> x + x, x)", + "(map ([x] -> (+ x x)), [x])", + ImmutableList.of("x"), + ImmutableSet.of(), + ImmutableSet.of("x") + ); + + validateApplyUnapplied( + "x + x", + "(+ x x_0)", + "(cartesian_map ([x, x_0] -> (+ x x_0)), [x, x_0])", + ImmutableList.of("x") + ); + + validateApplyUnapplied( + "x + x + x", + "(+ (+ x x_0) x_1)", + "(cartesian_map ([x, x_0, x_1] -> (+ (+ x x_0) x_1)), [x, x_0, x_1])", + ImmutableList.of("x") + ); + } + private void validateFlatten(String expression, String withoutFlatten, String withFlatten) { @@ -428,7 +463,7 @@ private void validateParser( final Expr parsed = Parser.parse(expression, ExprMacroTable.nil()); final Expr.BindingDetails deets = parsed.analyzeInputs(); Assert.assertEquals(expression, expected, parsed.toString()); - Assert.assertEquals(expression, identifiers, deets.getRequiredColumns()); + Assert.assertEquals(expression, identifiers, deets.getRequiredColumnsList()); Assert.assertEquals(expression, scalars, deets.getScalarVariables()); Assert.assertEquals(expression, arrays, deets.getArrayVariables()); } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/AggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/AggregatorFactory.java index 6b0f4a11c948..3077e59c7afd 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/AggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/AggregatorFactory.java @@ -149,7 +149,7 @@ public AggregatorFactory getMergingFactory(AggregatorFactory other) throws Aggre * "transfer" values from this aggreagtor to an incremental index that the outer query will run on. This method * only exists due to the design of GroupByStrategyV1, and should probably not be used for anything else. If you are * here because you are looking for a way to get the input fields required by this aggregator, and thought - * "getRequiredColumns" sounded right, please use {@link #requiredFields()} instead. + * "getRequiredColumnsList" sounded right, please use {@link #requiredFields()} instead. * * @return AggregatorFactories that can be used to "transfer" values from this aggregator into an incremental index * diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/SimpleDoubleAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/SimpleDoubleAggregatorFactory.java index d20b07ac139b..6995d57857e5 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/SimpleDoubleAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/SimpleDoubleAggregatorFactory.java @@ -121,7 +121,7 @@ public List requiredFields() { return fieldName != null ? Collections.singletonList(fieldName) - : fieldExpression.get().analyzeInputs().getRequiredColumns(); + : fieldExpression.get().analyzeInputs().getRequiredColumnsList(); } @Override diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/SimpleFloatAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/SimpleFloatAggregatorFactory.java index 92dbc972f77c..6619126cedd9 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/SimpleFloatAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/SimpleFloatAggregatorFactory.java @@ -115,7 +115,7 @@ public List requiredFields() { return fieldName != null ? Collections.singletonList(fieldName) - : fieldExpression.get().analyzeInputs().getRequiredColumns(); + : fieldExpression.get().analyzeInputs().getRequiredColumnsList(); } @Override diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/SimpleLongAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/SimpleLongAggregatorFactory.java index 3a77e3ce7e29..308100c0516b 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/SimpleLongAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/SimpleLongAggregatorFactory.java @@ -111,7 +111,7 @@ public List requiredFields() { return fieldName != null ? Collections.singletonList(fieldName) - : fieldExpression.get().analyzeInputs().getRequiredColumns(); + : fieldExpression.get().analyzeInputs().getRequiredColumnsList(); } @Override diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/post/ExpressionPostAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/post/ExpressionPostAggregator.java index 84421953b867..281356c66caf 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/post/ExpressionPostAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/post/ExpressionPostAggregator.java @@ -118,7 +118,7 @@ private ExpressionPostAggregator( macroTable, finalizers, parsed, - Suppliers.memoize(() -> parsed.get().analyzeInputs().getFreeVariables())); + Suppliers.memoize(() -> parsed.get().analyzeInputs().getRequiredColumns())); } private ExpressionPostAggregator( 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 e3b49db8ca1a..c7546a5e823e 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 @@ -19,15 +19,14 @@ package org.apache.druid.query.expression; +import com.google.common.collect.ImmutableSet; import org.apache.druid.java.util.common.IAE; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprEval; import org.apache.druid.math.expr.ExprMacroTable; import javax.annotation.Nonnull; -import java.util.HashSet; import java.util.List; -import java.util.Set; public abstract class TrimExprMacro implements ExprMacroTable.ExprMacro { @@ -239,16 +238,9 @@ public Expr visit(Shuttle shuttle) @Override public BindingDetails analyzeInputs() { - final String stringIdentifier = stringExpr.getIdentifierIfIdentifier(); - final Set scalars = new HashSet<>(); - if (stringIdentifier != null) { - scalars.add(stringIdentifier); - } - final String charsIdentifier = charsExpr.getIdentifierIfIdentifier(); - if (charsIdentifier != null) { - scalars.add(charsIdentifier); - } - return stringExpr.analyzeInputs().merge(charsExpr.analyzeInputs()).mergeWithScalars(scalars); + return stringExpr.analyzeInputs() + .with(charsExpr) + .withScalarArguments(ImmutableSet.of(stringExpr, charsExpr)); } } diff --git a/processing/src/main/java/org/apache/druid/query/filter/ExpressionDimFilter.java b/processing/src/main/java/org/apache/druid/query/filter/ExpressionDimFilter.java index 4e731e0c4b5e..0a67652ff6e6 100644 --- a/processing/src/main/java/org/apache/druid/query/filter/ExpressionDimFilter.java +++ b/processing/src/main/java/org/apache/druid/query/filter/ExpressionDimFilter.java @@ -77,7 +77,7 @@ public RangeSet getDimensionRangeSet(final String dimension) @Override public HashSet getRequiredColumns() { - return Sets.newHashSet(parsed.get().analyzeInputs().getFreeVariables()); + return Sets.newHashSet(parsed.get().analyzeInputs().getRequiredColumns()); } @Override diff --git a/processing/src/main/java/org/apache/druid/segment/filter/ExpressionFilter.java b/processing/src/main/java/org/apache/druid/segment/filter/ExpressionFilter.java index 5816c0ce0d61..f46cb8db4f33 100644 --- a/processing/src/main/java/org/apache/druid/segment/filter/ExpressionFilter.java +++ b/processing/src/main/java/org/apache/druid/segment/filter/ExpressionFilter.java @@ -48,7 +48,7 @@ public class ExpressionFilter implements Filter public ExpressionFilter(final Supplier expr) { this.expr = expr; - this.requiredBindings = Suppliers.memoize(() -> expr.get().analyzeInputs().getFreeVariables()); + this.requiredBindings = Suppliers.memoize(() -> expr.get().analyzeInputs().getRequiredColumns()); } @Override diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java index 0ef0b48ee79f..5078c04f15b3 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java @@ -142,7 +142,7 @@ public static ColumnValueSelector makeExprEvalSelector( { final Expr.BindingDetails exprDetails = expression.analyzeInputs(); Parser.validateExpr(expression, exprDetails); - final List columns = exprDetails.getRequiredColumns(); + final List columns = exprDetails.getRequiredColumnsList(); if (columns.size() == 1) { final String column = Iterables.getOnlyElement(columns); @@ -219,7 +219,7 @@ public static DimensionSelector makeDimensionSelector( { final Expr.BindingDetails exprDetails = expression.analyzeInputs(); Parser.validateExpr(expression, exprDetails); - final List columns = exprDetails.getRequiredColumns(); + final List columns = exprDetails.getRequiredColumnsList(); if (columns.size() == 1) { @@ -355,7 +355,7 @@ private static Expr.ObjectBinding createBindings( ) { final Map> suppliers = new HashMap<>(); - final List columns = bindingDetails.getRequiredColumns(); + final List columns = bindingDetails.getRequiredColumnsList(); for (String columnName : columns) { final ColumnCapabilities columnCapabilities = columnSelectorFactory .getColumnCapabilities(columnName); diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionVirtualColumn.java b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionVirtualColumn.java index d71ac28f7db2..871682d4982c 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionVirtualColumn.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionVirtualColumn.java @@ -111,7 +111,7 @@ public ColumnCapabilities capabilities(String columnName) @Override public List requiredColumns() { - return parsedExpression.get().analyzeInputs().getRequiredColumns(); + return parsedExpression.get().analyzeInputs().getRequiredColumnsList(); } @Override diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/SingleLongInputCachingExpressionColumnValueSelector.java b/processing/src/main/java/org/apache/druid/segment/virtual/SingleLongInputCachingExpressionColumnValueSelector.java index af71a9979b1e..8fb9cdc91321 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/SingleLongInputCachingExpressionColumnValueSelector.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/SingleLongInputCachingExpressionColumnValueSelector.java @@ -59,7 +59,7 @@ public SingleLongInputCachingExpressionColumnValueSelector( ) { // Verify expression has just one binding. - if (expression.analyzeInputs().getFreeVariables().size() != 1) { + if (expression.analyzeInputs().getRequiredColumns().size() != 1) { throw new ISE("WTF?! Expected expression with just one binding"); } diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/SingleStringInputCachingExpressionColumnValueSelector.java b/processing/src/main/java/org/apache/druid/segment/virtual/SingleStringInputCachingExpressionColumnValueSelector.java index 4d358e08a90b..d9581e798a85 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/SingleStringInputCachingExpressionColumnValueSelector.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/SingleStringInputCachingExpressionColumnValueSelector.java @@ -54,7 +54,7 @@ public SingleStringInputCachingExpressionColumnValueSelector( ) { // Verify expression has just one binding. - if (expression.analyzeInputs().getFreeVariables().size() != 1) { + if (expression.analyzeInputs().getRequiredColumns().size() != 1) { throw new ISE("WTF?! Expected expression with just one binding"); } diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/SingleStringInputDimensionSelector.java b/processing/src/main/java/org/apache/druid/segment/virtual/SingleStringInputDimensionSelector.java index 275869a7b636..fb372ddff930 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/SingleStringInputDimensionSelector.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/SingleStringInputDimensionSelector.java @@ -55,7 +55,7 @@ public SingleStringInputDimensionSelector( ) { // Verify expression has just one binding. - if (expression.analyzeInputs().getFreeVariables().size() != 1) { + if (expression.analyzeInputs().getRequiredColumns().size() != 1) { throw new ISE("WTF?! Expected expression with just one binding"); } diff --git a/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java b/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java index 4ff31b585a03..671ea30d1c95 100644 --- a/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java +++ b/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java @@ -47,6 +47,8 @@ import org.apache.druid.query.groupby.GroupByQueryConfig; import org.apache.druid.query.groupby.GroupByQueryRunnerTest; import org.apache.druid.query.groupby.GroupByQueryRunnerTestHelper; +import org.apache.druid.query.groupby.orderby.DefaultLimitSpec; +import org.apache.druid.query.groupby.orderby.OrderByColumnSpec; import org.apache.druid.query.groupby.strategy.GroupByStrategySelector; import org.apache.druid.query.spec.LegacySegmentSpec; import org.apache.druid.query.topn.TopNQuery; @@ -571,6 +573,52 @@ public void testGroupByExpressionMultiMultiAutoAuto() TestHelper.assertExpectedObjects(expectedResults, result.toList(), "expr-multi-multi-auto-auto"); } + @Test + public void testGroupByExpressionMultiMultiAutoAutoDupeIdentifier() + { + if (config.getDefaultStrategy().equals(GroupByStrategySelector.STRATEGY_V1)) { + expectedException.expect(RuntimeException.class); + expectedException.expectMessage("GroupBy v1 does not support dimension selectors with unknown cardinality."); + } + GroupByQuery query = GroupByQuery + .builder() + .setDataSource("xx") + .setQuerySegmentSpec(new LegacySegmentSpec("1970/3000")) + .setGranularity(Granularities.ALL) + .setDimensions(new DefaultDimensionSpec("texpr", "texpr")) + .setVirtualColumns( + new ExpressionVirtualColumn( + "texpr", + "concat(tags, tags)", + ValueType.STRING, + TestExprMacroTable.INSTANCE + ) + ) + .setLimitSpec(new DefaultLimitSpec(ImmutableList.of(new OrderByColumnSpec("count", OrderByColumnSpec.Direction.DESCENDING)), 5)) + .setAggregatorSpecs(new CountAggregatorFactory("count")) + .setContext(context) + .build(); + + Sequence result = helper.runQueryOnSegmentsObjs( + ImmutableList.of( + new QueryableIndexSegment(queryableIndex, SegmentId.dummy("sid1")), + new IncrementalIndexSegment(incrementalIndex, SegmentId.dummy("sid2")) + ), + query + ); + + List expectedResults = Arrays.asList( + GroupByQueryRunnerTestHelper.createExpectedRow("1970-01-01T00:00:00.000Z", "texpr", "t3t3", "count", 4L), + GroupByQueryRunnerTestHelper.createExpectedRow("1970-01-01T00:00:00.000Z", "texpr", "t5t5", "count", 4L), + GroupByQueryRunnerTestHelper.createExpectedRow("1970-01-01T00:00:00.000Z", "texpr", "t2t1", "count", 2L), + GroupByQueryRunnerTestHelper.createExpectedRow("1970-01-01T00:00:00.000Z", "texpr", "t1t2", "count", 2L), + GroupByQueryRunnerTestHelper.createExpectedRow("1970-01-01T00:00:00.000Z", "texpr", "t7t7", "count", 2L) + ); + + System.out.println(result.toList()); + TestHelper.assertExpectedObjects(expectedResults, result.toList(), "expr-multi-multi-auto-auto-self"); + } + @Test public void testGroupByExpressionMultiMultiAutoAutoWithFilter() { @@ -890,7 +938,7 @@ public void testGroupByExpressionMultiConflicting() { expectedException.expect(RuntimeException.class); expectedException.expectMessage( - "Invalid expression: (concat [(map ([x] -> (concat [x, othertags])), [tags]), tags]); [tags] used as both scalar and array variables" + "Invalid expression: (concat [(map ([x] -> (concat [x, othertags])), [tags]), tags_0]); [tags] used as both scalar and array variables" ); GroupByQuery query = GroupByQuery .builder() @@ -925,7 +973,7 @@ public void testGroupByExpressionMultiConflictingAlso() { expectedException.expect(RuntimeException.class); expectedException.expectMessage( - "Invalid expression: (array_concat [tags, (array_append [othertags, tags])]); [tags] used as both scalar and array variables" + "Invalid expression: (array_concat [tags, (array_append [othertags, tags_0])]); [tags] used as both scalar and array variables" ); GroupByQuery query = GroupByQuery .builder() diff --git a/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java b/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java index a29cc6ea1549..4e721dac20c8 100644 --- a/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java +++ b/processing/src/test/java/org/apache/druid/segment/virtual/ExpressionVirtualColumnTest.java @@ -588,7 +588,7 @@ public void testRequiredColumns() Assert.assertEquals(ImmutableList.of("x", "y"), X_PLUS_Y.requiredColumns()); Assert.assertEquals(ImmutableList.of(), CONSTANT_LIKE.requiredColumns()); Assert.assertEquals(ImmutableList.of("z"), Z_LIKE.requiredColumns()); - Assert.assertEquals(ImmutableList.of("z", "x"), Z_CONCAT_X.requiredColumns()); + Assert.assertEquals(ImmutableList.of("x", "z"), Z_CONCAT_X.requiredColumns()); } @Test diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index f28c5b299ac9..0c67b06ea321 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -7953,6 +7953,44 @@ public void testTimestampCeil() throws Exception ); } + @Test + public void testNvlColumns() throws Exception + { + testQuery( + "SELECT NVL(dim2, dim1), COUNT(*) FROM druid.foo GROUP BY NVL(dim2, dim1)\n", + ImmutableList.of( + GroupByQuery.builder() + .setDataSource(CalciteTests.DATASOURCE1) + .setInterval(querySegmentSpec(Filtration.eternity())) + .setGranularity(Granularities.ALL) + .setVirtualColumns( + expressionVirtualColumn( + "v0", + "case_searched(notnull(\"dim2\"),\"dim2\",\"dim1\")", + ValueType.STRING + ) + ) + .setDimensions(dimensions(new DefaultDimensionSpec("v0", "v0", ValueType.STRING))) + .setAggregatorSpecs(aggregators(new CountAggregatorFactory("a0"))) + .setContext(QUERY_CONTEXT_DEFAULT) + .build() + ), + NullHandling.replaceWithDefault() ? + ImmutableList.of( + new Object[]{"10.1", 1L}, + new Object[]{"2", 1L}, + new Object[]{"a", 2L}, + new Object[]{"abc", 2L} + ) : + ImmutableList.of( + new Object[]{"", 1L}, + new Object[]{"10.1", 1L}, + new Object[]{"a", 2L}, + new Object[]{"abc", 2L} + ) + ); + } + @Test public void testMultiValueStringWorksLikeStringGroupBy() throws Exception { @@ -8004,6 +8042,8 @@ public void testMultiValueStringWorksLikeStringGroupBy() throws Exception ); } + + @Test public void testMultiValueStringWorksLikeStringGroupByWithFilter() throws Exception { @@ -8049,14 +8089,14 @@ public void testMultiValueStringWorksLikeStringScan() throws Exception "SELECT concat(dim3, 'foo') FROM druid.numfoo", ImmutableList.of( new Druids.ScanQueryBuilder() - .dataSource(CalciteTests.DATASOURCE3) - .intervals(querySegmentSpec(Filtration.eternity())) - .virtualColumns(expressionVirtualColumn("v0", "concat(\"dim3\",'foo')", ValueType.STRING)) - .columns(ImmutableList.of("v0")) - .context(QUERY_CONTEXT_DEFAULT) - .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST) - .legacy(false) - .build() + .dataSource(CalciteTests.DATASOURCE3) + .intervals(querySegmentSpec(Filtration.eternity())) + .virtualColumns(expressionVirtualColumn("v0", "concat(\"dim3\",'foo')", ValueType.STRING)) + .columns(ImmutableList.of("v0")) + .context(QUERY_CONTEXT_DEFAULT) + .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST) + .legacy(false) + .build() ), ImmutableList.of( new Object[]{"[\"afoo\",\"bfoo\"]"}, @@ -8070,16 +8110,16 @@ public void testMultiValueStringWorksLikeStringScan() throws Exception } @Test - public void testMultiValueStringWorksLikeStringScanWithFilter() throws Exception + public void testMultiValueStringWorksLikeStringSelfConcatScan() throws Exception { + final String nullVal = NullHandling.replaceWithDefault() ? "[\"-lol-\"]" : "[null]"; testQuery( - "SELECT concat(dim3, 'foo') FROM druid.numfoo where concat(dim3, 'foo') = 'bfoo'", + "SELECT concat(dim3, '-lol-', dim3) FROM druid.numfoo", ImmutableList.of( new Druids.ScanQueryBuilder() .dataSource(CalciteTests.DATASOURCE3) .intervals(querySegmentSpec(Filtration.eternity())) - .virtualColumns(expressionVirtualColumn("v0", "concat(\"dim3\",'foo')", ValueType.STRING)) - .filters(selector("v0", "bfoo", null)) + .virtualColumns(expressionVirtualColumn("v0", "concat(\"dim3\",'-lol-',\"dim3\")", ValueType.STRING)) .columns(ImmutableList.of("v0")) .context(QUERY_CONTEXT_DEFAULT) .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST) @@ -8087,46 +8127,36 @@ public void testMultiValueStringWorksLikeStringScanWithFilter() throws Exception .build() ), ImmutableList.of( - new Object[]{"[\"afoo\",\"bfoo\"]"}, - new Object[]{"[\"bfoo\",\"cfoo\"]"} + new Object[]{"[\"a-lol-a\",\"a-lol-b\",\"b-lol-a\",\"b-lol-b\"]"}, + new Object[]{"[\"b-lol-b\",\"b-lol-c\",\"c-lol-b\",\"c-lol-c\"]"}, + new Object[]{"[\"d-lol-d\"]"}, + new Object[]{"[\"-lol-\"]"}, + new Object[]{nullVal}, + new Object[]{nullVal} ) ); } @Test - public void testNvlColumns() throws Exception + public void testMultiValueStringWorksLikeStringScanWithFilter() throws Exception { testQuery( - "SELECT NVL(dim2, dim1), COUNT(*) FROM druid.foo GROUP BY NVL(dim2, dim1)\n", + "SELECT concat(dim3, 'foo') FROM druid.numfoo where concat(dim3, 'foo') = 'bfoo'", ImmutableList.of( - GroupByQuery.builder() - .setDataSource(CalciteTests.DATASOURCE1) - .setInterval(querySegmentSpec(Filtration.eternity())) - .setGranularity(Granularities.ALL) - .setVirtualColumns( - expressionVirtualColumn( - "v0", - "case_searched(notnull(\"dim2\"),\"dim2\",\"dim1\")", - ValueType.STRING - ) - ) - .setDimensions(dimensions(new DefaultDimensionSpec("v0", "v0", ValueType.STRING))) - .setAggregatorSpecs(aggregators(new CountAggregatorFactory("a0"))) - .setContext(QUERY_CONTEXT_DEFAULT) - .build() + new Druids.ScanQueryBuilder() + .dataSource(CalciteTests.DATASOURCE3) + .intervals(querySegmentSpec(Filtration.eternity())) + .virtualColumns(expressionVirtualColumn("v0", "concat(\"dim3\",'foo')", ValueType.STRING)) + .filters(selector("v0", "bfoo", null)) + .columns(ImmutableList.of("v0")) + .context(QUERY_CONTEXT_DEFAULT) + .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST) + .legacy(false) + .build() ), - NullHandling.replaceWithDefault() ? ImmutableList.of( - new Object[]{"10.1", 1L}, - new Object[]{"2", 1L}, - new Object[]{"a", 2L}, - new Object[]{"abc", 2L} - ) : - ImmutableList.of( - new Object[]{"", 1L}, - new Object[]{"10.1", 1L}, - new Object[]{"a", 2L}, - new Object[]{"abc", 2L} + new Object[]{"[\"afoo\",\"bfoo\"]"}, + new Object[]{"[\"bfoo\",\"cfoo\"]"} ) ); } @@ -8270,7 +8300,6 @@ public void testMultiValueStringContainsFilter() throws Exception @Test public void testMultiValueStringSlice() throws Exception { - final String nullVal = NullHandling.replaceWithDefault() ? "[\"foo\"]" : "[null]"; testQuery( "SELECT MV_SLICE(dim3, 1) FROM druid.numfoo", ImmutableList.of( From 51f46c03d11c696786cdc1e7dc997ac999c152ae Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 3 Jul 2019 13:13:34 -0700 Subject: [PATCH 2/8] fixes --- .../druid/segment/virtual/ExpressionSelectors.java | 10 +++++----- .../virtual/RowBasedExpressionColumnValueSelector.java | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java index 5078c04f15b3..edd2aa623666 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java @@ -160,7 +160,7 @@ public static ColumnValueSelector makeExprEvalSelector( && capabilities.isDictionaryEncoded() && capabilities.isComplete() && !capabilities.hasMultipleValues() - && !exprDetails.getArrayVariables().contains(column)) { + && !exprDetails.getArrayColumns().contains(column)) { // Optimization for expressions that hit one string column and nothing else. return new SingleStringInputCachingExpressionColumnValueSelector( columnSelectorFactory.makeDimensionSelector(new DefaultDimensionSpec(column, column, ValueType.STRING)), @@ -176,7 +176,7 @@ public static ColumnValueSelector makeExprEvalSelector( final List needsApplied = columns.stream() - .filter(c -> actualArrays.contains(c) && !exprDetails.getArrayVariables().contains(c)) + .filter(c -> actualArrays.contains(c) && !exprDetails.getArrayColumns().contains(c)) .collect(Collectors.toList()); final Expr finalExpr; if (needsApplied.size() > 0) { @@ -231,7 +231,7 @@ public static DimensionSelector makeDimensionSelector( && capabilities.isDictionaryEncoded() && capabilities.isComplete() && !capabilities.hasMultipleValues() - && !exprDetails.getArrayVariables().contains(column) + && !exprDetails.getArrayColumns().contains(column) ) { // Optimization for dimension selectors that wrap a single underlying string column. return new SingleStringInputDimensionSelector( @@ -249,7 +249,7 @@ public static DimensionSelector makeDimensionSelector( final ColumnValueSelector baseSelector = makeExprEvalSelector(columnSelectorFactory, expression); final boolean multiVal = actualArrays.size() > 0 || - exprDetails.getArrayVariables().size() > 0 || + exprDetails.getArrayColumns().size() > 0 || unknownIfArrays.size() > 0; if (baseSelector instanceof ConstantExprEvalSelector) { @@ -530,7 +530,7 @@ private static Pair, Set> examineColumnSelectorFactoryArrays } else if ( !capabilities.isComplete() && capabilities.getType().equals(ValueType.STRING) && - !exprDetails.getArrayVariables().contains(column) + !exprDetails.getArrayColumns().contains(column) ) { unknownIfArrays.add(column); } diff --git a/processing/src/main/java/org/apache/druid/segment/virtual/RowBasedExpressionColumnValueSelector.java b/processing/src/main/java/org/apache/druid/segment/virtual/RowBasedExpressionColumnValueSelector.java index e34f26a606e7..719f664e625b 100644 --- a/processing/src/main/java/org/apache/druid/segment/virtual/RowBasedExpressionColumnValueSelector.java +++ b/processing/src/main/java/org/apache/druid/segment/virtual/RowBasedExpressionColumnValueSelector.java @@ -53,7 +53,7 @@ public RowBasedExpressionColumnValueSelector( { super(expression, bindings); this.unknownColumns = unknownColumnsSet.stream() - .filter(x -> !baseExprBindingDetails.getArrayVariables().contains(x)) + .filter(x -> !baseExprBindingDetails.getArrayColumns().contains(x)) .collect(Collectors.toList()); this.baseExprBindingDetails = baseExprBindingDetails; this.ignoredColumns = new HashSet<>(); From 768e94d2a458c2341152df23616818d63816e1c4 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 3 Jul 2019 14:08:02 -0700 Subject: [PATCH 3/8] more docs and test --- .../java/org/apache/druid/math/expr/Expr.java | 5 ++--- .../druid/math/expr/ExprListenerImpl.java | 11 ++++++++++- .../java/org/apache/druid/math/expr/Parser.java | 17 ++++++++++++----- .../org/apache/druid/math/expr/ParserTest.java | 8 ++++++++ 4 files changed, 32 insertions(+), 9 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 e3062266aaf7..8962b9558b85 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 @@ -226,7 +226,6 @@ public Set getArrayColumns() /** * Total set of 'free' identifiers of an {@link Expr}, that are not supplied by a {@link LambdaExpr} binding - * @return */ public Set getFreeVariables() { @@ -570,13 +569,13 @@ class IdentifierExpr implements Expr private final String identifier; private final String bindingIdentifier; - public IdentifierExpr(String value) + IdentifierExpr(String value) { this.identifier = value; this.bindingIdentifier = value; } - public IdentifierExpr(String identifier, String bindingIdentifier) + IdentifierExpr(String identifier, String bindingIdentifier) { this.identifier = identifier; this.bindingIdentifier = bindingIdentifier; 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 3373d7a9ce31..8edac557189e 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 @@ -423,7 +423,10 @@ public void exitEmptyArray(ExprParser.EmptyArrayContext ctx) * 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 * {@link IdentifierExpr#bindingIdentifier} value. {@link LambdaExpr} identifiers however, will always have - * {@link IdentifierExpr#identifier} be the same as {@link IdentifierExpr#bindingIdentifier}. + * {@link IdentifierExpr#identifier} be the same as {@link IdentifierExpr#bindingIdentifier} because they have + * synthetic bindings set at evaluation time. This is done to aid in analysis needed for the automatic expression + * translation which maps scalar expressions to multi-value inputs. See + * {@link Parser#applyUnappliedIdentifiers(Expr, Expr.BindingDetails, List)}} for additional details. */ private IdentifierExpr createIdentifierExpr(String binding) { @@ -438,6 +441,9 @@ private IdentifierExpr createIdentifierExpr(String binding) return new IdentifierExpr(binding); } + /** + * Remove double quotes from an identifier variable string, returning unqouted identifier + */ private static String sanitizeIdentifierString(String text) { if (text.charAt(0) == '"' && text.charAt(text.length() - 1) == '"') { @@ -446,6 +452,9 @@ private static String sanitizeIdentifierString(String text) return text; } + /** + * Remove single quote from a string literal, returning unquoted string value + */ private static String escapeStringLiteral(String text) { String unquoted = text.substring(1, text.length() - 1); 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 2b07cbaa097b..f1bef036acae 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 @@ -214,10 +214,13 @@ public static Expr applyUnappliedIdentifiers(Expr expr, Expr.BindingDetails bind return applyUnapplied(newExpr, remainingUnappliedArgs); } + /** + * translate an {@link Expr} into an {@link ApplyFunctionExpr} for {@link ApplyFunction.MapFunction} or + * {@link ApplyFunction.CartesianMapFunction} if there are multiple unbound arguments to be applied + */ private static Expr applyUnapplied(Expr expr, List unapplied) { // wrap an expression in either map or cartesian_map to apply any unapplied identifiers - final Map toReplace = new HashMap<>(); final List args = expr.analyzeInputs() .getFreeVariables() @@ -226,12 +229,16 @@ private static Expr applyUnapplied(Expr expr, List unapplied) .collect(Collectors.toList()); final List lambdaArgs = new ArrayList<>(); + + // construct lambda args from list of args to apply for (IdentifierExpr applyFnArg : args) { - IdentifierExpr lambdaRewrite = new IdentifierExpr(applyFnArg.getIdentifier(), applyFnArg.getIdentifier()); + IdentifierExpr lambdaRewrite = new IdentifierExpr(applyFnArg.getIdentifier()); lambdaArgs.add(lambdaRewrite); toReplace.put(applyFnArg, lambdaRewrite); } + // rewrite identifiers in the expression which will become the lambda body, so they match the lambda identifiers we + // are constructing Expr newExpr = expr.visit(childExpr -> { if (childExpr instanceof IdentifierExpr) { if (toReplace.containsKey(childExpr)) { @@ -264,7 +271,6 @@ private static Expr applyUnapplied(Expr expr, List unapplied) */ private static ApplyFunctionExpr liftApplyLambda(ApplyFunctionExpr expr, List unappliedArgs) { - // recursively evaluate arguments to ensure they are properly transformed into arrays as necessary Set unappliedInThisApply = unappliedArgs.stream() @@ -347,11 +353,12 @@ private static ApplyFunctionExpr liftApplyLambda(ApplyFunctionExpr expr, List acc + x + y + z, x, y, acc) => // cartesian_fold((x, y, z, acc) -> acc + x + y + z, x, y, z, acc) - final List newFoldArgs = new ArrayList<>(expr.argsExpr.size() + unappliedLambdaBindings.size()); + final List newFoldArgs = + new ArrayList<>(expr.argsExpr.size() + unappliedLambdaBindings.size()); final List newFoldLambdaIdentifiers = new ArrayList<>(expr.lambdaExpr.getIdentifiers().size() + unappliedLambdaBindings.size()); final List existingFoldLambdaIdentifiers = expr.lambdaExpr.getIdentifierExprs(); - // accumulator argument is last argument, slice it off when constructing new arg list and lambda args identifiers + // accumulator argument is last argument, slice it off when constructing new arg list and lambda args for (int i = 0; i < expr.argsExpr.size() - 1; i++) { newFoldArgs.add(expr.argsExpr.get(i)); newFoldLambdaIdentifiers.add(existingFoldLambdaIdentifiers.get(i)); 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 03c488941889..54a285fadb3f 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 @@ -433,6 +433,14 @@ public void testUniquify() "(cartesian_map ([x, x_0, x_1] -> (+ (+ x x_0) x_1)), [x, x_0, x_1])", ImmutableList.of("x") ); + + // heh + validateApplyUnapplied( + "x + x + x + y + y + y + y + z + z + z", + "(+ (+ (+ (+ (+ (+ (+ (+ (+ x x_0) x_1) y) y_2) y_3) y_4) z) z_5) z_6)", + "(cartesian_map ([x, x_0, x_1, y, y_2, y_3, y_4, z, z_5, z_6] -> (+ (+ (+ (+ (+ (+ (+ (+ (+ x x_0) x_1) y) y_2) y_3) y_4) z) z_5) z_6)), [x, x_0, x_1, y, y_2, y_3, y_4, z, z_5, z_6])", + ImmutableList.of("x", "y", "z") + ); } From f7df14020e3cd516581d62d506f3e74a81f67515 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 3 Jul 2019 15:58:21 -0700 Subject: [PATCH 4/8] revert unintended doc change --- .../org/apache/druid/query/aggregation/AggregatorFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/AggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/AggregatorFactory.java index 3077e59c7afd..6b0f4a11c948 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/AggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/AggregatorFactory.java @@ -149,7 +149,7 @@ public AggregatorFactory getMergingFactory(AggregatorFactory other) throws Aggre * "transfer" values from this aggreagtor to an incremental index that the outer query will run on. This method * only exists due to the design of GroupByStrategyV1, and should probably not be used for anything else. If you are * here because you are looking for a way to get the input fields required by this aggregator, and thought - * "getRequiredColumnsList" sounded right, please use {@link #requiredFields()} instead. + * "getRequiredColumns" sounded right, please use {@link #requiredFields()} instead. * * @return AggregatorFactories that can be used to "transfer" values from this aggregator into an incremental index * From 222c211ac5632f3c548849229b10b16347dead4f Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 3 Jul 2019 16:44:39 -0700 Subject: [PATCH 5/8] formatting --- .../java/org/apache/druid/sql/calcite/CalciteQueryTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index 0c67b06ea321..7ec81620afd0 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -8042,8 +8042,6 @@ public void testMultiValueStringWorksLikeStringGroupBy() throws Exception ); } - - @Test public void testMultiValueStringWorksLikeStringGroupByWithFilter() throws Exception { @@ -8324,7 +8322,6 @@ public void testMultiValueStringSlice() throws Exception ); } - @Test public void testMultiValueStringLength() throws Exception { From 3fcb7c5aa69f22615e9ee4a2061a79d6b932206f Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 3 Jul 2019 17:04:21 -0700 Subject: [PATCH 6/8] change tostring to print binding identifier --- .../java/org/apache/druid/math/expr/Expr.java | 2 +- .../apache/druid/math/expr/ParserTest.java | 20 +++++++++---------- .../druid/query/MultiValuedDimensionTest.java | 4 ++-- 3 files changed, 13 insertions(+), 13 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 8962b9558b85..0f2636af437c 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 @@ -584,7 +584,7 @@ class IdentifierExpr implements Expr @Override public String toString() { - return identifier; + return bindingIdentifier; } /** 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 54a285fadb3f..f70a7162e1d1 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 @@ -93,7 +93,7 @@ public void testSimpleAdditivityOp2() validateParser("x-y+z", "(+ (- x y) z)", ImmutableList.of("x", "y", "z")); validateParser("x-y-z", "(- (- x y) z)", ImmutableList.of("x", "y", "z")); - validateParser("x-y-x", "(- (- x y) x_0)", ImmutableList.of("x", "y"), ImmutableSet.of("x", "x_0", "y")); + validateParser("x-y-x", "(- (- x y) x)", ImmutableList.of("x", "y"), ImmutableSet.of("x", "x_0", "y")); } @Test @@ -273,7 +273,7 @@ public void testApplyFunctions() ); validateParser( "x + map((x) -> x + 1, x)", - "(+ x (map ([x] -> (+ x 1)), [x_0]))", + "(+ x (map ([x] -> (+ x 1)), [x]))", ImmutableList.of("x"), ImmutableSet.of("x"), ImmutableSet.of("x_0") @@ -404,10 +404,10 @@ public void testApplyUnapplied() @Test public void testUniquify() { - validateParser("x-x", "(- x x_0)", ImmutableList.of("x"), ImmutableSet.of("x", "x_0")); + validateParser("x-x", "(- x x)", ImmutableList.of("x"), ImmutableSet.of("x", "x_0")); validateParser( "x - x + x", - "(+ (- x x_0) x_1)", + "(+ (- x x) x)", ImmutableList.of("x"), ImmutableSet.of("x", "x_0", "x_1") ); @@ -422,23 +422,23 @@ public void testUniquify() validateApplyUnapplied( "x + x", - "(+ x x_0)", - "(cartesian_map ([x, x_0] -> (+ x x_0)), [x, x_0])", + "(+ x x)", + "(cartesian_map ([x, x_0] -> (+ x x_0)), [x, x])", ImmutableList.of("x") ); validateApplyUnapplied( "x + x + x", - "(+ (+ x x_0) x_1)", - "(cartesian_map ([x, x_0, x_1] -> (+ (+ x x_0) x_1)), [x, x_0, x_1])", + "(+ (+ x x) x)", + "(cartesian_map ([x, x_0, x_1] -> (+ (+ x x_0) x_1)), [x, x, x])", ImmutableList.of("x") ); // heh validateApplyUnapplied( "x + x + x + y + y + y + y + z + z + z", - "(+ (+ (+ (+ (+ (+ (+ (+ (+ x x_0) x_1) y) y_2) y_3) y_4) z) z_5) z_6)", - "(cartesian_map ([x, x_0, x_1, y, y_2, y_3, y_4, z, z_5, z_6] -> (+ (+ (+ (+ (+ (+ (+ (+ (+ x x_0) x_1) y) y_2) y_3) y_4) z) z_5) z_6)), [x, x_0, x_1, y, y_2, y_3, y_4, z, z_5, z_6])", + "(+ (+ (+ (+ (+ (+ (+ (+ (+ x x) x) y) y) y) y) z) z) z)", + "(cartesian_map ([x, x_0, x_1, y, y_2, y_3, y_4, z, z_5, z_6] -> (+ (+ (+ (+ (+ (+ (+ (+ (+ x x_0) x_1) y) y_2) y_3) y_4) z) z_5) z_6)), [x, x, x, y, y, y, y, z, z, z])", ImmutableList.of("x", "y", "z") ); } diff --git a/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java b/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java index 671ea30d1c95..616a49c48d80 100644 --- a/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java +++ b/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java @@ -938,7 +938,7 @@ public void testGroupByExpressionMultiConflicting() { expectedException.expect(RuntimeException.class); expectedException.expectMessage( - "Invalid expression: (concat [(map ([x] -> (concat [x, othertags])), [tags]), tags_0]); [tags] used as both scalar and array variables" + "Invalid expression: (concat [(map ([x] -> (concat [x, othertags])), [tags]), tags]); [tags] used as both scalar and array variables" ); GroupByQuery query = GroupByQuery .builder() @@ -973,7 +973,7 @@ public void testGroupByExpressionMultiConflictingAlso() { expectedException.expect(RuntimeException.class); expectedException.expectMessage( - "Invalid expression: (array_concat [tags, (array_append [othertags, tags_0])]); [tags] used as both scalar and array variables" + "Invalid expression: (array_concat [tags, (array_append [othertags, tags])]); [tags] used as both scalar and array variables" ); GroupByQuery query = GroupByQuery .builder() From 5ef86e9217e37a741c9ce64e96106768a86be879 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 3 Jul 2019 18:01:40 -0700 Subject: [PATCH 7/8] review fixup --- .../main/java/org/apache/druid/math/expr/Expr.java | 11 ++++++----- .../org/apache/druid/math/expr/ExprListenerImpl.java | 10 +++++----- .../main/java/org/apache/druid/math/expr/Parser.java | 2 +- 3 files changed, 12 insertions(+), 11 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 0f2636af437c..a9f1eef301c2 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,6 +19,7 @@ package org.apache.druid.math.expr; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -201,7 +202,7 @@ public List getRequiredColumnsList() } /** - * Get the set of required column inputs to evaluate an expression + * Get the set of required column inputs to evaluate an expression, {@link IdentifierExpr#bindingIdentifier} */ public Set getRequiredColumns() { @@ -209,7 +210,7 @@ public Set getRequiredColumns() } /** - * Set of identifiers which are used with scalar operators and functions + * Set of {@link IdentifierExpr#bindingIdentifier} which are used with scalar operators and functions */ public Set getScalarColumns() { @@ -217,7 +218,7 @@ public Set getScalarColumns() } /** - * Set of identifiers which are used with array typed functions and apply functions. + * Set of {@link IdentifierExpr#bindingIdentifier} which are used with array typed functions and apply functions. */ public Set getArrayColumns() { @@ -233,7 +234,7 @@ public Set getFreeVariables() } /** - * Set of identifiers which are used with scalar operators and functions + * Set of {@link IdentifierExpr#identifier} which are used with scalar operators and functions. */ public Set getScalarVariables() { @@ -241,7 +242,7 @@ public Set getScalarVariables() } /** - * Set of identifiers which are used with array typed functions and apply functions. + * Set of {@link IdentifierExpr#identifier} which are used with array typed functions and apply functions. */ public Set getArrayVariables() { 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 8edac557189e..b4c91a3436d3 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 @@ -47,7 +47,7 @@ public class ExprListenerImpl extends ExprBaseListener private final ExprMacroTable macroTable; private final ParseTree rootNodeKey; - private final Set lambadIdentifiers; + private final Set lambdaIdentifiers; private final Set uniqueIdentifiers; private int uniqueCounter = 0; @@ -56,7 +56,7 @@ public class ExprListenerImpl extends ExprBaseListener this.rootNodeKey = rootNodeKey; this.macroTable = macroTable; this.nodes = new HashMap<>(); - this.lambadIdentifiers = new HashSet<>(); + this.lambdaIdentifiers = new HashSet<>(); this.uniqueIdentifiers = new HashSet<>(); } @@ -367,7 +367,7 @@ public void enterLambda(ExprParser.LambdaContext ctx) for (int i = 0; i < ctx.IDENTIFIER().size(); i++) { String text = ctx.IDENTIFIER(i).getText(); text = sanitizeIdentifierString(text); - this.lambadIdentifiers.add(text); + this.lambdaIdentifiers.add(text); } } @@ -380,7 +380,7 @@ public void exitLambda(ExprParser.LambdaContext ctx) text = sanitizeIdentifierString(text); identifiers.add(i, createIdentifierExpr(text)); // clean up lambda identifier references on exit - lambadIdentifiers.remove(text); + lambdaIdentifiers.remove(text); } nodes.put(ctx, new LambdaExpr(identifiers, (Expr) nodes.get(ctx.expr()))); @@ -430,7 +430,7 @@ public void exitEmptyArray(ExprParser.EmptyArrayContext ctx) */ private IdentifierExpr createIdentifierExpr(String binding) { - if (!lambadIdentifiers.contains(binding)) { + if (!lambdaIdentifiers.contains(binding)) { String uniqueIdentifier = binding; while (uniqueIdentifiers.contains(uniqueIdentifier)) { uniqueIdentifier = StringUtils.format("%s_%s", binding, uniqueCounter++); 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 f1bef036acae..55144abaec85 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 @@ -171,7 +171,7 @@ public static Expr applyUnappliedIdentifiers(Expr expr, Expr.BindingDetails bind return expr; } List unapplied = toApply.stream() - .filter(x -> bindingDetails.getRequiredColumnsList().contains(x)) + .filter(x -> bindingDetails.getRequiredColumns().contains(x)) .collect(Collectors.toList()); // any unapplied identifiers that are inside a lambda expression need that lambda expression to be rewritten From 79d3101d1a20d1d3af15649485a991582166da0c Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 3 Jul 2019 18:14:37 -0700 Subject: [PATCH 8/8] oops --- core/src/main/java/org/apache/druid/math/expr/Expr.java | 1 - 1 file changed, 1 deletion(-) 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 a9f1eef301c2..07c20fafd5b5 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,7 +19,6 @@ package org.apache.druid.math.expr; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet;