You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by gi...@apache.org on 2019/08/02 20:21:34 UTC
[incubator-druid] branch master updated: optimize single input
column multi-value expressions (#8047)
This is an automated email from the ASF dual-hosted git repository.
gian pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-druid.git
The following commit(s) were added to refs/heads/master by this push:
new e7c6dea optimize single input column multi-value expressions (#8047)
e7c6dea is described below
commit e7c6deac768d66c12b71daecae12ecb5a7db2380
Author: Clint Wylie <cw...@apache.org>
AuthorDate: Fri Aug 2 13:21:25 2019 -0700
optimize single input column multi-value expressions (#8047)
* optimize single input column multi-value expressions
* javadocs
* merge fixup
* vectorization fixup
* more fixes
* more docs
* more links
* empty
* javadocs are hard
* suppress javadoc refs issue
* fix it
---
.../org/apache/druid/math/expr/ApplyFunction.java | 31 ++++
.../main/java/org/apache/druid/math/expr/Expr.java | 193 ++++++++++++++++-----
.../apache/druid/math/expr/ExprListenerImpl.java | 6 +-
.../java/org/apache/druid/math/expr/Function.java | 167 +++++++++++++-----
.../java/org/apache/druid/math/expr/Parser.java | 55 +++---
.../org/apache/druid/math/expr/ParserTest.java | 4 +-
.../aggregation/SimpleDoubleAggregatorFactory.java | 2 +-
.../aggregation/SimpleFloatAggregatorFactory.java | 2 +-
.../aggregation/SimpleLongAggregatorFactory.java | 2 +-
.../aggregation/post/ExpressionPostAggregator.java | 2 +-
.../druid/query/filter/ExpressionDimFilter.java | 2 +-
.../segment/column/ColumnCapabilitiesImpl.java | 9 +-
.../druid/segment/filter/ExpressionFilter.java | 2 +-
.../druid/segment/virtual/ExpressionSelectors.java | 32 ++--
.../segment/virtual/ExpressionVirtualColumn.java | 2 +-
.../RowBasedExpressionColumnValueSelector.java | 4 +-
...gInputCachingExpressionColumnValueSelector.java | 2 +-
...gInputCachingExpressionColumnValueSelector.java | 2 +-
.../SingleStringInputDimensionSelector.java | 9 +-
.../druid/query/MultiValuedDimensionTest.java | 6 +-
.../query/groupby/GroupByQueryRunnerTest.java | 9 +-
.../druid/segment/filter/BaseFilterTest.java | 62 +++----
.../druid/segment/filter/ExpressionFilterTest.java | 1 +
.../apache/druid/sql/calcite/CalciteQueryTest.java | 3 +-
24 files changed, 419 insertions(+), 190 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 ac1d02b..82fc6cb 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,18 @@ public interface ApplyFunction
*/
Set<Expr> getArrayInputs(List<Expr> args);
+ /**
+ * 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(LambdaExpr lambdaExpr)
+ {
+ return false;
+ }
+
+ /**
+ * Validate apply function arguments, throwing an exception if incorrect
+ */
void validateArguments(LambdaExpr lambdaExpr, List<Expr> args);
/**
@@ -69,6 +81,12 @@ public interface ApplyFunction
*/
abstract class BaseMapFunction implements ApplyFunction
{
+ @Override
+ public boolean hasArrayOutput(LambdaExpr lambdaExpr)
+ {
+ return true;
+ }
+
/**
* Evaluate {@link LambdaExpr} against every index position of an {@link IndexableMapLambdaObjectBinding}
*/
@@ -260,6 +278,13 @@ public interface ApplyFunction
}
return ExprEval.bestEffortOf(accumulator);
}
+
+ @Override
+ public boolean hasArrayOutput(LambdaExpr lambdaExpr)
+ {
+ Expr.BindingDetails lambdaBindingDetails = lambdaExpr.analyzeInputs();
+ return lambdaBindingDetails.isOutputArray();
+ }
}
/**
@@ -398,6 +423,12 @@ public interface ApplyFunction
}
@Override
+ public boolean hasArrayOutput(LambdaExpr lambdaExpr)
+ {
+ return true;
+ }
+
+ @Override
public ExprEval apply(LambdaExpr lambdaExpr, List<Expr> argsExpr, Expr.ObjectBinding bindings)
{
Expr arrayExpr = argsExpr.get(0);
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 07c20fa..3c54149 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 @@ public interface Expr
}
/**
- * 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,69 +163,104 @@ public interface Expr
/**
* 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 column input, and also to determine if certain optimizations can be taken.
+ *
+ * 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.
+ * 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,
+ * 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 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
+ * @see org.apache.druid.segment.virtual.ExpressionSelectors#makeColumnValueSelector
*/
+ @SuppressWarnings("JavadocReference")
class BindingDetails
{
private final ImmutableSet<IdentifierExpr> freeVariables;
private final ImmutableSet<IdentifierExpr> scalarVariables;
private final ImmutableSet<IdentifierExpr> 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<IdentifierExpr> freeVariables,
ImmutableSet<IdentifierExpr> scalarVariables,
- ImmutableSet<IdentifierExpr> arrayVariables
+ ImmutableSet<IdentifierExpr> arrayVariables,
+ boolean hasInputArrays,
+ boolean isOutputArray
)
{
this.freeVariables = freeVariables;
this.scalarVariables = scalarVariables;
this.arrayVariables = arrayVariables;
+ this.hasInputArrays = hasInputArrays;
+ this.isOutputArray = isOutputArray;
}
/**
- * 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<String> getRequiredColumnsList()
+ public List<String> getRequiredBindingsList()
{
- return new ArrayList<>(freeVariables.stream().map(IdentifierExpr::getIdentifierBindingIfIdentifier).collect(Collectors.toSet()));
+ return new ArrayList<>(
+ 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<String> getRequiredColumns()
+ public Set<String> 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<String> getScalarColumns()
+ public Set<String> 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<String> getArrayColumns()
+ public Set<String> 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<IdentifierExpr> getFreeVariables()
{
@@ -233,7 +268,7 @@ public interface Expr
}
/**
- * 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<String> getScalarVariables()
{
@@ -241,7 +276,8 @@ public interface Expr
}
/**
- * 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<String> getArrayVariables()
{
@@ -249,6 +285,30 @@ public interface Expr
}
/**
+ * 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()
+ {
+ return hasInputArrays;
+ }
+
+ /**
+ * 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()
+ {
+ return isOutputArray;
+ }
+
+ /**
* Combine with {@link BindingDetails} from {@link Expr#analyzeInputs()}
*/
public BindingDetails with(Expr other)
@@ -264,7 +324,9 @@ public interface Expr
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 || other.hasInputArrays,
+ isOutputArray || other.isOutputArray
);
}
@@ -284,7 +346,9 @@ public interface Expr
return new BindingDetails(
ImmutableSet.copyOf(Sets.union(freeVariables, moreScalars)),
ImmutableSet.copyOf(Sets.union(scalarVariables, moreScalars)),
- arrayVariables
+ arrayVariables,
+ hasInputArrays,
+ isOutputArray
);
}
@@ -304,7 +368,37 @@ public interface Expr
return new BindingDetails(
ImmutableSet.copyOf(Sets.union(freeVariables, arrayIdentifiers)),
scalarVariables,
- ImmutableSet.copyOf(Sets.union(arrayVariables, arrayIdentifiers))
+ ImmutableSet.copyOf(Sets.union(arrayVariables, arrayIdentifiers)),
+ 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 || arrayVariables.size() > 0,
+ isOutputArray
+ );
+ }
+
+ /**
+ * Copy, setting if an expression produces an array output
+ */
+ public BindingDetails withArrayOutput(boolean isOutputArray)
+ {
+ return new BindingDetails(
+ freeVariables,
+ scalarVariables,
+ arrayVariables,
+ hasInputArrays,
+ isOutputArray
);
}
@@ -315,9 +409,11 @@ public interface Expr
public BindingDetails removeLambdaArguments(Set<String> 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
);
}
}
@@ -567,28 +663,37 @@ class DoubleArrayExpr extends ConstantExpr
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()
@@ -597,12 +702,12 @@ class IdentifierExpr implements Expr
}
/**
- * 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
@@ -614,9 +719,9 @@ class IdentifierExpr implements Expr
@Nullable
@Override
- public String getIdentifierBindingIfIdentifier()
+ public String getBindingIfIdentifier()
{
- return bindingIdentifier;
+ return binding;
}
@Nullable
@@ -635,7 +740,7 @@ class IdentifierExpr implements Expr
@Override
public ExprEval eval(ObjectBinding bindings)
{
- return ExprEval.bestEffortOf(bindings.get(bindingIdentifier));
+ return ExprEval.bestEffortOf(bindings.get(binding));
}
@Override
@@ -785,7 +890,9 @@ class FunctionExpr implements Expr
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 +931,11 @@ class ApplyFunctionExpr implements Expr
}
lambdaBindingDetails = lambdaExpr.analyzeInputs();
- bindingDetails = accumulator.with(lambdaBindingDetails).withArrayArguments(function.getArrayInputs(argsExpr));
+
+ bindingDetails = accumulator.with(lambdaBindingDetails)
+ .withArrayArguments(function.getArrayInputs(argsExpr))
+ .withArrayInputs(true)
+ .withArrayOutput(function.hasArrayOutput(lambdaExpr));
argsBindingDetails = argBindingDetailsBuilder.build();
}
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 b4c91a3..825c5ed 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,11 +422,11 @@ public class ExprListenerImpl extends ExprBaseListener
/**
* 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#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/Function.java b/core/src/main/java/org/apache/druid/math/expr/Function.java
index 84c73e7..4439a66 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,6 +77,23 @@ interface Function
}
/**
+ * Returns true if a function expects any array arguments
+ */
+ default boolean hasArrayInputs()
+ {
+ return false;
+ }
+
+ /**
+ * Returns true if function produces an array. All {@link Function} implementations are expected to
+ * exclusively produce either scalar or array values.
+ */
+ default boolean hasArrayOutput()
+ {
+ return false;
+ }
+
+ /**
* Validate function arguments
*/
void validateArguments(List<Expr> args);
@@ -236,6 +253,12 @@ interface Function
}
@Override
+ public boolean hasArrayInputs()
+ {
+ return true;
+ }
+
+ @Override
public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
{
final ExprEval arrayExpr = args.get(0).eval(bindings);
@@ -275,6 +298,12 @@ interface Function
}
@Override
+ public boolean hasArrayInputs()
+ {
+ return true;
+ }
+
+ @Override
public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
{
final ExprEval arrayExpr1 = args.get(0).eval(bindings);
@@ -1802,23 +1831,29 @@ interface Function
@Override
+ public Set<Expr> getScalarInputs(List<Expr> args)
+ {
+ return ImmutableSet.copyOf(args);
+ }
+
+ @Override
public Set<Expr> getArrayInputs(List<Expr> args)
{
return Collections.emptySet();
}
@Override
- public void validateArguments(List<Expr> args)
+ public boolean hasArrayOutput()
{
- if (!(args.size() > 0)) {
- throw new IAE("Function[%s] needs at least 1 argument", name());
- }
+ return true;
}
@Override
- public Set<Expr> getScalarInputs(List<Expr> args)
+ public void validateArguments(List<Expr> args)
{
- return ImmutableSet.copyOf(args);
+ if (args.isEmpty()) {
+ throw new IAE("Function[%s] needs at least 1 argument", name());
+ }
}
}
@@ -1853,6 +1888,12 @@ interface Function
}
@Override
+ public boolean hasArrayInputs()
+ {
+ return true;
+ }
+
+ @Override
public void validateArguments(List<Expr> args)
{
if (args.size() != 1) {
@@ -1901,6 +1942,12 @@ interface Function
{
return ImmutableSet.copyOf(args);
}
+
+ @Override
+ public boolean hasArrayOutput()
+ {
+ return true;
+ }
}
class ArrayToStringFunction extends ArrayScalarFunction
@@ -2037,6 +2084,12 @@ interface Function
}
@Override
+ public boolean hasArrayOutput()
+ {
+ return true;
+ }
+
+ @Override
ExprEval doApply(ExprEval arrayExpr, ExprEval scalarExpr)
{
switch (arrayExpr.type()) {
@@ -2081,6 +2134,18 @@ interface Function
}
@Override
+ public Set<Expr> getArrayInputs(List<Expr> args)
+ {
+ return ImmutableSet.copyOf(args);
+ }
+
+ @Override
+ public boolean hasArrayOutput()
+ {
+ return true;
+ }
+
+ @Override
ExprEval doApply(ExprEval lhsExpr, ExprEval rhsExpr)
{
final Object[] array1 = lhsExpr.asArray();
@@ -2119,12 +2184,6 @@ interface Function
l.addAll(Arrays.asList(array2));
return l.stream();
}
-
- @Override
- public Set<Expr> getArrayInputs(List<Expr> args)
- {
- return ImmutableSet.copyOf(args);
- }
}
class ArrayContainsFunction extends ArraysFunction
@@ -2136,6 +2195,12 @@ interface Function
}
@Override
+ public boolean hasArrayOutput()
+ {
+ return true;
+ }
+
+ @Override
ExprEval doApply(ExprEval lhsExpr, ExprEval rhsExpr)
{
final Object[] array1 = lhsExpr.asArray();
@@ -2182,6 +2247,34 @@ interface Function
}
@Override
+ public Set<Expr> getScalarInputs(List<Expr> args)
+ {
+ if (args.size() == 3) {
+ return ImmutableSet.of(args.get(1), args.get(2));
+ } else {
+ return ImmutableSet.of(args.get(1));
+ }
+ }
+
+ @Override
+ public Set<Expr> getArrayInputs(List<Expr> args)
+ {
+ return ImmutableSet.of(args.get(0));
+ }
+
+ @Override
+ public boolean hasArrayInputs()
+ {
+ return true;
+ }
+
+ @Override
+ public boolean hasArrayOutput()
+ {
+ return true;
+ }
+
+ @Override
public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
{
final ExprEval expr = args.get(0).eval(bindings);
@@ -2214,38 +2307,46 @@ interface Function
}
throw new RE("Unable to slice to unknown type %s", expr.type());
}
+ }
+ class ArrayPrependFunction implements Function
+ {
@Override
- public Set<Expr> getScalarInputs(List<Expr> 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<Expr> args)
+ {
+ if (args.size() != 2) {
+ throw new IAE("Function[%s] needs 2 arguments", name());
}
}
@Override
- public Set<Expr> getArrayInputs(List<Expr> args)
+ public Set<Expr> getScalarInputs(List<Expr> args)
{
return ImmutableSet.of(args.get(0));
}
- }
- class ArrayPrependFunction implements Function
- {
@Override
- public String name()
+ public Set<Expr> getArrayInputs(List<Expr> args)
{
- return "array_prepend";
+ return ImmutableSet.of(args.get(1));
}
@Override
- public void validateArguments(List<Expr> 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 +2381,11 @@ interface Function
throw new RE("Unable to prepend to unknown type %s", arrayExpr.type());
}
-
private <T> Stream<T> prepend(T val, T[] array)
{
List<T> l = new ArrayList<>(Arrays.asList(array));
l.add(0, val);
return l.stream();
}
- @Override
- public Set<Expr> getScalarInputs(List<Expr> args)
- {
- return ImmutableSet.of(args.get(0));
- }
-
- @Override
- public Set<Expr> getArrayInputs(List<Expr> args)
- {
- return ImmutableSet.of(args.get(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 55144ab..ce0c35f 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 class Parser
* 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<String> toApply)
+ public static Expr applyUnappliedBindings(Expr expr, Expr.BindingDetails bindingDetails, List<String> bindingsToApply)
{
- if (toApply.size() == 0) {
+ if (bindingsToApply.isEmpty()) {
+ // nothing to do, expression is fine as is
return expr;
}
- List<String> unapplied = toApply.stream()
- .filter(x -> bindingDetails.getRequiredColumns().contains(x))
+ // filter the list of bindings to those which are used in this expression
+ List<String> 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 class Parser
List<Expr> 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 class Parser
Expr.BindingDetails newExprBindings = newExpr.analyzeInputs();
final Set<String> expectedArrays = newExprBindings.getArrayVariables();
- List<String> remainingUnappliedArgs =
- unapplied.stream().filter(x -> !expectedArrays.contains(x)).collect(Collectors.toList());
+
+ List<String> 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.size() == 0) {
+ 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<String> unapplied)
+ private static Expr applyUnapplied(Expr expr, List<String> unappliedBindings)
{
- // wrap an expression in either map or cartesian_map to apply any unapplied identifiers
final Map<IdentifierExpr, IdentifierExpr> toReplace = new HashMap<>();
+
+ // filter to get list of IdentifierExpr that are backed by the unapplied bindings
final List<IdentifierExpr> args = expr.analyzeInputs()
.getFreeVariables()
.stream()
- .filter(x -> unapplied.contains(x.getBindingIdentifier()))
+ .filter(x -> unappliedBindings.contains(x.getBinding()))
.collect(Collectors.toList());
final List<IdentifierExpr> 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 @@ public class Parser
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 @@ public class Parser
// recursively evaluate arguments to ensure they are properly transformed into arrays as necessary
Set<String> unappliedInThisApply =
unappliedArgs.stream()
- .filter(u -> !expr.bindingDetails.getArrayColumns().contains(u))
+ .filter(u -> !expr.bindingDetails.getArrayBindings().contains(u))
.collect(Collectors.toSet());
List<String> unappliedIdentifiers =
expr.bindingDetails
.getFreeVariables()
.stream()
- .filter(x -> unappliedInThisApply.contains(x.getIdentifierBindingIfIdentifier()))
+ .filter(x -> unappliedInThisApply.contains(x.getBindingIfIdentifier()))
.map(IdentifierExpr::getIdentifierIfIdentifier)
.collect(Collectors.toList());
List<Expr> 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,11 +307,11 @@ public class Parser
List<IdentifierExpr> 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.size() == 0) {
+ if (unappliedLambdaBindings.isEmpty()) {
return new ApplyFunctionExpr(expr.function, expr.name, expr.lambdaExpr, newArgs);
}
@@ -386,8 +393,8 @@ public class Parser
public static void validateExpr(Expr expression, Expr.BindingDetails bindingDetails)
{
final Set<String> conflicted =
- Sets.intersection(bindingDetails.getScalarColumns(), bindingDetails.getArrayColumns());
- if (conflicted.size() != 0) {
+ 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 f70a716..cac4733 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 @@ public class ParserTest
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 @@ public class ParserTest
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 6995d57..6b3643a 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 abstract class SimpleDoubleAggregatorFactory extends NullableAggregatorFa
{
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 6619126..78e86de 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 abstract class SimpleFloatAggregatorFactory extends NullableAggregatorFac
{
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 308100c..cb2d66f 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 abstract class SimpleLongAggregatorFactory extends NullableAggregatorFact
{
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 e978f18..9184e14 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 @@ public class ExpressionPostAggregator implements PostAggregator
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 0a67652..ec4a6d8 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 class ExpressionDimFilter implements DimFilter
@Override
public HashSet<String> 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/column/ColumnCapabilitiesImpl.java b/processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilitiesImpl.java
index 94cdbe3..3f827bf 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
@@ -43,16 +43,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()
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 f46cb8d..c721e9e 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> 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 edd2aa6..1348029 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 class ExpressionSelectors
{
final Expr.BindingDetails exprDetails = expression.analyzeInputs();
Parser.validateExpr(expression, exprDetails);
- final List<String> columns = exprDetails.getRequiredColumnsList();
+ final List<String> columns = exprDetails.getRequiredBindingsList();
if (columns.size() == 1) {
final String column = Iterables.getOnlyElement(columns);
@@ -160,8 +160,8 @@ public class ExpressionSelectors
&& capabilities.isDictionaryEncoded()
&& capabilities.isComplete()
&& !capabilities.hasMultipleValues()
- && !exprDetails.getArrayColumns().contains(column)) {
- // Optimization for expressions that hit one string column and nothing else.
+ && 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)),
expression
@@ -176,11 +176,11 @@ public class ExpressionSelectors
final List<String> 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,21 +219,29 @@ public class ExpressionSelectors
{
final Expr.BindingDetails exprDetails = expression.analyzeInputs();
Parser.validateExpr(expression, exprDetails);
- final List<String> columns = exprDetails.getRequiredColumnsList();
+ final List<String> columns = exprDetails.getRequiredBindingsList();
if (columns.size() == 1) {
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()
- && !capabilities.hasMultipleValues()
- && !exprDetails.getArrayColumns().contains(column)
+ && !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
@@ -249,7 +257,7 @@ public class ExpressionSelectors
final ColumnValueSelector<ExprEval> 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) {
@@ -355,7 +363,7 @@ public class ExpressionSelectors
)
{
final Map<String, Supplier<Object>> suppliers = new HashMap<>();
- final List<String> columns = bindingDetails.getRequiredColumnsList();
+ final List<String> columns = bindingDetails.getRequiredBindingsList();
for (String columnName : columns) {
final ColumnCapabilities columnCapabilities = columnSelectorFactory
.getColumnCapabilities(columnName);
@@ -530,7 +538,7 @@ public class ExpressionSelectors
} 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 c322fff..0d9feef 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 class ExpressionVirtualColumn implements VirtualColumn
@Override
public List<String> 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 719f664..19d9ebf 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 class RowBasedExpressionColumnValueSelector extends ExpressionColumnValue
{
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 class RowBasedExpressionColumnValueSelector extends ExpressionColumnValue
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 8fb9cdc..df9241b 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 class SingleLongInputCachingExpressionColumnValueSelector implements Colu
)
{
// 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 0b8c443..a3bf08c 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 class SingleStringInputCachingExpressionColumnValueSelector implements Co
)
{
// 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 074ad73..00c9d58 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 class SingleStringInputDimensionSelector implements DimensionSelector
)
{
// 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");
}
@@ -71,15 +71,12 @@ public class SingleStringInputDimensionSelector implements DimensionSelector
}
/**
- * 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 5689258..7ec9ed9 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,8 @@ public class MultiValuedDimensionTest
@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.");
- }
+ // 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 d157488..af4a207 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.Supplier;
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;
@@ -1012,15 +1011,9 @@ public class GroupByQueryRunnerTest
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)
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 c420642..a77aa8e 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
@@ -288,26 +288,21 @@ public abstract class BaseFilterTest
final Sequence<Cursor> cursors = makeCursorSequence(makeFilter(filter));
Sequence<List<String>> seq = Sequences.map(
cursors,
- new Function<Cursor, List<String>>()
- {
- @Override
- public List<String> apply(Cursor input)
- {
- final DimensionSelector selector = input
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec(selectColumn, selectColumn));
-
- final List<String> values = new ArrayList<>();
-
- while (!input.isDone()) {
- IndexedInts row = selector.getRow();
- Preconditions.checkState(row.size() == 1);
- values.add(selector.lookupName(row.get(0)));
- input.advance();
- }
+ cursor -> {
+ final DimensionSelector selector = cursor
+ .getColumnSelectorFactory()
+ .makeDimensionSelector(new DefaultDimensionSpec(selectColumn, selectColumn));
- return values;
+ final List<String> values = new ArrayList<>();
+
+ while (!cursor.isDone()) {
+ IndexedInts row = selector.getRow();
+ Preconditions.checkState(row.size() == 1);
+ values.add(selector.lookupName(row.get(0)));
+ cursor.advance();
}
+
+ return values;
}
);
return seq.toList().get(0);
@@ -417,26 +412,21 @@ public abstract class BaseFilterTest
final Sequence<Cursor> cursors = makeCursorSequence(postFilteringFilter);
Sequence<List<String>> seq = Sequences.map(
cursors,
- new Function<Cursor, List<String>>()
- {
- @Override
- public List<String> apply(Cursor input)
- {
- final DimensionSelector selector = input
- .getColumnSelectorFactory()
- .makeDimensionSelector(new DefaultDimensionSpec(selectColumn, selectColumn));
-
- final List<String> values = new ArrayList<>();
-
- while (!input.isDone()) {
- IndexedInts row = selector.getRow();
- Preconditions.checkState(row.size() == 1);
- values.add(selector.lookupName(row.get(0)));
- input.advance();
- }
+ cursor -> {
+ final DimensionSelector selector = cursor
+ .getColumnSelectorFactory()
+ .makeDimensionSelector(new DefaultDimensionSpec(selectColumn, selectColumn));
- return values;
+ final List<String> values = new ArrayList<>();
+
+ while (!cursor.isDone()) {
+ IndexedInts row = selector.getRow();
+ Preconditions.checkState(row.size() == 1);
+ values.add(selector.lookupName(row.get(0)));
+ cursor.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 578e4d2..caa0f8a 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 class ExpressionFilterTest extends BaseFilterTest
}
assertFilterMatchesSkipVectorize(edf("dim4 == '1'"), ImmutableList.of("0"));
assertFilterMatchesSkipVectorize(edf("dim4 == '3'"), ImmutableList.of("3"));
+ assertFilterMatchesSkipVectorize(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 9da1ac4..1244850 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
@@ -8226,8 +8226,9 @@ public class CalciteQueryTest extends BaseCalciteQueryTest
List<Object[]> 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}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org