From 0b22a312d7ab0e4f9f1d0f8abdf542b3d03ce1f7 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Tue, 9 Jul 2019 12:51:51 -0700 Subject: [PATCH 01/11] optimize single input column multi-value expressions --- .../apache/druid/math/expr/ApplyFunction.java | 17 ++ .../java/org/apache/druid/math/expr/Expr.java | 82 +++++++-- .../org/apache/druid/math/expr/Function.java | 160 +++++++++++++----- .../segment/virtual/ExpressionSelectors.java | 7 +- .../SingleStringInputDimensionSelector.java | 7 +- .../druid/query/MultiValuedDimensionTest.java | 4 - .../druid/segment/filter/BaseFilterTest.java | 85 ++++------ .../segment/filter/ExpressionFilterTest.java | 1 + .../druid/sql/calcite/CalciteQueryTest.java | 3 +- 9 files changed, 252 insertions(+), 114 deletions(-) diff --git a/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java b/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java index ac1d02b21cdd..501abc81b566 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java +++ b/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java @@ -60,6 +60,11 @@ public interface ApplyFunction */ Set getArrayInputs(List args); + default boolean hasArrayOutput() + { + return false; + } + void validateArguments(LambdaExpr lambdaExpr, List args); /** @@ -69,6 +74,12 @@ public interface ApplyFunction */ abstract class BaseMapFunction implements ApplyFunction { + @Override + public boolean hasArrayOutput() + { + return true; + } + /** * Evaluate {@link LambdaExpr} against every index position of an {@link IndexableMapLambdaObjectBinding} */ @@ -397,6 +408,12 @@ public String name() return NAME; } + @Override + public boolean hasArrayOutput() + { + return true; + } + @Override public ExprEval apply(LambdaExpr lambdaExpr, List argsExpr, Expr.ObjectBinding bindings) { 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 07c20fafd5b5..e6e4e5b066d8 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 @@ -170,26 +170,32 @@ class BindingDetails private final ImmutableSet freeVariables; private final ImmutableSet scalarVariables; private final ImmutableSet arrayVariables; + private final boolean hasInputArrays; + private final boolean isOutputArray; public BindingDetails() { - this(ImmutableSet.of(), ImmutableSet.of(), ImmutableSet.of()); + this(ImmutableSet.of(), ImmutableSet.of(), ImmutableSet.of(), false, false); } public BindingDetails(IdentifierExpr expr) { - this(ImmutableSet.of(expr), ImmutableSet.of(), ImmutableSet.of()); + this(ImmutableSet.of(expr), ImmutableSet.of(), ImmutableSet.of(), false, false); } public BindingDetails( ImmutableSet freeVariables, ImmutableSet scalarVariables, - ImmutableSet arrayVariables + ImmutableSet arrayVariables, + boolean hasInputArrays, + boolean isOutputArray ) { this.freeVariables = freeVariables; this.scalarVariables = scalarVariables; this.arrayVariables = arrayVariables; + this.hasInputArrays = hasInputArrays; + this.isOutputArray = isOutputArray; } /** @@ -197,7 +203,9 @@ public BindingDetails( */ public List getRequiredColumnsList() { - return new ArrayList<>(freeVariables.stream().map(IdentifierExpr::getIdentifierBindingIfIdentifier).collect(Collectors.toSet())); + return new ArrayList<>( + freeVariables.stream().map(IdentifierExpr::getIdentifierBindingIfIdentifier).collect(Collectors.toSet()) + ); } /** @@ -248,6 +256,19 @@ public Set getArrayVariables() return arrayVariables.stream().map(IdentifierExpr::getIdentifier).collect(Collectors.toSet()); } + public boolean hasInputArrays() + { + return hasInputArrays; + } + + /** + * Returns true if this expression evaluates to an array type + */ + public boolean isOutputArray() + { + return isOutputArray; + } + /** * Combine with {@link BindingDetails} from {@link Expr#analyzeInputs()} */ @@ -264,7 +285,9 @@ public BindingDetails with(BindingDetails other) return new BindingDetails( ImmutableSet.copyOf(Sets.union(freeVariables, other.freeVariables)), ImmutableSet.copyOf(Sets.union(scalarVariables, other.scalarVariables)), - ImmutableSet.copyOf(Sets.union(arrayVariables, other.arrayVariables)) + ImmutableSet.copyOf(Sets.union(arrayVariables, other.arrayVariables)), + hasInputArrays, + isOutputArray ); } @@ -284,7 +307,9 @@ public BindingDetails withScalarArguments(Set scalarArguments) return new BindingDetails( ImmutableSet.copyOf(Sets.union(freeVariables, moreScalars)), ImmutableSet.copyOf(Sets.union(scalarVariables, moreScalars)), - arrayVariables + arrayVariables, + hasInputArrays, + isOutputArray ); } @@ -304,7 +329,31 @@ public BindingDetails withArrayArguments(Set arrayArguments) return new BindingDetails( ImmutableSet.copyOf(Sets.union(freeVariables, arrayIdentifiers)), scalarVariables, - ImmutableSet.copyOf(Sets.union(arrayVariables, arrayIdentifiers)) + ImmutableSet.copyOf(Sets.union(arrayVariables, arrayIdentifiers)), + hasInputArrays, + isOutputArray + ); + } + + public BindingDetails withArrayInputs(boolean hasArrays) + { + return new BindingDetails( + freeVariables, + scalarVariables, + arrayVariables, + hasArrays, + isOutputArray + ); + } + + public BindingDetails withArrayOutput(boolean isOutputArray) + { + return new BindingDetails( + freeVariables, + scalarVariables, + arrayVariables, + hasInputArrays, + isOutputArray ); } @@ -315,9 +364,11 @@ public BindingDetails withArrayArguments(Set arrayArguments) 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()) + 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()), + hasInputArrays, + isOutputArray ); } } @@ -785,7 +836,9 @@ public BindingDetails analyzeInputs() accumulator = accumulator.with(arg); } return accumulator.withScalarArguments(function.getScalarInputs(args)) - .withArrayArguments(function.getArrayInputs(args)); + .withArrayArguments(function.getArrayInputs(args)) + .withArrayInputs(function.hasArrayInputs()) + .withArrayOutput(function.hasArrayOutput()); } } @@ -824,7 +877,12 @@ class ApplyFunctionExpr implements Expr } lambdaBindingDetails = lambdaExpr.analyzeInputs(); - bindingDetails = accumulator.with(lambdaBindingDetails).withArrayArguments(function.getArrayInputs(argsExpr)); + boolean isArrayOutput = function.hasArrayOutput() || (function instanceof ApplyFunction.BaseFoldFunction + && lambdaBindingDetails.isOutputArray()); + bindingDetails = accumulator.with(lambdaBindingDetails) + .withArrayArguments(function.getArrayInputs(argsExpr)) + .withArrayInputs(true) + .withArrayOutput(isArrayOutput); argsBindingDetails = argBindingDetailsBuilder.build(); } diff --git a/core/src/main/java/org/apache/druid/math/expr/Function.java b/core/src/main/java/org/apache/druid/math/expr/Function.java index 84c73e7ea1b6..751f885e2ca0 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Function.java +++ b/core/src/main/java/org/apache/druid/math/expr/Function.java @@ -76,6 +76,16 @@ default Set getArrayInputs(List args) return Collections.emptySet(); } + default boolean hasArrayInputs() + { + return false; + } + + default boolean hasArrayOutput() + { + return false; + } + /** * Validate function arguments */ @@ -235,6 +245,12 @@ public Set getArrayInputs(List args) return ImmutableSet.of(args.get(0)); } + @Override + public boolean hasArrayInputs() + { + return true; + } + @Override public ExprEval apply(List args, Expr.ObjectBinding bindings) { @@ -274,6 +290,12 @@ public Set getArrayInputs(List args) return ImmutableSet.copyOf(args); } + @Override + public boolean hasArrayInputs() + { + return true; + } + @Override public ExprEval apply(List args, Expr.ObjectBinding bindings) { @@ -1801,6 +1823,12 @@ static void setArrayOutputElement( } + @Override + public Set getScalarInputs(List args) + { + return ImmutableSet.copyOf(args); + } + @Override public Set getArrayInputs(List args) { @@ -1808,17 +1836,17 @@ public Set getArrayInputs(List args) } @Override - public void validateArguments(List args) + public boolean hasArrayOutput() { - if (!(args.size() > 0)) { - throw new IAE("Function[%s] needs at least 1 argument", name()); - } + return true; } @Override - public Set getScalarInputs(List args) + public void validateArguments(List args) { - return ImmutableSet.copyOf(args); + if (!(args.size() > 0)) { + throw new IAE("Function[%s] needs at least 1 argument", name()); + } } } @@ -1852,6 +1880,12 @@ public Set getArrayInputs(List args) return ImmutableSet.of(args.get(0)); } + @Override + public boolean hasArrayInputs() + { + return true; + } + @Override public void validateArguments(List args) { @@ -1901,6 +1935,12 @@ public Set getScalarInputs(List args) { return ImmutableSet.copyOf(args); } + + @Override + public boolean hasArrayOutput() + { + return true; + } } class ArrayToStringFunction extends ArrayScalarFunction @@ -2036,6 +2076,12 @@ public String name() return "array_append"; } + @Override + public boolean hasArrayOutput() + { + return true; + } + @Override ExprEval doApply(ExprEval arrayExpr, ExprEval scalarExpr) { @@ -2080,6 +2126,18 @@ public String name() return "array_concat"; } + @Override + public Set getArrayInputs(List args) + { + return ImmutableSet.copyOf(args); + } + + @Override + public boolean hasArrayOutput() + { + return true; + } + @Override ExprEval doApply(ExprEval lhsExpr, ExprEval rhsExpr) { @@ -2119,12 +2177,6 @@ private Stream cat(T[] array1, T[] array2) l.addAll(Arrays.asList(array2)); return l.stream(); } - - @Override - public Set getArrayInputs(List args) - { - return ImmutableSet.copyOf(args); - } } class ArrayContainsFunction extends ArraysFunction @@ -2135,6 +2187,12 @@ public String name() return "array_contains"; } + @Override + public boolean hasArrayOutput() + { + return true; + } + @Override ExprEval doApply(ExprEval lhsExpr, ExprEval rhsExpr) { @@ -2181,6 +2239,34 @@ public void validateArguments(List args) } } + @Override + public Set getScalarInputs(List args) + { + if (args.size() == 3) { + return ImmutableSet.of(args.get(1), args.get(2)); + } else { + return ImmutableSet.of(args.get(1)); + } + } + + @Override + public Set getArrayInputs(List args) + { + return ImmutableSet.of(args.get(0)); + } + + @Override + public boolean hasArrayInputs() + { + return true; + } + + @Override + public boolean hasArrayOutput() + { + return true; + } + @Override public ExprEval apply(List args, Expr.ObjectBinding bindings) { @@ -2214,38 +2300,46 @@ public ExprEval apply(List args, Expr.ObjectBinding bindings) } throw new RE("Unable to slice to unknown type %s", expr.type()); } + } + class ArrayPrependFunction implements Function + { @Override - public Set getScalarInputs(List args) + public String name() { - if (args.size() == 3) { - return ImmutableSet.of(args.get(1), args.get(2)); - } else { - return ImmutableSet.of(args.get(1)); + return "array_prepend"; + } + + @Override + public void validateArguments(List args) + { + if (args.size() != 2) { + throw new IAE("Function[%s] needs 2 arguments", name()); } } @Override - public Set getArrayInputs(List args) + public Set getScalarInputs(List args) { return ImmutableSet.of(args.get(0)); } - } - class ArrayPrependFunction implements Function - { @Override - public String name() + public Set getArrayInputs(List args) { - return "array_prepend"; + return ImmutableSet.of(args.get(1)); } @Override - public void validateArguments(List args) + public boolean hasArrayInputs() { - if (args.size() != 2) { - throw new IAE("Function[%s] needs 2 arguments", name()); - } + return true; + } + + @Override + public boolean hasArrayOutput() + { + return true; } @Override @@ -2280,23 +2374,11 @@ public ExprEval apply(List args, Expr.ObjectBinding bindings) throw new RE("Unable to prepend to unknown type %s", arrayExpr.type()); } - private Stream prepend(T val, T[] array) { List l = new ArrayList<>(Arrays.asList(array)); l.add(0, val); return l.stream(); } - @Override - public Set getScalarInputs(List args) - { - return ImmutableSet.of(args.get(0)); - } - - @Override - public Set getArrayInputs(List args) - { - return ImmutableSet.of(args.get(1)); - } } } 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 edd2aa623666..b77c14064b0b 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.getArrayColumns().contains(column)) { + && exprDetails.getArrayColumns().size() == 0) { // Optimization for expressions that hit one string column and nothing else. return new SingleStringInputCachingExpressionColumnValueSelector( columnSelectorFactory.makeDimensionSelector(new DefaultDimensionSpec(column, column, ValueType.STRING)), @@ -230,8 +230,9 @@ public static DimensionSelector makeDimensionSelector( && capabilities.getType() == ValueType.STRING && capabilities.isDictionaryEncoded() && capabilities.isComplete() - && !capabilities.hasMultipleValues() - && !exprDetails.getArrayColumns().contains(column) + && !exprDetails.hasInputArrays() + && !exprDetails.isOutputArray() + && (!capabilities.hasMultipleValues() || exprDetails.getFreeVariables().size() == 1) ) { // Optimization for dimension selectors that wrap a single underlying string column. return new SingleStringInputDimensionSelector( 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 d6462b148522..a21f5ff7527e 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 @@ -70,15 +70,12 @@ public void inspectRuntimeShape(final RuntimeShapeInspector inspector) } /** - * Get the underlying selector {@link IndexedInts} row, or the null adjusted row. + * Get the underlying selector {@link IndexedInts} row */ @Override public IndexedInts getRow() { - final IndexedInts row = selector.getRow(); - - assert row.size() <= 1; - return row; + return selector.getRow(); } @Override 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 616a49c48d80..12d18ac48c2d 100644 --- a/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java +++ b/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java @@ -668,10 +668,6 @@ public void testGroupByExpressionMultiMultiAutoAutoWithFilter() @Test public void testGroupByExpressionAuto() { - 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") diff --git a/processing/src/test/java/org/apache/druid/segment/filter/BaseFilterTest.java b/processing/src/test/java/org/apache/druid/segment/filter/BaseFilterTest.java index 9200ec48b921..611e8d43ad45 100644 --- a/processing/src/test/java/org/apache/druid/segment/filter/BaseFilterTest.java +++ b/processing/src/test/java/org/apache/druid/segment/filter/BaseFilterTest.java @@ -264,26 +264,21 @@ private List selectColumnValuesMatchingFilter(final DimFilter filter, fi final Sequence cursors = makeCursorSequence(makeFilter(filter)); Sequence> seq = Sequences.map( cursors, - new Function>() - { - @Override - public List apply(Cursor input) - { - final DimensionSelector selector = input - .getColumnSelectorFactory() - .makeDimensionSelector(new DefaultDimensionSpec(selectColumn, selectColumn)); - - final List values = new ArrayList<>(); - - while (!input.isDone()) { - IndexedInts row = selector.getRow(); - Preconditions.checkState(row.size() == 1); - values.add(selector.lookupName(row.get(0))); - input.advance(); - } + input -> { + final DimensionSelector selector = input + .getColumnSelectorFactory() + .makeDimensionSelector(new DefaultDimensionSpec(selectColumn, selectColumn)); + + final List values = new ArrayList<>(); - return values; + while (!input.isDone()) { + IndexedInts row = selector.getRow(); + Preconditions.checkState(row.size() == 1); + values.add(selector.lookupName(row.get(0))); + input.advance(); } + + return values; } ); return seq.toList().get(0); @@ -294,22 +289,17 @@ private long selectCountUsingFilteredAggregator(final DimFilter filter) final Sequence cursors = makeCursorSequence(makeFilter(filter)); Sequence aggSeq = Sequences.map( cursors, - new Function() - { - @Override - public Aggregator apply(Cursor input) - { - Aggregator agg = new FilteredAggregatorFactory( - new CountAggregatorFactory("count"), - maybeOptimize(filter) - ).factorize(input.getColumnSelectorFactory()); - - for (; !input.isDone(); input.advance()) { - agg.aggregate(); - } + input -> { + Aggregator agg = new FilteredAggregatorFactory( + new CountAggregatorFactory("count"), + maybeOptimize(filter) + ).factorize(input.getColumnSelectorFactory()); - return agg; + for (; !input.isDone(); input.advance()) { + agg.aggregate(); } + + return agg; } ); return aggSeq.toList().get(0).getLong(); @@ -357,26 +347,21 @@ public double estimateSelectivity(BitmapIndexSelector indexSelector) final Sequence cursors = makeCursorSequence(postFilteringFilter); Sequence> seq = Sequences.map( cursors, - new Function>() - { - @Override - public List apply(Cursor input) - { - final DimensionSelector selector = input - .getColumnSelectorFactory() - .makeDimensionSelector(new DefaultDimensionSpec(selectColumn, selectColumn)); - - final List values = new ArrayList<>(); - - while (!input.isDone()) { - IndexedInts row = selector.getRow(); - Preconditions.checkState(row.size() == 1); - values.add(selector.lookupName(row.get(0))); - input.advance(); - } + input -> { + final DimensionSelector selector = input + .getColumnSelectorFactory() + .makeDimensionSelector(new DefaultDimensionSpec(selectColumn, selectColumn)); - return values; + final List values = new ArrayList<>(); + + while (!input.isDone()) { + IndexedInts row = selector.getRow(); + Preconditions.checkState(row.size() == 1); + values.add(selector.lookupName(row.get(0))); + input.advance(); } + + return values; } ); return seq.toList().get(0); diff --git a/processing/src/test/java/org/apache/druid/segment/filter/ExpressionFilterTest.java b/processing/src/test/java/org/apache/druid/segment/filter/ExpressionFilterTest.java index 91b89652392e..70080f8a0b1a 100644 --- a/processing/src/test/java/org/apache/druid/segment/filter/ExpressionFilterTest.java +++ b/processing/src/test/java/org/apache/druid/segment/filter/ExpressionFilterTest.java @@ -147,6 +147,7 @@ public void testOneMultiValuedStringColumn() } assertFilterMatches(edf("dim4 == '1'"), ImmutableList.of("0")); assertFilterMatches(edf("dim4 == '3'"), ImmutableList.of("3")); + assertFilterMatches(edf("dim4 == '4'"), ImmutableList.of("4", "5")); } @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 7ec81620afd0..2ebc3019caa7 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 @@ -7997,8 +7997,9 @@ public void testMultiValueStringWorksLikeStringGroupBy() throws Exception List expected; if (NullHandling.replaceWithDefault()) { expected = ImmutableList.of( - new Object[]{"foo", 3L}, new Object[]{"bfoo", 2L}, + new Object[]{"foo", 2L}, + new Object[]{"", 1L}, new Object[]{"afoo", 1L}, new Object[]{"cfoo", 1L}, new Object[]{"dfoo", 1L} From 102b23f4708c435132f0ea240d88da94e40a5169 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Tue, 9 Jul 2019 13:22:05 -0700 Subject: [PATCH 02/11] javadocs --- .../apache/druid/math/expr/ApplyFunction.java | 6 ++++++ .../java/org/apache/druid/math/expr/Expr.java | 19 +++++++++++++++---- .../org/apache/druid/math/expr/Function.java | 6 ++++++ 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java b/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java index 501abc81b566..f958fd698c89 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java +++ b/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java @@ -60,11 +60,17 @@ public interface ApplyFunction */ Set getArrayInputs(List args); + /** + * Returns true if apply function produces an array output + */ default boolean hasArrayOutput() { return false; } + /** + * Validate apply function arguments, throwing an exception if incorrect + */ void validateArguments(LambdaExpr lambdaExpr, List args); /** 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 e6e4e5b066d8..f8f24518a95b 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 @@ -256,6 +256,11 @@ public Set getArrayVariables() return arrayVariables.stream().map(IdentifierExpr::getIdentifier).collect(Collectors.toSet()); } + /** + * Returns true if the expression has any array inputs. Note that in some cases, this can be true and + * {@link BindingDetails#getArrayVariables} can be empty, since the latter can only shallowly collect identifiers + * that are explicitly used as arrays. + */ public boolean hasInputArrays() { return hasInputArrays; @@ -286,8 +291,8 @@ public BindingDetails with(BindingDetails other) ImmutableSet.copyOf(Sets.union(freeVariables, other.freeVariables)), ImmutableSet.copyOf(Sets.union(scalarVariables, other.scalarVariables)), ImmutableSet.copyOf(Sets.union(arrayVariables, other.arrayVariables)), - hasInputArrays, - isOutputArray + hasInputArrays || other.hasInputArrays, + isOutputArray || other.isOutputArray ); } @@ -330,22 +335,28 @@ public BindingDetails withArrayArguments(Set arrayArguments) ImmutableSet.copyOf(Sets.union(freeVariables, arrayIdentifiers)), scalarVariables, ImmutableSet.copyOf(Sets.union(arrayVariables, arrayIdentifiers)), - hasInputArrays, + hasInputArrays || arrayArguments.size() > 0, isOutputArray ); } + /** + * Copy, setting if an expression has array inputs + */ public BindingDetails withArrayInputs(boolean hasArrays) { return new BindingDetails( freeVariables, scalarVariables, arrayVariables, - hasArrays, + hasArrays || arrayVariables.size() > 0, isOutputArray ); } + /** + * Copy, setting if an expression produces an array output + */ public BindingDetails withArrayOutput(boolean isOutputArray) { return new BindingDetails( diff --git a/core/src/main/java/org/apache/druid/math/expr/Function.java b/core/src/main/java/org/apache/druid/math/expr/Function.java index 751f885e2ca0..8db8b1fc3364 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Function.java +++ b/core/src/main/java/org/apache/druid/math/expr/Function.java @@ -76,11 +76,17 @@ default Set getArrayInputs(List args) return Collections.emptySet(); } + /** + * Returns true if a function takes an array arguments + */ default boolean hasArrayInputs() { return false; } + /** + * Returns true if function produces an array + */ default boolean hasArrayOutput() { return false; From d31089415a84f57d9fba0f835b489e35e85615d2 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Fri, 12 Jul 2019 16:05:18 -0700 Subject: [PATCH 03/11] merge fixup --- .../java/org/apache/druid/segment/filter/BaseFilterTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/processing/src/test/java/org/apache/druid/segment/filter/BaseFilterTest.java b/processing/src/test/java/org/apache/druid/segment/filter/BaseFilterTest.java index 34b8b3f9c7e0..b92760f9caf8 100644 --- a/processing/src/test/java/org/apache/druid/segment/filter/BaseFilterTest.java +++ b/processing/src/test/java/org/apache/druid/segment/filter/BaseFilterTest.java @@ -295,7 +295,7 @@ private List selectColumnValuesMatchingFilter(final DimFilter filter, fi final List values = new ArrayList<>(); - while (!input.isDone()) { + while (!cursor.isDone()) { IndexedInts row = selector.getRow(); Preconditions.checkState(row.size() == 1); values.add(selector.lookupName(row.get(0))); @@ -419,7 +419,7 @@ public double estimateSelectivity(BitmapIndexSelector indexSelector) final List values = new ArrayList<>(); - while (!input.isDone()) { + while (!cursor.isDone()) { IndexedInts row = selector.getRow(); Preconditions.checkState(row.size() == 1); values.add(selector.lookupName(row.get(0))); From 9f95ca30c9ea7c7878bf14859281425037f2d64a Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Fri, 12 Jul 2019 17:47:50 -0700 Subject: [PATCH 04/11] vectorization fixup --- .../druid/segment/column/ColumnCapabilitiesImpl.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilitiesImpl.java b/processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilitiesImpl.java index 620a0f6e45c4..ba55864703d0 100644 --- a/processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilitiesImpl.java +++ b/processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilitiesImpl.java @@ -39,16 +39,19 @@ public class ColumnCapabilitiesImpl implements ColumnCapabilities @JsonIgnore private boolean filterable; + + @JsonIgnore + private boolean complete = false; + public static ColumnCapabilitiesImpl copyOf(final ColumnCapabilities other) { final ColumnCapabilitiesImpl capabilities = new ColumnCapabilitiesImpl(); capabilities.merge(other); + capabilities.setFilterable(other.isFilterable()); + capabilities.setIsComplete(other.isComplete()); return capabilities; } - @JsonIgnore - private boolean complete = false; - @Override @JsonProperty public ValueType getType() From e7605fc448ec65a0c748f89db4ef5f439297517a Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Mon, 15 Jul 2019 16:32:46 -0700 Subject: [PATCH 05/11] more fixes --- .../org/apache/druid/query/MultiValuedDimensionTest.java | 2 ++ .../druid/query/groupby/GroupByQueryRunnerTest.java | 9 +-------- 2 files changed, 3 insertions(+), 8 deletions(-) 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 12d18ac48c2d..8567c685467d 100644 --- a/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java +++ b/processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java @@ -668,6 +668,8 @@ public void testGroupByExpressionMultiMultiAutoAutoWithFilter() @Test public void testGroupByExpressionAuto() { + // virtual column is a single input column and input is not used explicitly as an array, + // so this one will work for group by v1, even with multi-value inputs GroupByQuery query = GroupByQuery .builder() .setDataSource("xx") diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java index 3eb69c0dda41..07bc74e5e6dd 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java @@ -25,7 +25,6 @@ import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Ordering; @@ -974,15 +973,9 @@ public void testGroupByWithStringPostAggregator() public void testGroupByWithStringVirtualColumn() { // Cannot vectorize due to virtual columns. + // all virtual columns are single input column, so it will work for group by v1, even with multi-value inputs cannotVectorize(); - // Cannot run with groupBy v1 on IncrementalIndex, because expressions would turn multi-value inputs - // into cardinalityless selectors, and groupBy v1 requires selectors that have a cardinality. - if (config.getDefaultStrategy().equals(GroupByStrategySelector.STRATEGY_V1) - && ImmutableSet.of("rtIndex", "noRollupRtIndex").contains(runnerName)) { - expectedException.expectMessage("GroupBy v1 does not support dimension selectors with unknown cardinality."); - } - GroupByQuery query = makeQueryBuilder() .setDataSource(QueryRunnerTestHelper.dataSource) .setQuerySegmentSpec(QueryRunnerTestHelper.firstToThird) From cf9e0954bddcf253ce1f9186974bff60aae8ab92 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 31 Jul 2019 23:55:17 -0700 Subject: [PATCH 06/11] more docs --- .../apache/druid/math/expr/ApplyFunction.java | 16 +++++++++--- .../java/org/apache/druid/math/expr/Expr.java | 26 +++++++++++++++---- .../org/apache/druid/math/expr/Function.java | 5 ++-- .../segment/virtual/ExpressionSelectors.java | 13 +++++++--- 4 files changed, 46 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java b/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java index f958fd698c89..82fc6cb6100c 100644 --- a/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java +++ b/core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java @@ -61,9 +61,10 @@ public interface ApplyFunction Set getArrayInputs(List args); /** - * Returns true if apply function produces an array output + * Returns true if apply function produces an array output. All {@link ApplyFunction} implementations are expected to + * exclusively produce either scalar or array values. */ - default boolean hasArrayOutput() + default boolean hasArrayOutput(LambdaExpr lambdaExpr) { return false; } @@ -81,7 +82,7 @@ default boolean hasArrayOutput() abstract class BaseMapFunction implements ApplyFunction { @Override - public boolean hasArrayOutput() + public boolean hasArrayOutput(LambdaExpr lambdaExpr) { return true; } @@ -277,6 +278,13 @@ ExprEval applyFold(LambdaExpr lambdaExpr, Object accumulator, IndexableFoldLambd } return ExprEval.bestEffortOf(accumulator); } + + @Override + public boolean hasArrayOutput(LambdaExpr lambdaExpr) + { + Expr.BindingDetails lambdaBindingDetails = lambdaExpr.analyzeInputs(); + return lambdaBindingDetails.isOutputArray(); + } } /** @@ -415,7 +423,7 @@ public String name() } @Override - public boolean hasArrayOutput() + public boolean hasArrayOutput(LambdaExpr lambdaExpr) { return true; } 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 f8f24518a95b..bc3f15045244 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 @@ -164,6 +164,23 @@ interface Shuttle * Information about the context in which {@link IdentifierExpr} are used in a greater {@link Expr}, listing * the 'free variables' (total set of required input columns or values) and distinguishing between which identifiers * are used as scalar values and which are used as array values. + * + * This type is primarily used at query time when creating expression column selectors to decide if an expression + * can properly deal with a multi-valued input column and also to determine if certain optimizations can be taken. + * + * Current implementations of {@link #analyzeInputs()} only 'shallowly' recognize {@link Function} and + * {@link ApplyFunction} arguments which are direct children {@link IdentifierExpr} as scalar or array typed. + * Identifiers inside of argument expressions which are other expression types will not be considered to belong + * directly to that function, and so are classified by their children instead. + * + * This means in rare cases and mostly for "questionable" expressions which we still allow to function 'correctly', + * these lists might not be fully reliable without a complete type inference system in place. Due to this shortcoming, + * boolean values {@link BindingDetails#hasInputArrays()} and {@link BindingDetails#isOutputArray()} are provided to + * allow functions to explicitly declare that they utilize array typed values, used when determining if some types of + * optimizations can be applied when constructing the expression column value selector. + * + * @see org.apache.druid.segment.virtual.ExpressionSelectors#makeDimensionSelector + * @see org.apache.druid.segment.virtual.ExpressionSelectors#makeColumnValueSelector */ class BindingDetails { @@ -258,8 +275,8 @@ public Set getArrayVariables() /** * Returns true if the expression has any array inputs. Note that in some cases, this can be true and - * {@link BindingDetails#getArrayVariables} can be empty, since the latter can only shallowly collect identifiers - * that are explicitly used as arrays. + * {@link BindingDetails#getArrayVariables} can be empty. This is because arrayVariables contains a best-effort list + * of {@link IdentifierExpr} which were explicitly used as an array argument */ public boolean hasInputArrays() { @@ -888,12 +905,11 @@ class ApplyFunctionExpr implements Expr } lambdaBindingDetails = lambdaExpr.analyzeInputs(); - boolean isArrayOutput = function.hasArrayOutput() || (function instanceof ApplyFunction.BaseFoldFunction - && lambdaBindingDetails.isOutputArray()); + bindingDetails = accumulator.with(lambdaBindingDetails) .withArrayArguments(function.getArrayInputs(argsExpr)) .withArrayInputs(true) - .withArrayOutput(isArrayOutput); + .withArrayOutput(function.hasArrayOutput(lambdaExpr)); argsBindingDetails = argBindingDetailsBuilder.build(); } diff --git a/core/src/main/java/org/apache/druid/math/expr/Function.java b/core/src/main/java/org/apache/druid/math/expr/Function.java index 8db8b1fc3364..ec6e15b5427a 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Function.java +++ b/core/src/main/java/org/apache/druid/math/expr/Function.java @@ -77,7 +77,7 @@ default Set getArrayInputs(List args) } /** - * Returns true if a function takes an array arguments + * Returns true if a function expects any array arguments */ default boolean hasArrayInputs() { @@ -85,7 +85,8 @@ default boolean hasArrayInputs() } /** - * Returns true if function produces an array + * Returns true if function produces an array. All {@link Function} implementations are expected to + * exclusively produce either scalar or array values. */ default boolean hasArrayOutput() { 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 b77c14064b0b..c41d4841bf0d 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,8 +160,8 @@ public static ColumnValueSelector makeExprEvalSelector( && capabilities.isDictionaryEncoded() && capabilities.isComplete() && !capabilities.hasMultipleValues() - && exprDetails.getArrayColumns().size() == 0) { - // Optimization for expressions that hit one string column and nothing else. + && exprDetails.getArrayColumns().isEmpty()) { + // Optimization for expressions that hit one scalar string column and nothing else. return new SingleStringInputCachingExpressionColumnValueSelector( columnSelectorFactory.makeDimensionSelector(new DefaultDimensionSpec(column, column, ValueType.STRING)), expression @@ -226,15 +226,22 @@ public static DimensionSelector makeDimensionSelector( final String column = Iterables.getOnlyElement(columns); final ColumnCapabilities capabilities = columnSelectorFactory.getColumnCapabilities(column); + // Optimization for dimension selectors that wrap a single underlying string column. + // The string column can be multi-valued, but if so, it must be implicitly mappable (i.e. the expression is + // not treating it as an array, not wanting to output an array, and the multi-value dimension appears + // exactly once). if (capabilities != null && capabilities.getType() == ValueType.STRING && capabilities.isDictionaryEncoded() && capabilities.isComplete() && !exprDetails.hasInputArrays() && !exprDetails.isOutputArray() + // the following condition specifically is to handle the case of when a multi-value column identifier + // appears more than once in an expression, + // e.g. 'x + x' is fine if 'x' is scalar, but if 'x' is multi-value it should be translated to + // 'cartesian_map((x_1, x_2) -> x_1 + x_2, x, x)' && (!capabilities.hasMultipleValues() || exprDetails.getFreeVariables().size() == 1) ) { - // Optimization for dimension selectors that wrap a single underlying string column. return new SingleStringInputDimensionSelector( columnSelectorFactory.makeDimensionSelector(new DefaultDimensionSpec(column, column, ValueType.STRING)), expression From a49ad4fd41d7e56a8e3cead0cc248964d99a4416 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Thu, 1 Aug 2019 00:12:24 -0700 Subject: [PATCH 07/11] more links --- core/src/main/java/org/apache/druid/math/expr/Expr.java | 3 +++ 1 file changed, 3 insertions(+) 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 bc3f15045244..dabdb3a13990 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 @@ -179,6 +179,9 @@ interface Shuttle * allow functions to explicitly declare that they utilize array typed values, used when determining if some types of * optimizations can be applied when constructing the expression column value selector. * + * @see Parser#applyUnappliedIdentifiers + * @see Parser#applyUnapplied + * @see Parser#liftApplyLambda * @see org.apache.druid.segment.virtual.ExpressionSelectors#makeDimensionSelector * @see org.apache.druid.segment.virtual.ExpressionSelectors#makeColumnValueSelector */ From b8cedc8e2cb45e5b71ff88f05b5f843d3db20704 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Thu, 1 Aug 2019 11:35:15 -0700 Subject: [PATCH 08/11] empty --- .../main/java/org/apache/druid/math/expr/Function.java | 2 +- core/src/main/java/org/apache/druid/math/expr/Parser.java | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/druid/math/expr/Function.java b/core/src/main/java/org/apache/druid/math/expr/Function.java index ec6e15b5427a..4439a667531b 100644 --- a/core/src/main/java/org/apache/druid/math/expr/Function.java +++ b/core/src/main/java/org/apache/druid/math/expr/Function.java @@ -1851,7 +1851,7 @@ public boolean hasArrayOutput() @Override public void validateArguments(List args) { - if (!(args.size() > 0)) { + if (args.isEmpty()) { throw new IAE("Function[%s] needs at least 1 argument", name()); } } 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 55144abaec85..57e0757e74ad 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 @@ -167,7 +167,7 @@ public static Expr flatten(Expr expr) */ public static Expr applyUnappliedIdentifiers(Expr expr, Expr.BindingDetails bindingDetails, List toApply) { - if (toApply.size() == 0) { + if (toApply.isEmpty()) { return expr; } List unapplied = toApply.stream() @@ -207,7 +207,7 @@ public static Expr applyUnappliedIdentifiers(Expr expr, Expr.BindingDetails bind unapplied.stream().filter(x -> !expectedArrays.contains(x)).collect(Collectors.toList()); // if lifting the lambdas got rid of all missing bindings, return the transformed expression - if (remainingUnappliedArgs.size() == 0) { + if (remainingUnappliedArgs.isEmpty()) { return newExpr; } @@ -304,7 +304,7 @@ private static ApplyFunctionExpr liftApplyLambda(ApplyFunctionExpr expr, List new IdentifierExpr(x.getIdentifier(), x.getBindingIdentifier())) .collect(Collectors.toList()); - if (unappliedLambdaBindings.size() == 0) { + if (unappliedLambdaBindings.isEmpty()) { return new ApplyFunctionExpr(expr.function, expr.name, expr.lambdaExpr, newArgs); } @@ -387,7 +387,7 @@ public static void validateExpr(Expr expression, Expr.BindingDetails bindingDeta { final Set conflicted = Sets.intersection(bindingDetails.getScalarColumns(), bindingDetails.getArrayColumns()); - if (conflicted.size() != 0) { + if (!conflicted.isEmpty()) { throw new RE("Invalid expression: %s; %s used as both scalar and array variables", expression, conflicted); } } From c4d03c4aa097d0a052dce3bf2d3800efcfcb6b4c Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Thu, 1 Aug 2019 16:27:07 -0700 Subject: [PATCH 09/11] javadocs are hard --- .../java/org/apache/druid/math/expr/Expr.java | 100 +++++++++++------- .../druid/math/expr/ExprListenerImpl.java | 2 +- .../org/apache/druid/math/expr/Parser.java | 51 +++++---- .../apache/druid/math/expr/ParserTest.java | 4 +- .../SimpleDoubleAggregatorFactory.java | 2 +- .../SimpleFloatAggregatorFactory.java | 2 +- .../SimpleLongAggregatorFactory.java | 2 +- .../post/ExpressionPostAggregator.java | 2 +- .../query/filter/ExpressionDimFilter.java | 2 +- .../segment/filter/ExpressionFilter.java | 2 +- .../segment/virtual/ExpressionSelectors.java | 16 +-- .../virtual/ExpressionVirtualColumn.java | 2 +- ...RowBasedExpressionColumnValueSelector.java | 4 +- ...tCachingExpressionColumnValueSelector.java | 2 +- ...tCachingExpressionColumnValueSelector.java | 2 +- .../SingleStringInputDimensionSelector.java | 2 +- 16 files changed, 113 insertions(+), 84 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 dabdb3a13990..c1dcaa0ef7e3 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 @@ -90,11 +90,11 @@ default String getIdentifierIfIdentifier() } /** - * Returns the string identifier to use to get a value from {@link Expr.ObjectBinding} of an {@link IdentifierExpr}, + * Returns the string key to use to get a value from {@link Expr.ObjectBinding} of an {@link IdentifierExpr}, * else null */ @Nullable - default String getIdentifierBindingIfIdentifier() + default String getBindingIfIdentifier() { // overridden by things that are identifiers return null; @@ -163,15 +163,17 @@ interface Shuttle /** * Information about the context in which {@link IdentifierExpr} are used in a greater {@link Expr}, listing * the 'free variables' (total set of required input columns or values) and distinguishing between which identifiers - * are used as scalar values and which are used as array values. + * are used as scalar inputs and which are used as array inputs. * * This type is primarily used at query time when creating expression column selectors to decide if an expression - * can properly deal with a multi-valued input column and also to determine if certain optimizations can be taken. + * can properly deal with a multi-valued column input, and also to determine if certain optimizations can be taken. * - * Current implementations of {@link #analyzeInputs()} only 'shallowly' recognize {@link Function} and + * Current implementations of {@link #analyzeInputs()} provide context about {@link Function} and * {@link ApplyFunction} arguments which are direct children {@link IdentifierExpr} as scalar or array typed. - * Identifiers inside of argument expressions which are other expression types will not be considered to belong - * directly to that function, and so are classified by their children instead. + * This is defined by {@link Function#getScalarInputs(List)}, {@link Function#getArrayInputs(List)} and + * {@link ApplyFunction#getArrayInputs(List)}. Identifiers that are nested inside of argument expressions which + * are other expression types will not be considered to belong directly to that function, and so are classified by the + * context their children are using them as instead. * * This means in rare cases and mostly for "questionable" expressions which we still allow to function 'correctly', * these lists might not be fully reliable without a complete type inference system in place. Due to this shortcoming, @@ -179,7 +181,10 @@ interface Shuttle * allow functions to explicitly declare that they utilize array typed values, used when determining if some types of * optimizations can be applied when constructing the expression column value selector. * - * @see Parser#applyUnappliedIdentifiers + * @see Function#getScalarInputs + * @see Function#getArrayInputs + * @see ApplyFunction#getArrayInputs + * @see Parser#applyUnappliedBindings * @see Parser#applyUnapplied * @see Parser#liftApplyLambda * @see org.apache.druid.segment.virtual.ExpressionSelectors#makeDimensionSelector @@ -219,41 +224,42 @@ public BindingDetails( } /** - * Get the list of required column inputs to evaluate an expression + * Get the list of required column inputs to evaluate an expression ({@link IdentifierExpr#binding}) */ - public List getRequiredColumnsList() + public List getRequiredBindingsList() { return new ArrayList<>( - freeVariables.stream().map(IdentifierExpr::getIdentifierBindingIfIdentifier).collect(Collectors.toSet()) + freeVariables.stream().map(IdentifierExpr::getBindingIfIdentifier).collect(Collectors.toSet()) ); } /** - * Get the set of required column inputs to evaluate an expression, {@link IdentifierExpr#bindingIdentifier} + * Get the set of required column inputs to evaluate an expression ({@link IdentifierExpr#binding}) */ - public Set getRequiredColumns() + public Set getRequiredBindings() { - return freeVariables.stream().map(IdentifierExpr::getIdentifierBindingIfIdentifier).collect(Collectors.toSet()); + return freeVariables.stream().map(IdentifierExpr::getBindingIfIdentifier).collect(Collectors.toSet()); } /** - * Set of {@link IdentifierExpr#bindingIdentifier} which are used with scalar operators and functions + * Set of {@link IdentifierExpr#binding} which are used as scalar inputs to operators and functions. */ - public Set getScalarColumns() + public Set getScalarBindings() { - return scalarVariables.stream().map(IdentifierExpr::getIdentifierBindingIfIdentifier).collect(Collectors.toSet()); + return scalarVariables.stream().map(IdentifierExpr::getBindingIfIdentifier).collect(Collectors.toSet()); } /** - * Set of {@link IdentifierExpr#bindingIdentifier} which are used with array typed functions and apply functions. + * Set of {@link IdentifierExpr#binding} which are used as array inputs to operators, functions, and apply + * functions. */ - public Set getArrayColumns() + public Set getArrayBindings() { - return arrayVariables.stream().map(IdentifierExpr::getIdentifierBindingIfIdentifier).collect(Collectors.toSet()); + return arrayVariables.stream().map(IdentifierExpr::getBindingIfIdentifier).collect(Collectors.toSet()); } /** - * Total set of 'free' identifiers of an {@link Expr}, that are not supplied by a {@link LambdaExpr} binding + * Total set of 'free' inputs of an {@link Expr}, that are not supplied by a {@link LambdaExpr} binding */ public Set getFreeVariables() { @@ -261,7 +267,7 @@ public Set getFreeVariables() } /** - * Set of {@link IdentifierExpr#identifier} which are used with scalar operators and functions. + * Set of {@link IdentifierExpr#identifier} which are used as scalar inputs to operators and functions. */ public Set getScalarVariables() { @@ -269,7 +275,8 @@ public Set getScalarVariables() } /** - * Set of {@link IdentifierExpr#identifier} which are used with array typed functions and apply functions. + * Set of {@link IdentifierExpr#identifier} which are used as array inputs to operators, functions, and apply + * functions. */ public Set getArrayVariables() { @@ -277,9 +284,14 @@ public Set getArrayVariables() } /** - * Returns true if the expression has any array inputs. Note that in some cases, this can be true and - * {@link BindingDetails#getArrayVariables} can be empty. This is because arrayVariables contains a best-effort list - * of {@link IdentifierExpr} which were explicitly used as an array argument + * Returns true if any expression in the expression tree has any array inputs. Note that in some cases, this can be + * true and {@link #getArrayBindings()} or {@link #getArrayVariables()} can be empty. + * + * This is because these collections contain identifiers/bindings which were classified as either scalar or array + * inputs based on the context of their usage by {@link Expr#analyzeInputs()}, where as this value and + * {@link #isOutputArray()} are set based on information reported by {@link Function#hasArrayInputs()}, + * {@link Function#hasArrayOutput()}, and {@link ApplyFunction#hasArrayOutput(LambdaExpr)}, without regards to + * identifiers or anything else. */ public boolean hasInputArrays() { @@ -287,7 +299,8 @@ public boolean hasInputArrays() } /** - * Returns true if this expression evaluates to an array type + * Returns true if any expression in this expression tree produces array outputs as reported by + * {@link Function#hasArrayOutput()} or {@link ApplyFunction#hasArrayOutput(LambdaExpr)} */ public boolean isOutputArray() { @@ -649,28 +662,37 @@ public ExprEval eval(ObjectBinding bindings) class IdentifierExpr implements Expr { private final String identifier; - private final String bindingIdentifier; + private final String binding; + /** + * Construct a identifier expression for a {@link LambdaExpr}, where the {@link #identifier} is equal to + * {@link #binding} + */ IdentifierExpr(String value) { this.identifier = value; - this.bindingIdentifier = value; + this.binding = value; } - IdentifierExpr(String identifier, String bindingIdentifier) + /** + * Construct a normal identifier expression, where {@link #binding} is the key to fetch the backing value from + * {@link Expr.ObjectBinding} and the {@link #identifier} is a unique string that identifies this usage of the + * binding. + */ + IdentifierExpr(String identifier, String binding) { this.identifier = identifier; - this.bindingIdentifier = bindingIdentifier; + this.binding = binding; } @Override public String toString() { - return bindingIdentifier; + return binding; } /** - * Unique identifier + * Unique identifier for the binding */ @Nullable public String getIdentifier() @@ -679,12 +701,12 @@ public String getIdentifier() } /** - * Value binding identifier + * Value binding, key to retrieve value from {@link Expr.ObjectBinding#get(String)} */ @Nullable - public String getBindingIdentifier() + public String getBinding() { - return bindingIdentifier; + return binding; } @Nullable @@ -696,9 +718,9 @@ public String getIdentifierIfIdentifier() @Nullable @Override - public String getIdentifierBindingIfIdentifier() + public String getBindingIfIdentifier() { - return bindingIdentifier; + return binding; } @Nullable @@ -717,7 +739,7 @@ public BindingDetails analyzeInputs() @Override public ExprEval eval(ObjectBinding bindings) { - return ExprEval.bestEffortOf(bindings.get(bindingIdentifier)); + return ExprEval.bestEffortOf(bindings.get(binding)); } @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 b4c91a3436d3..24654a5a7a88 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 @@ -426,7 +426,7 @@ public void exitEmptyArray(ExprParser.EmptyArrayContext ctx) * {@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. + * {@link Parser#applyUnappliedBindings(Expr, Expr.BindingDetails, List)}} for additional details. */ private IdentifierExpr createIdentifierExpr(String binding) { 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 57e0757e74ad..ce0c35f0663a 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 @@ -162,24 +162,26 @@ public static Expr flatten(Expr expr) * used in a scalar manner, walking the {@link Expr} tree and lifting array variables into the {@link LambdaExpr} of * {@link ApplyFunctionExpr} and transforming the arguments of {@link FunctionExpr} * @param expr expression to visit and rewrite - * @param toApply + * @param bindingsToApply * @return */ - public static Expr applyUnappliedIdentifiers(Expr expr, Expr.BindingDetails bindingDetails, List toApply) + public static Expr applyUnappliedBindings(Expr expr, Expr.BindingDetails bindingDetails, List bindingsToApply) { - if (toApply.isEmpty()) { + if (bindingsToApply.isEmpty()) { + // nothing to do, expression is fine as is return expr; } - List unapplied = toApply.stream() - .filter(x -> bindingDetails.getRequiredColumns().contains(x)) + // filter the list of bindings to those which are used in this expression + List unappliedBindingsInExpression = bindingsToApply.stream() + .filter(x -> bindingDetails.getRequiredBindings().contains(x)) .collect(Collectors.toList()); - // any unapplied identifiers that are inside a lambda expression need that lambda expression to be rewritten + // any unapplied bindings that are inside a lambda expression need that lambda expression to be rewritten Expr newExpr = expr.visit( childExpr -> { if (childExpr instanceof ApplyFunctionExpr) { // try to lift unapplied arguments into the apply function lambda - return liftApplyLambda((ApplyFunctionExpr) childExpr, unapplied); + return liftApplyLambda((ApplyFunctionExpr) childExpr, unappliedBindingsInExpression); } else if (childExpr instanceof FunctionExpr) { // check array function arguments for unapplied identifiers to transform if necessary FunctionExpr fnExpr = (FunctionExpr) childExpr; @@ -187,7 +189,7 @@ public static Expr applyUnappliedIdentifiers(Expr expr, Expr.BindingDetails bind List newArgs = new ArrayList<>(); for (Expr arg : fnExpr.args) { if (arg.getIdentifierIfIdentifier() == null && arrayInputs.contains(arg)) { - Expr newArg = applyUnappliedIdentifiers(arg, bindingDetails, unapplied); + Expr newArg = applyUnappliedBindings(arg, bindingDetails, unappliedBindingsInExpression); newArgs.add(newArg); } else { newArgs.add(arg); @@ -203,34 +205,37 @@ public static Expr applyUnappliedIdentifiers(Expr expr, Expr.BindingDetails bind Expr.BindingDetails newExprBindings = newExpr.analyzeInputs(); final Set expectedArrays = newExprBindings.getArrayVariables(); - List remainingUnappliedArgs = - unapplied.stream().filter(x -> !expectedArrays.contains(x)).collect(Collectors.toList()); + + List remainingUnappliedBindings = + unappliedBindingsInExpression.stream().filter(x -> !expectedArrays.contains(x)).collect(Collectors.toList()); // if lifting the lambdas got rid of all missing bindings, return the transformed expression - if (remainingUnappliedArgs.isEmpty()) { + if (remainingUnappliedBindings.isEmpty()) { return newExpr; } - return applyUnapplied(newExpr, remainingUnappliedArgs); + return applyUnapplied(newExpr, remainingUnappliedBindings); } /** * 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) + private static Expr applyUnapplied(Expr expr, List unappliedBindings) { - // wrap an expression in either map or cartesian_map to apply any unapplied identifiers final Map toReplace = new HashMap<>(); + + // filter to get list of IdentifierExpr that are backed by the unapplied bindings final List args = expr.analyzeInputs() .getFreeVariables() .stream() - .filter(x -> unapplied.contains(x.getBindingIdentifier())) + .filter(x -> unappliedBindings.contains(x.getBinding())) .collect(Collectors.toList()); final List lambdaArgs = new ArrayList<>(); - // construct lambda args from list of args to apply + // construct lambda args from list of args to apply. Identifiers in a lambda body have artificial 'binding' values + // that is the same as the 'identifier', because the bindings are supplied by the wrapping apply function for (IdentifierExpr applyFnArg : args) { IdentifierExpr lambdaRewrite = new IdentifierExpr(applyFnArg.getIdentifier()); lambdaArgs.add(lambdaRewrite); @@ -248,6 +253,8 @@ private static Expr applyUnapplied(Expr expr, List unapplied) return childExpr; }); + + // wrap an expression in either map or cartesian_map to apply any unapplied identifiers final LambdaExpr lambdaExpr = new LambdaExpr(lambdaArgs, newExpr); final ApplyFunction fn; if (args.size() == 1) { @@ -274,21 +281,21 @@ private static ApplyFunctionExpr liftApplyLambda(ApplyFunctionExpr expr, List unappliedInThisApply = unappliedArgs.stream() - .filter(u -> !expr.bindingDetails.getArrayColumns().contains(u)) + .filter(u -> !expr.bindingDetails.getArrayBindings().contains(u)) .collect(Collectors.toSet()); List unappliedIdentifiers = expr.bindingDetails .getFreeVariables() .stream() - .filter(x -> unappliedInThisApply.contains(x.getIdentifierBindingIfIdentifier())) + .filter(x -> unappliedInThisApply.contains(x.getBindingIfIdentifier())) .map(IdentifierExpr::getIdentifierIfIdentifier) .collect(Collectors.toList()); List newArgs = new ArrayList<>(); for (int i = 0; i < expr.argsExpr.size(); i++) { newArgs.add( - applyUnappliedIdentifiers( + applyUnappliedBindings( expr.argsExpr.get(i), expr.argsBindingDetails.get(i), unappliedIdentifiers @@ -300,8 +307,8 @@ private static ApplyFunctionExpr liftApplyLambda(ApplyFunctionExpr expr, List unappliedLambdaBindings = expr.lambdaBindingDetails.getFreeVariables() .stream() - .filter(x -> unappliedArgs.contains(x.getIdentifierBindingIfIdentifier())) - .map(x -> new IdentifierExpr(x.getIdentifier(), x.getBindingIdentifier())) + .filter(x -> unappliedArgs.contains(x.getBindingIfIdentifier())) + .map(x -> new IdentifierExpr(x.getIdentifier(), x.getBinding())) .collect(Collectors.toList()); if (unappliedLambdaBindings.isEmpty()) { @@ -386,7 +393,7 @@ private static ApplyFunctionExpr liftApplyLambda(ApplyFunctionExpr expr, List conflicted = - Sets.intersection(bindingDetails.getScalarColumns(), bindingDetails.getArrayColumns()); + Sets.intersection(bindingDetails.getScalarBindings(), bindingDetails.getArrayBindings()); if (!conflicted.isEmpty()) { 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 f70a7162e1d1..cac4733024f2 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 @@ -471,7 +471,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.getRequiredColumnsList()); + Assert.assertEquals(expression, identifiers, deets.getRequiredBindingsList()); Assert.assertEquals(expression, scalars, deets.getScalarVariables()); Assert.assertEquals(expression, arrays, deets.getArrayVariables()); } @@ -486,7 +486,7 @@ private void validateApplyUnapplied( final Expr parsed = Parser.parse(expression, ExprMacroTable.nil()); Expr.BindingDetails deets = parsed.analyzeInputs(); Parser.validateExpr(parsed, deets); - final Expr transformed = Parser.applyUnappliedIdentifiers(parsed, deets, identifiers); + final Expr transformed = Parser.applyUnappliedBindings(parsed, deets, identifiers); Assert.assertEquals(expression, unapplied, parsed.toString()); Assert.assertEquals(applied, applied, transformed.toString()); } 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 6995d57857e5..6b3643abbee0 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().getRequiredColumnsList(); + : fieldExpression.get().analyzeInputs().getRequiredBindingsList(); } @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 6619126cedd9..78e86de8e009 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().getRequiredColumnsList(); + : fieldExpression.get().analyzeInputs().getRequiredBindingsList(); } @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 308100c0516b..cb2d66f1d1eb 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().getRequiredColumnsList(); + : fieldExpression.get().analyzeInputs().getRequiredBindingsList(); } @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 281356c66caf..5bb9feebc47f 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().getRequiredColumns())); + Suppliers.memoize(() -> parsed.get().analyzeInputs().getRequiredBindings())); } private ExpressionPostAggregator( 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 0a67652ff6e6..ec4a6d8fcb0c 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().getRequiredColumns()); + return Sets.newHashSet(parsed.get().analyzeInputs().getRequiredBindings()); } @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 f46cb8db4f33..c721e9e0fb5d 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().getRequiredColumns()); + this.requiredBindings = Suppliers.memoize(() -> expr.get().analyzeInputs().getRequiredBindings()); } @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 c41d4841bf0d..1348029d9980 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.getRequiredColumnsList(); + final List columns = exprDetails.getRequiredBindingsList(); if (columns.size() == 1) { final String column = Iterables.getOnlyElement(columns); @@ -160,7 +160,7 @@ public static ColumnValueSelector makeExprEvalSelector( && capabilities.isDictionaryEncoded() && capabilities.isComplete() && !capabilities.hasMultipleValues() - && exprDetails.getArrayColumns().isEmpty()) { + && exprDetails.getArrayBindings().isEmpty()) { // Optimization for expressions that hit one scalar string column and nothing else. return new SingleStringInputCachingExpressionColumnValueSelector( columnSelectorFactory.makeDimensionSelector(new DefaultDimensionSpec(column, column, ValueType.STRING)), @@ -176,11 +176,11 @@ public static ColumnValueSelector makeExprEvalSelector( final List needsApplied = columns.stream() - .filter(c -> actualArrays.contains(c) && !exprDetails.getArrayColumns().contains(c)) + .filter(c -> actualArrays.contains(c) && !exprDetails.getArrayBindings().contains(c)) .collect(Collectors.toList()); final Expr finalExpr; if (needsApplied.size() > 0) { - finalExpr = Parser.applyUnappliedIdentifiers(expression, exprDetails, needsApplied); + finalExpr = Parser.applyUnappliedBindings(expression, exprDetails, needsApplied); } else { finalExpr = expression; } @@ -219,7 +219,7 @@ public static DimensionSelector makeDimensionSelector( { final Expr.BindingDetails exprDetails = expression.analyzeInputs(); Parser.validateExpr(expression, exprDetails); - final List columns = exprDetails.getRequiredColumnsList(); + final List columns = exprDetails.getRequiredBindingsList(); if (columns.size() == 1) { @@ -257,7 +257,7 @@ public static DimensionSelector makeDimensionSelector( final ColumnValueSelector baseSelector = makeExprEvalSelector(columnSelectorFactory, expression); final boolean multiVal = actualArrays.size() > 0 || - exprDetails.getArrayColumns().size() > 0 || + exprDetails.getArrayBindings().size() > 0 || unknownIfArrays.size() > 0; if (baseSelector instanceof ConstantExprEvalSelector) { @@ -363,7 +363,7 @@ private static Expr.ObjectBinding createBindings( ) { final Map> suppliers = new HashMap<>(); - final List columns = bindingDetails.getRequiredColumnsList(); + final List columns = bindingDetails.getRequiredBindingsList(); for (String columnName : columns) { final ColumnCapabilities columnCapabilities = columnSelectorFactory .getColumnCapabilities(columnName); @@ -538,7 +538,7 @@ private static Pair, Set> examineColumnSelectorFactoryArrays } else if ( !capabilities.isComplete() && capabilities.getType().equals(ValueType.STRING) && - !exprDetails.getArrayColumns().contains(column) + !exprDetails.getArrayBindings().contains(column) ) { unknownIfArrays.add(column); } 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 c322fff9d2bf..0d9feefc459a 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 @@ -114,7 +114,7 @@ public ColumnCapabilities capabilities(String columnName) @Override public List requiredColumns() { - return parsedExpression.get().analyzeInputs().getRequiredColumnsList(); + return parsedExpression.get().analyzeInputs().getRequiredBindingsList(); } @Override 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 719f664e625b..19d9ebfc72ae 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.getArrayColumns().contains(x)) + .filter(x -> !baseExprBindingDetails.getArrayBindings().contains(x)) .collect(Collectors.toList()); this.baseExprBindingDetails = baseExprBindingDetails; this.ignoredColumns = new HashSet<>(); @@ -79,7 +79,7 @@ public ExprEval getObject() if (transformedCache.containsKey(key)) { return transformedCache.get(key).eval(bindings); } - Expr transformed = Parser.applyUnappliedIdentifiers(expression, baseExprBindingDetails, arrayBindings); + Expr transformed = Parser.applyUnappliedBindings(expression, baseExprBindingDetails, arrayBindings); transformedCache.put(key, transformed); return transformed.eval(bindings); } 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 8fb9cdc91321..df9241b9eead 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().getRequiredColumns().size() != 1) { + if (expression.analyzeInputs().getRequiredBindings().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 0b8c44353624..a3bf08c8f6f5 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 @@ -55,7 +55,7 @@ public SingleStringInputCachingExpressionColumnValueSelector( ) { // Verify expression has just one binding. - if (expression.analyzeInputs().getRequiredColumns().size() != 1) { + if (expression.analyzeInputs().getRequiredBindings().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 4abfa83a38f6..00c9d58001bd 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 @@ -49,7 +49,7 @@ public SingleStringInputDimensionSelector( ) { // Verify expression has just one binding. - if (expression.analyzeInputs().getRequiredColumns().size() != 1) { + if (expression.analyzeInputs().getRequiredBindings().size() != 1) { throw new ISE("WTF?! Expected expression with just one binding"); } From 2fea2ec347f3e4cc14d31bb54f157c1791e628f4 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Thu, 1 Aug 2019 16:35:10 -0700 Subject: [PATCH 10/11] suppress javadoc refs issue --- core/src/main/java/org/apache/druid/math/expr/Expr.java | 1 + 1 file changed, 1 insertion(+) 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 c1dcaa0ef7e3..3c54149af919 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 @@ -190,6 +190,7 @@ interface Shuttle * @see org.apache.druid.segment.virtual.ExpressionSelectors#makeDimensionSelector * @see org.apache.druid.segment.virtual.ExpressionSelectors#makeColumnValueSelector */ + @SuppressWarnings("JavadocReference") class BindingDetails { private final ImmutableSet freeVariables; From 346fb3372585f64314908dc95ca9dde2d9ea6a50 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Thu, 1 Aug 2019 18:34:00 -0700 Subject: [PATCH 11/11] fix it --- .../java/org/apache/druid/math/expr/ExprListenerImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 24654a5a7a88..825c5ed9f98d 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 @@ -422,8 +422,8 @@ 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} because they have + * {@link IdentifierExpr#binding} value. {@link LambdaExpr} identifiers however, will always have + * {@link IdentifierExpr#identifier} be the same as {@link IdentifierExpr#binding} 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#applyUnappliedBindings(Expr, Expr.BindingDetails, List)}} for additional details.