You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by cw...@apache.org on 2019/06/01 03:56:37 UTC
[incubator-druid] branch master updated: Fix repeated expr parsing
in ExpressionPostAggregation (#7791)
This is an automated email from the ASF dual-hosted git repository.
cwylie 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 55af692 Fix repeated expr parsing in ExpressionPostAggregation (#7791)
55af692 is described below
commit 55af692b5606242d74d8cbf954b05ee891680e98
Author: litao91 <li...@gmail.com>
AuthorDate: Sat Jun 1 11:56:31 2019 +0800
Fix repeated expr parsing in ExpressionPostAggregation (#7791)
* Fix repeatedly expr parsing in ExpressionPostAggregation
Change-Id: Ib739fb1cbc460afeb59a255f635305441dc6997b
* Style fix and avoid code copying
Change-Id: I2d6ba3d1ae37f1fb84b6f7eaab5dab817e1980ec
* Lazilly parse expressions in ExpressionVirtualColumn and ExpressionDimFilter
Change-Id: I5ae2bb3ef9a18fbbfb5e0780c86f6bc0039edc83
---
.../aggregation/post/ExpressionPostAggregator.java | 58 +++++++++++++++++++---
.../druid/query/filter/ExpressionDimFilter.java | 8 +--
.../druid/segment/filter/ExpressionFilter.java | 26 +++++-----
.../segment/virtual/ExpressionVirtualColumn.java | 12 +++--
4 files changed, 77 insertions(+), 27 deletions(-)
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 6463f80..8b35a03 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
@@ -24,6 +24,8 @@ import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Function;
import com.google.common.base.Preconditions;
+import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
@@ -63,8 +65,8 @@ public class ExpressionPostAggregator implements PostAggregator
private final ExprMacroTable macroTable;
private final Map<String, Function<Object, Object>> finalizers;
- private final Expr parsed;
- private final Set<String> dependentFields;
+ private final Supplier<Expr> parsed;
+ private final Supplier<Set<String>> dependentFields;
/**
* Constructor for serialization.
@@ -91,6 +93,45 @@ public class ExpressionPostAggregator implements PostAggregator
final Map<String, Function<Object, Object>> finalizers
)
{
+ this(
+ name,
+ expression,
+ ordering,
+ macroTable,
+ finalizers,
+ Suppliers.memoize(() -> Parser.parse(expression, macroTable))
+ );
+ }
+
+ private ExpressionPostAggregator(
+ final String name,
+ final String expression,
+ @Nullable final String ordering,
+ final ExprMacroTable macroTable,
+ final Map<String, Function<Object, Object>> finalizers,
+ final Supplier<Expr> parsed
+ )
+ {
+ this(
+ name,
+ expression,
+ ordering,
+ macroTable,
+ finalizers,
+ parsed,
+ Suppliers.memoize(() -> ImmutableSet.copyOf(Parser.findRequiredBindings(parsed.get()))));
+ }
+
+ private ExpressionPostAggregator(
+ final String name,
+ final String expression,
+ @Nullable final String ordering,
+ final ExprMacroTable macroTable,
+ final Map<String, Function<Object, Object>> finalizers,
+ final Supplier<Expr> parsed,
+ final Supplier<Set<String>> dependentFields
+ )
+ {
Preconditions.checkArgument(expression != null, "expression cannot be null");
this.name = name;
@@ -100,14 +141,15 @@ public class ExpressionPostAggregator implements PostAggregator
this.macroTable = macroTable;
this.finalizers = finalizers;
- this.parsed = Parser.parse(expression, macroTable);
- this.dependentFields = ImmutableSet.copyOf(Parser.findRequiredBindings(parsed));
+ this.parsed = parsed;
+ this.dependentFields = dependentFields;
}
+
@Override
public Set<String> getDependentFields()
{
- return dependentFields;
+ return dependentFields.get();
}
@Override
@@ -128,7 +170,7 @@ public class ExpressionPostAggregator implements PostAggregator
}
);
- return parsed.eval(Parser.withMap(finalizedValues)).value();
+ return parsed.get().eval(Parser.withMap(finalizedValues)).value();
}
@Override
@@ -151,7 +193,9 @@ public class ExpressionPostAggregator implements PostAggregator
entry -> entry.getKey(),
entry -> entry.getValue()::finalizeComputation
)
- )
+ ),
+ parsed,
+ dependentFields
);
}
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 75061ca..9a34800 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
@@ -22,6 +22,8 @@ package org.apache.druid.query.filter;
import com.fasterxml.jackson.annotation.JacksonInject;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;
import com.google.common.collect.RangeSet;
import com.google.common.collect.Sets;
import org.apache.druid.math.expr.Expr;
@@ -36,7 +38,7 @@ import java.util.Objects;
public class ExpressionDimFilter implements DimFilter
{
private final String expression;
- private final Expr parsed;
+ private final Supplier<Expr> parsed;
@JsonCreator
public ExpressionDimFilter(
@@ -45,7 +47,7 @@ public class ExpressionDimFilter implements DimFilter
)
{
this.expression = expression;
- this.parsed = Parser.parse(expression, macroTable);
+ this.parsed = Suppliers.memoize(() -> Parser.parse(expression, macroTable));
}
@JsonProperty
@@ -75,7 +77,7 @@ public class ExpressionDimFilter implements DimFilter
@Override
public HashSet<String> getRequiredColumns()
{
- return Sets.newHashSet(Parser.findRequiredBindings(parsed));
+ return Sets.newHashSet(Parser.findRequiredBindings(parsed.get()));
}
@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 208ea4d..f1ccf82 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
@@ -19,6 +19,8 @@
package org.apache.druid.segment.filter;
+import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import org.apache.druid.common.config.NullHandling;
@@ -41,19 +43,19 @@ import java.util.Set;
public class ExpressionFilter implements Filter
{
- private final Expr expr;
- private final Set<String> requiredBindings;
+ private final Supplier<Expr> expr;
+ private final Supplier<Set<String>> requiredBindings;
- public ExpressionFilter(final Expr expr)
+ public ExpressionFilter(final Supplier<Expr> expr)
{
this.expr = expr;
- this.requiredBindings = ImmutableSet.copyOf(Parser.findRequiredBindings(expr));
+ this.requiredBindings = Suppliers.memoize(() -> ImmutableSet.copyOf(Parser.findRequiredBindings(expr.get())));
}
@Override
public ValueMatcher makeMatcher(final ColumnSelectorFactory factory)
{
- final ColumnValueSelector<ExprEval> selector = ExpressionSelectors.makeExprEvalSelector(factory, expr);
+ final ColumnValueSelector<ExprEval> selector = ExpressionSelectors.makeExprEvalSelector(factory, expr.get());
return new ValueMatcher()
{
@Override
@@ -76,14 +78,14 @@ public class ExpressionFilter implements Filter
@Override
public boolean supportsBitmapIndex(final BitmapIndexSelector selector)
{
- if (requiredBindings.isEmpty()) {
+ if (requiredBindings.get().isEmpty()) {
// Constant expression.
return true;
- } else if (requiredBindings.size() == 1) {
+ } else if (requiredBindings.get().size() == 1) {
// Single-column expression. We can use bitmap indexes if this column has an index and does not have
// multiple values. The lack of multiple values is important because expression filters treat multi-value
// arrays as nulls, which doesn't permit index based filtering.
- final String column = Iterables.getOnlyElement(requiredBindings);
+ final String column = Iterables.getOnlyElement(requiredBindings.get());
return selector.getBitmapIndex(column) != null && !selector.hasMultipleValues(column);
} else {
// Multi-column expression.
@@ -94,9 +96,9 @@ public class ExpressionFilter implements Filter
@Override
public <T> T getBitmapResult(final BitmapIndexSelector selector, final BitmapResultFactory<T> bitmapResultFactory)
{
- if (requiredBindings.isEmpty()) {
+ if (requiredBindings.get().isEmpty()) {
// Constant expression.
- if (expr.eval(ExprUtils.nilBindings()).asBoolean()) {
+ if (expr.get().eval(ExprUtils.nilBindings()).asBoolean()) {
return bitmapResultFactory.wrapAllTrue(Filters.allTrue(selector));
} else {
return bitmapResultFactory.wrapAllFalse(Filters.allFalse(selector));
@@ -104,12 +106,12 @@ public class ExpressionFilter implements Filter
} else {
// Can assume there's only one binding and it has a bitmap index, otherwise supportsBitmapIndex would have
// returned false and the caller should not have called us.
- final String column = Iterables.getOnlyElement(requiredBindings);
+ final String column = Iterables.getOnlyElement(requiredBindings.get());
return Filters.matchPredicate(
column,
selector,
bitmapResultFactory,
- value -> expr.eval(identifierName -> {
+ value -> expr.get().eval(identifierName -> {
// There's only one binding, and it must be the single column, so it can safely be ignored in production.
assert column.equals(identifierName);
// convert null to Empty before passing to expressions if needed.
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 45a25c4..d029f7f 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
@@ -23,6 +23,8 @@ import com.fasterxml.jackson.annotation.JacksonInject;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Preconditions;
+import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;
import org.apache.druid.math.expr.Expr;
import org.apache.druid.math.expr.ExprMacroTable;
import org.apache.druid.math.expr.Parser;
@@ -44,7 +46,7 @@ public class ExpressionVirtualColumn implements VirtualColumn
private final String name;
private final String expression;
private final ValueType outputType;
- private final Expr parsedExpression;
+ private final Supplier<Expr> parsedExpression;
@JsonCreator
public ExpressionVirtualColumn(
@@ -57,7 +59,7 @@ public class ExpressionVirtualColumn implements VirtualColumn
this.name = Preconditions.checkNotNull(name, "name");
this.expression = Preconditions.checkNotNull(expression, "expression");
this.outputType = outputType != null ? outputType : ValueType.FLOAT;
- this.parsedExpression = Parser.parse(expression, macroTable);
+ this.parsedExpression = Suppliers.memoize(() -> Parser.parse(expression, macroTable));
}
@JsonProperty("name")
@@ -88,7 +90,7 @@ public class ExpressionVirtualColumn implements VirtualColumn
return dimensionSpec.decorate(
ExpressionSelectors.makeDimensionSelector(
columnSelectorFactory,
- parsedExpression,
+ parsedExpression.get(),
dimensionSpec.getExtractionFn()
)
);
@@ -97,7 +99,7 @@ public class ExpressionVirtualColumn implements VirtualColumn
@Override
public ColumnValueSelector<?> makeColumnValueSelector(String columnName, ColumnSelectorFactory factory)
{
- return ExpressionSelectors.makeColumnValueSelector(factory, parsedExpression);
+ return ExpressionSelectors.makeColumnValueSelector(factory, parsedExpression.get());
}
@Override
@@ -109,7 +111,7 @@ public class ExpressionVirtualColumn implements VirtualColumn
@Override
public List<String> requiredColumns()
{
- return Parser.findRequiredBindings(parsedExpression);
+ return Parser.findRequiredBindings(parsedExpression.get());
}
@Override
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org