From a4164ebfd43015b30c243f2d61d4e65b2c02f147 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Fri, 5 Mar 2021 16:31:44 -0800 Subject: [PATCH 1/6] Add explicit EOF; use assert instead of precondition --- core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 | 2 ++ .../apache/druid/sql/calcite/filtration/BottomUpTransform.java | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 b/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 index 9b9eda9dfded..a04f7a499db5 100644 --- a/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 +++ b/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 @@ -15,6 +15,8 @@ grammar Expr; +start : expr EOF ; + expr : NULL # null | ('-'|'!') expr # unaryOpExpr | expr '^' expr # powOpExpr diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/filtration/BottomUpTransform.java b/sql/src/main/java/org/apache/druid/sql/calcite/filtration/BottomUpTransform.java index 9aacde425ab4..f594878b2947 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/filtration/BottomUpTransform.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/filtration/BottomUpTransform.java @@ -36,7 +36,8 @@ public abstract class BottomUpTransform implements Function Date: Sun, 7 Mar 2021 15:42:20 -0800 Subject: [PATCH 2/6] add caching --- .../apache/druid/query/JoinDataSource.java | 26 ++++ .../DoubleMaxAggregatorFactory.java | 13 ++ .../DoubleMinAggregatorFactory.java | 13 ++ .../DoubleSumAggregatorFactory.java | 13 ++ .../FloatMaxAggregatorFactory.java | 13 ++ .../FloatMinAggregatorFactory.java | 13 ++ .../FloatSumAggregatorFactory.java | 13 ++ .../aggregation/LongMaxAggregatorFactory.java | 13 ++ .../aggregation/LongMinAggregatorFactory.java | 13 ++ .../aggregation/LongSumAggregatorFactory.java | 13 ++ .../SimpleDoubleAggregatorFactory.java | 19 ++- .../SimpleFloatAggregatorFactory.java | 19 ++- .../SimpleLongAggregatorFactory.java | 19 ++- .../post/ExpressionPostAggregator.java | 20 +++ .../query/filter/ExpressionDimFilter.java | 7 + .../segment/join/JoinConditionAnalysis.java | 29 ++++- .../virtual/ExpressionVirtualColumn.java | 12 +- .../aggregation/builtin/AvgSqlAggregator.java | 1 + .../aggregation/builtin/MaxSqlAggregator.java | 36 ++++- .../aggregation/builtin/MinSqlAggregator.java | 36 ++++- .../builtin/SimpleSqlAggregator.java | 3 +- .../aggregation/builtin/SumSqlAggregator.java | 36 ++++- .../calcite/expression/DruidExpression.java | 20 ++- .../sql/calcite/expression/Expressions.java | 25 ++-- .../ArrayContainsOperatorConversion.java | 3 +- .../ArrayOverlapOperatorConversion.java | 3 +- ...ExpressionDimFilterOperatorConversion.java | 2 +- .../builtin/CastOperatorConversion.java | 8 +- .../builtin/DateTruncOperatorConversion.java | 6 +- .../QueryLookupOperatorConversion.java | 4 +- .../RegexpExtractOperatorConversion.java | 4 +- .../builtin/TimeFloorOperatorConversion.java | 9 +- .../builtin/TruncateOperatorConversion.java | 2 +- .../calcite/planner/CachingExprParser.java | 79 +++++++++++ .../calcite/planner/CalcitePlannerModule.java | 6 +- .../sql/calcite/planner/DruidRexExecutor.java | 3 +- .../sql/calcite/planner/PlannerConfig.java | 20 ++- .../sql/calcite/planner/PlannerContext.java | 7 + .../sql/calcite/rel/DruidJoinQueryRel.java | 2 +- .../druid/sql/calcite/rel/DruidQuery.java | 4 +- .../druid/sql/calcite/rel/Grouping.java | 6 +- .../druid/sql/calcite/rel/Projection.java | 3 +- .../calcite/rel/VirtualColumnRegistry.java | 4 +- .../sql/calcite/SqlPlannerExprCacheTest.java | 123 ++++++++++++++++++ .../planner/CachingExprParserTest.java | 105 +++++++++++++++ .../planner/CalcitePlannerModuleTest.java | 51 +++++++- 46 files changed, 796 insertions(+), 83 deletions(-) create mode 100644 sql/src/main/java/org/apache/druid/sql/calcite/planner/CachingExprParser.java create mode 100644 sql/src/test/java/org/apache/druid/sql/calcite/SqlPlannerExprCacheTest.java create mode 100644 sql/src/test/java/org/apache/druid/sql/calcite/planner/CachingExprParserTest.java diff --git a/processing/src/main/java/org/apache/druid/query/JoinDataSource.java b/processing/src/main/java/org/apache/druid/query/JoinDataSource.java index 0855e29f9889..d0da4e76254e 100644 --- a/processing/src/main/java/org/apache/druid/query/JoinDataSource.java +++ b/processing/src/main/java/org/apache/druid/query/JoinDataSource.java @@ -23,9 +23,11 @@ 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.collect.ImmutableList; import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.query.filter.DimFilter; import org.apache.druid.segment.join.JoinConditionAnalysis; @@ -114,6 +116,30 @@ public static JoinDataSource create( ); } + public static JoinDataSource create( + DataSource left, + DataSource right, + String rightPrefix, + String condition, + JoinType joinType, + DimFilter leftFilter, + Supplier conditionExprSupplier + ) + { + return new JoinDataSource( + left, + right, + StringUtils.nullToEmptyNonDruidDataString(rightPrefix), + JoinConditionAnalysis.forExpression( + Preconditions.checkNotNull(condition, "condition"), + StringUtils.nullToEmptyNonDruidDataString(rightPrefix), + conditionExprSupplier + ), + joinType, + leftFilter + ); + } + /** * Create a join dataSource from an existing {@link JoinConditionAnalysis}. */ diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/DoubleMaxAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/DoubleMaxAggregatorFactory.java index ce22009f2577..f986d9a5cc51 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/DoubleMaxAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/DoubleMaxAggregatorFactory.java @@ -22,7 +22,9 @@ 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 org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.segment.BaseDoubleColumnValueSelector; import org.apache.druid.segment.vector.VectorColumnSelectorFactory; @@ -48,6 +50,17 @@ public DoubleMaxAggregatorFactory( super(macroTable, name, fieldName, expression); } + public DoubleMaxAggregatorFactory( + String name, + String fieldName, + @Nullable String expression, + Supplier expressionSupplier, + ExprMacroTable macroTable + ) + { + super(macroTable, name, fieldName, expression, expressionSupplier); + } + public DoubleMaxAggregatorFactory(String name, String fieldName) { this(name, fieldName, null, ExprMacroTable.nil()); diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/DoubleMinAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/DoubleMinAggregatorFactory.java index 9c068ce6404b..412ae8924286 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/DoubleMinAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/DoubleMinAggregatorFactory.java @@ -22,7 +22,9 @@ 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 org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.segment.BaseDoubleColumnValueSelector; import org.apache.druid.segment.vector.VectorColumnSelectorFactory; @@ -48,6 +50,17 @@ public DoubleMinAggregatorFactory( super(macroTable, name, fieldName, expression); } + public DoubleMinAggregatorFactory( + String name, + String fieldName, + @Nullable String expression, + Supplier expressionSupplier, + ExprMacroTable macroTable + ) + { + super(macroTable, name, fieldName, expression, expressionSupplier); + } + public DoubleMinAggregatorFactory(String name, String fieldName) { this(name, fieldName, null, ExprMacroTable.nil()); diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/DoubleSumAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/DoubleSumAggregatorFactory.java index 1753d18103cf..2c26470abd6b 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/DoubleSumAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/DoubleSumAggregatorFactory.java @@ -22,7 +22,9 @@ 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 org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.segment.BaseDoubleColumnValueSelector; import org.apache.druid.segment.vector.VectorColumnSelectorFactory; @@ -48,6 +50,17 @@ public DoubleSumAggregatorFactory( super(macroTable, name, fieldName, expression); } + public DoubleSumAggregatorFactory( + String name, + String fieldName, + @Nullable String expression, + Supplier expressionSupplier, + ExprMacroTable macroTable + ) + { + super(macroTable, name, fieldName, expression, expressionSupplier); + } + public DoubleSumAggregatorFactory(String name, String fieldName) { this(name, fieldName, null, ExprMacroTable.nil()); diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/FloatMaxAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/FloatMaxAggregatorFactory.java index 9398f622250a..34fbb427e867 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/FloatMaxAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/FloatMaxAggregatorFactory.java @@ -22,7 +22,9 @@ 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 org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.segment.BaseFloatColumnValueSelector; import org.apache.druid.segment.vector.VectorColumnSelectorFactory; @@ -48,6 +50,17 @@ public FloatMaxAggregatorFactory( super(macroTable, name, fieldName, expression); } + public FloatMaxAggregatorFactory( + String name, + String fieldName, + @Nullable String expression, + Supplier expressionSupplier, + ExprMacroTable macroTable + ) + { + super(macroTable, name, fieldName, expression, expressionSupplier); + } + public FloatMaxAggregatorFactory(String name, String fieldName) { this(name, fieldName, null, ExprMacroTable.nil()); diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/FloatMinAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/FloatMinAggregatorFactory.java index 465aa381d41e..915a0d07bff7 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/FloatMinAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/FloatMinAggregatorFactory.java @@ -22,7 +22,9 @@ 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 org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.segment.BaseFloatColumnValueSelector; import org.apache.druid.segment.vector.VectorColumnSelectorFactory; @@ -48,6 +50,17 @@ public FloatMinAggregatorFactory( super(macroTable, name, fieldName, expression); } + public FloatMinAggregatorFactory( + String name, + String fieldName, + @Nullable String expression, + Supplier expressionSupplier, + ExprMacroTable macroTable + ) + { + super(macroTable, name, fieldName, expression, expressionSupplier); + } + public FloatMinAggregatorFactory(String name, String fieldName) { this(name, fieldName, null, ExprMacroTable.nil()); diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregatorFactory.java index 3175b401ad29..5b48004a1324 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/FloatSumAggregatorFactory.java @@ -22,7 +22,9 @@ 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 org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.segment.BaseFloatColumnValueSelector; import org.apache.druid.segment.vector.VectorColumnSelectorFactory; @@ -48,6 +50,17 @@ public FloatSumAggregatorFactory( super(macroTable, name, fieldName, expression); } + public FloatSumAggregatorFactory( + String name, + String fieldName, + @Nullable String expression, + Supplier expressionSupplier, + ExprMacroTable macroTable + ) + { + super(macroTable, name, fieldName, expression, expressionSupplier); + } + public FloatSumAggregatorFactory(String name, String fieldName) { this(name, fieldName, null, ExprMacroTable.nil()); diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/LongMaxAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/LongMaxAggregatorFactory.java index a9bc7c5ae57b..704a4eecbb25 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/LongMaxAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/LongMaxAggregatorFactory.java @@ -22,7 +22,9 @@ 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 org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.segment.BaseLongColumnValueSelector; import org.apache.druid.segment.vector.VectorColumnSelectorFactory; @@ -48,6 +50,17 @@ public LongMaxAggregatorFactory( super(macroTable, name, fieldName, expression); } + public LongMaxAggregatorFactory( + String name, + String fieldName, + @Nullable String expression, + Supplier expressionSupplier, + ExprMacroTable macroTable + ) + { + super(macroTable, name, fieldName, expression, expressionSupplier); + } + public LongMaxAggregatorFactory(String name, String fieldName) { this(name, fieldName, null, ExprMacroTable.nil()); diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/LongMinAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/LongMinAggregatorFactory.java index 18a3ee684849..ed36e1a84e9a 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/LongMinAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/LongMinAggregatorFactory.java @@ -22,7 +22,9 @@ 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 org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.segment.BaseLongColumnValueSelector; import org.apache.druid.segment.vector.VectorColumnSelectorFactory; @@ -48,6 +50,17 @@ public LongMinAggregatorFactory( super(macroTable, name, fieldName, expression); } + public LongMinAggregatorFactory( + String name, + String fieldName, + @Nullable String expression, + Supplier expressionSupplier, + ExprMacroTable macroTable + ) + { + super(macroTable, name, fieldName, expression, expressionSupplier); + } + public LongMinAggregatorFactory(String name, String fieldName) { this(name, fieldName, null, ExprMacroTable.nil()); diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregatorFactory.java index 8c9364f0de59..85a8518d140a 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregatorFactory.java @@ -22,7 +22,9 @@ 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 org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.segment.BaseLongColumnValueSelector; import org.apache.druid.segment.vector.VectorColumnSelectorFactory; @@ -48,6 +50,17 @@ public LongSumAggregatorFactory( super(macroTable, name, fieldName, expression); } + public LongSumAggregatorFactory( + String name, + String fieldName, + @Nullable String expression, + Supplier expressionSupplier, + ExprMacroTable macroTable + ) + { + super(macroTable, name, fieldName, expression, expressionSupplier); + } + public LongSumAggregatorFactory(String name, String fieldName) { this(name, fieldName, null, ExprMacroTable.nil()); 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 b8644329c71f..28a5c91a8de0 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 @@ -66,13 +66,30 @@ public SimpleDoubleAggregatorFactory( @Nullable final String fieldName, @Nullable String expression ) + { + this( + macroTable, + name, + fieldName, + expression, + Suppliers.memoize(() -> expression == null ? null : Parser.parse(expression, macroTable)) + ); + } + + public SimpleDoubleAggregatorFactory( + ExprMacroTable macroTable, + String name, + String fieldName, + @Nullable String expression, + Supplier expressionSupplier + ) { this.macroTable = macroTable; this.name = name; this.fieldName = fieldName; this.expression = expression; this.storeDoubleAsFloat = ColumnHolder.storeDoubleAsFloat(); - this.fieldExpression = Suppliers.memoize(() -> expression == null ? null : Parser.parse(expression, macroTable)); + this.fieldExpression = expressionSupplier; Preconditions.checkNotNull(name, "Must have a valid, non-null aggregator name"); Preconditions.checkArgument( fieldName == null ^ expression == null, 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 380ceb149419..5bc0cb12fc9b 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 @@ -58,12 +58,29 @@ public SimpleFloatAggregatorFactory( @Nullable final String fieldName, @Nullable String expression ) + { + this( + macroTable, + name, + fieldName, + expression, + Suppliers.memoize(() -> expression == null ? null : Parser.parse(expression, macroTable)) + ); + } + + public SimpleFloatAggregatorFactory( + ExprMacroTable macroTable, + String name, + String fieldName, + @Nullable String expression, + Supplier expressionSupplier + ) { this.macroTable = macroTable; this.name = name; this.fieldName = fieldName; this.expression = expression; - this.fieldExpression = Suppliers.memoize(() -> expression == null ? null : Parser.parse(expression, macroTable)); + this.fieldExpression = expressionSupplier; Preconditions.checkNotNull(name, "Must have a valid, non-null aggregator name"); Preconditions.checkArgument( fieldName == null ^ expression == null, 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 7d148d5b6f0b..16b92f6c4bc0 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 @@ -64,12 +64,29 @@ public SimpleLongAggregatorFactory( @Nullable final String fieldName, @Nullable String expression ) + { + this( + macroTable, + name, + fieldName, + expression, + Suppliers.memoize(() -> expression == null ? null : Parser.parse(expression, macroTable)) + ); + } + + public SimpleLongAggregatorFactory( + ExprMacroTable macroTable, + String name, + String fieldName, + @Nullable String expression, + Supplier expressionSupplier + ) { this.macroTable = macroTable; this.name = name; this.fieldName = fieldName; this.expression = expression; - this.fieldExpression = Suppliers.memoize(() -> expression == null ? null : Parser.parse(expression, macroTable)); + this.fieldExpression = expressionSupplier; Preconditions.checkNotNull(name, "Must have a valid, non-null aggregator name"); Preconditions.checkArgument( fieldName == null ^ expression == null, 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 978bf3e04abc..1a36a0212cc7 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 @@ -96,6 +96,26 @@ public ExpressionPostAggregator( ); } + public ExpressionPostAggregator( + final String name, + final String expression, + @Nullable final String ordering, + final ExprMacroTable macroTable, + final Supplier parsed + ) + { + this( + name, + expression, + ordering, + null, // in the future this will be computed by decorate + macroTable, + ImmutableMap.of(), + parsed, + Suppliers.memoize(() -> parsed.get().analyzeInputs().getRequiredBindings()) + ); + } + private ExpressionPostAggregator( final String name, final String expression, 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 2f65cc64fdad..01feea70caf3 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 @@ -56,6 +56,13 @@ public ExpressionDimFilter( this.parsed = Suppliers.memoize(() -> Parser.parse(expression, macroTable)); } + public ExpressionDimFilter(final String expression, final Supplier exprSupplier) + { + this.expression = expression; + this.filterTuning = null; + this.parsed = exprSupplier; + } + @VisibleForTesting public ExpressionDimFilter(final String expression, ExprMacroTable macroTable) { diff --git a/processing/src/main/java/org/apache/druid/segment/join/JoinConditionAnalysis.java b/processing/src/main/java/org/apache/druid/segment/join/JoinConditionAnalysis.java index 23875a0aceb0..4247311147f4 100644 --- a/processing/src/main/java/org/apache/druid/segment/join/JoinConditionAnalysis.java +++ b/processing/src/main/java/org/apache/druid/segment/join/JoinConditionAnalysis.java @@ -20,6 +20,8 @@ package org.apache.druid.segment.join; import com.google.common.base.Preconditions; +import com.google.common.base.Supplier; +import com.google.common.base.Suppliers; import org.apache.druid.java.util.common.Pair; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; @@ -82,21 +84,34 @@ private JoinConditionAnalysis( rightKeyColumns = getEquiConditions().stream().map(Equality::getRightColumn).collect(Collectors.toSet()); } + public static JoinConditionAnalysis forExpression( + final String condition, + final String rightPrefix, + final ExprMacroTable macroTable + ) + { + return forExpression( + condition, + rightPrefix, + Suppliers.memoize(() -> Parser.parse(condition, macroTable)) + ); + } + /** * Analyze a join condition. * - * @param condition the condition expression - * @param rightPrefix prefix for the right-hand side of the join; will be used to determine which identifiers in - * the condition come from the right-hand side and which come from the left-hand side - * @param macroTable macro table for parsing the condition expression + * @param condition the condition expression + * @param rightPrefix prefix for the right-hand side of the join; will be used to determine which identifiers in + * the condition come from the right-hand side and which come from the left-hand side + * @param exprSupplier {@link Expr} supplier for the parsed condition */ public static JoinConditionAnalysis forExpression( final String condition, final String rightPrefix, - final ExprMacroTable macroTable - ) + final Supplier exprSupplier + ) { - final Expr conditionExpr = Parser.parse(condition, macroTable); + final Expr conditionExpr = exprSupplier.get(); final List equiConditions = new ArrayList<>(); final List nonEquiConditions = new ArrayList<>(); 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 4a7635cad0f2..923a6cef5037 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 @@ -68,11 +68,21 @@ public ExpressionVirtualColumn( @JsonProperty("outputType") @Nullable ValueType outputType, @JacksonInject ExprMacroTable macroTable ) + { + this(name, expression, outputType, Suppliers.memoize(() -> Parser.parse(expression, macroTable))); + } + + public ExpressionVirtualColumn( + String name, + String expression, + @Nullable ValueType outputType, + Supplier exprSupplier + ) { this.name = Preconditions.checkNotNull(name, "name"); this.expression = Preconditions.checkNotNull(expression, "expression"); this.outputType = outputType; - this.parsedExpression = Suppliers.memoize(() -> Parser.parse(expression, macroTable)); + this.parsedExpression = exprSupplier; } /** diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/AvgSqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/AvgSqlAggregator.java index 3b9734436681..9812479c779c 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/AvgSqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/AvgSqlAggregator.java @@ -103,6 +103,7 @@ public Aggregation toDruidAggregation( final String sumName = Calcites.makePrefixedName(name, "sum"); final String countName = Calcites.makePrefixedName(name, "count"); final AggregatorFactory sum = SumSqlAggregator.createSumAggregatorFactory( + plannerContext, sumType, sumName, fieldName, diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MaxSqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MaxSqlAggregator.java index 302560c9bf47..f022192190da 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MaxSqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MaxSqlAggregator.java @@ -31,6 +31,7 @@ import org.apache.druid.segment.column.ValueType; import org.apache.druid.sql.calcite.aggregation.Aggregation; import org.apache.druid.sql.calcite.planner.Calcites; +import org.apache.druid.sql.calcite.planner.PlannerContext; public class MaxSqlAggregator extends SimpleSqlAggregator { @@ -42,6 +43,7 @@ public SqlAggFunction calciteFunction() @Override Aggregation getAggregation( + final PlannerContext plannerContext, final String name, final AggregateCall aggregateCall, final ExprMacroTable macroTable, @@ -50,10 +52,18 @@ Aggregation getAggregation( ) { final ValueType valueType = Calcites.getValueTypeForRelDataType(aggregateCall.getType()); - return Aggregation.create(createMaxAggregatorFactory(valueType, name, fieldName, expression, macroTable)); + return Aggregation.create(createMaxAggregatorFactory( + plannerContext, + valueType, + name, + fieldName, + expression, + macroTable + )); } private static AggregatorFactory createMaxAggregatorFactory( + final PlannerContext plannerContext, final ValueType aggregationType, final String name, final String fieldName, @@ -63,11 +73,29 @@ private static AggregatorFactory createMaxAggregatorFactory( { switch (aggregationType) { case LONG: - return new LongMaxAggregatorFactory(name, fieldName, expression, macroTable); + return new LongMaxAggregatorFactory( + name, + fieldName, + expression, + plannerContext.getCachingExprParser().lazyParse(expression), + macroTable + ); case FLOAT: - return new FloatMaxAggregatorFactory(name, fieldName, expression, macroTable); + return new FloatMaxAggregatorFactory( + name, + fieldName, + expression, + plannerContext.getCachingExprParser().lazyParse(expression), + macroTable + ); case DOUBLE: - return new DoubleMaxAggregatorFactory(name, fieldName, expression, macroTable); + return new DoubleMaxAggregatorFactory( + name, + fieldName, + expression, + plannerContext.getCachingExprParser().lazyParse(expression), + macroTable + ); default: throw new ISE("Cannot create aggregator factory for type[%s]", aggregationType); } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MinSqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MinSqlAggregator.java index 9e41ce5474b9..10492c32fbfa 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MinSqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MinSqlAggregator.java @@ -31,6 +31,7 @@ import org.apache.druid.segment.column.ValueType; import org.apache.druid.sql.calcite.aggregation.Aggregation; import org.apache.druid.sql.calcite.planner.Calcites; +import org.apache.druid.sql.calcite.planner.PlannerContext; public class MinSqlAggregator extends SimpleSqlAggregator { @@ -42,6 +43,7 @@ public SqlAggFunction calciteFunction() @Override Aggregation getAggregation( + final PlannerContext plannerContext, final String name, final AggregateCall aggregateCall, final ExprMacroTable macroTable, @@ -50,10 +52,18 @@ Aggregation getAggregation( ) { final ValueType valueType = Calcites.getValueTypeForRelDataType(aggregateCall.getType()); - return Aggregation.create(createMinAggregatorFactory(valueType, name, fieldName, expression, macroTable)); + return Aggregation.create(createMinAggregatorFactory( + plannerContext, + valueType, + name, + fieldName, + expression, + macroTable + )); } private static AggregatorFactory createMinAggregatorFactory( + final PlannerContext plannerContext, final ValueType aggregationType, final String name, final String fieldName, @@ -63,11 +73,29 @@ private static AggregatorFactory createMinAggregatorFactory( { switch (aggregationType) { case LONG: - return new LongMinAggregatorFactory(name, fieldName, expression, macroTable); + return new LongMinAggregatorFactory( + name, + fieldName, + expression, + plannerContext.getCachingExprParser().lazyParse(expression), + macroTable + ); case FLOAT: - return new FloatMinAggregatorFactory(name, fieldName, expression, macroTable); + return new FloatMinAggregatorFactory( + name, + fieldName, + expression, + plannerContext.getCachingExprParser().lazyParse(expression), + macroTable + ); case DOUBLE: - return new DoubleMinAggregatorFactory(name, fieldName, expression, macroTable); + return new DoubleMinAggregatorFactory( + name, + fieldName, + expression, + plannerContext.getCachingExprParser().lazyParse(expression), + macroTable + ); default: throw new ISE("Cannot create aggregator factory for type[%s]", aggregationType); } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SimpleSqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SimpleSqlAggregator.java index 0f7f937e3e0e..ba005e3b9f4a 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SimpleSqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SimpleSqlAggregator.java @@ -88,10 +88,11 @@ public Aggregation toDruidAggregation( expression = arg.getExpression(); } - return getAggregation(name, aggregateCall, macroTable, fieldName, expression); + return getAggregation(plannerContext, name, aggregateCall, macroTable, fieldName, expression); } abstract Aggregation getAggregation( + PlannerContext plannerContext, String name, AggregateCall aggregateCall, ExprMacroTable macroTable, diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumSqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumSqlAggregator.java index b9a5af57d003..c4afc3974279 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumSqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumSqlAggregator.java @@ -31,6 +31,7 @@ import org.apache.druid.segment.column.ValueType; import org.apache.druid.sql.calcite.aggregation.Aggregation; import org.apache.druid.sql.calcite.planner.Calcites; +import org.apache.druid.sql.calcite.planner.PlannerContext; public class SumSqlAggregator extends SimpleSqlAggregator { @@ -42,6 +43,7 @@ public SqlAggFunction calciteFunction() @Override Aggregation getAggregation( + final PlannerContext plannerContext, final String name, final AggregateCall aggregateCall, final ExprMacroTable macroTable, @@ -50,10 +52,18 @@ Aggregation getAggregation( ) { final ValueType valueType = Calcites.getValueTypeForRelDataType(aggregateCall.getType()); - return Aggregation.create(createSumAggregatorFactory(valueType, name, fieldName, expression, macroTable)); + return Aggregation.create(createSumAggregatorFactory( + plannerContext, + valueType, + name, + fieldName, + expression, + macroTable + )); } static AggregatorFactory createSumAggregatorFactory( + final PlannerContext plannerContext, final ValueType aggregationType, final String name, final String fieldName, @@ -63,11 +73,29 @@ static AggregatorFactory createSumAggregatorFactory( { switch (aggregationType) { case LONG: - return new LongSumAggregatorFactory(name, fieldName, expression, macroTable); + return new LongSumAggregatorFactory( + name, + fieldName, + expression, + plannerContext.getCachingExprParser().lazyParse(expression), + macroTable + ); case FLOAT: - return new FloatSumAggregatorFactory(name, fieldName, expression, macroTable); + return new FloatSumAggregatorFactory( + name, + fieldName, + expression, + plannerContext.getCachingExprParser().lazyParse(expression), + macroTable + ); case DOUBLE: - return new DoubleSumAggregatorFactory(name, fieldName, expression, macroTable); + return new DoubleSumAggregatorFactory( + name, + fieldName, + expression, + plannerContext.getCachingExprParser().lazyParse(expression), + macroTable + ); default: throw new ISE("Cannot create aggregator factory for type[%s]", aggregationType); } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/DruidExpression.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/DruidExpression.java index 6e0e80c66f30..4e408ba406db 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/DruidExpression.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/DruidExpression.java @@ -23,11 +23,9 @@ import com.google.common.io.BaseEncoding; import com.google.common.primitives.Chars; import org.apache.druid.java.util.common.StringUtils; -import org.apache.druid.math.expr.Expr; -import org.apache.druid.math.expr.ExprMacroTable; -import org.apache.druid.math.expr.Parser; import org.apache.druid.segment.column.ValueType; import org.apache.druid.segment.virtual.ExpressionVirtualColumn; +import org.apache.druid.sql.calcite.planner.PlannerContext; import java.util.Arrays; import java.util.List; @@ -153,23 +151,23 @@ public boolean isSimpleExtraction() return simpleExtraction != null; } - public Expr parse(final ExprMacroTable macroTable) - { - return Parser.parse(expression, macroTable); - } - public SimpleExtraction getSimpleExtraction() { return Preconditions.checkNotNull(simpleExtraction); } public ExpressionVirtualColumn toVirtualColumn( + final PlannerContext plannerContext, final String name, - final ValueType outputType, - final ExprMacroTable macroTable + final ValueType outputType ) { - return new ExpressionVirtualColumn(name, expression, outputType, macroTable); + return new ExpressionVirtualColumn( + name, + expression, + outputType, + plannerContext.getCachingExprParser().lazyParse(expression) + ); } public DruidExpression map( diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java index 3ddddd9957ff..f1a5fb02487c 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java @@ -36,8 +36,6 @@ import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.granularity.Granularity; import org.apache.druid.math.expr.Expr; -import org.apache.druid.math.expr.ExprMacroTable; -import org.apache.druid.math.expr.Parser; import org.apache.druid.query.aggregation.PostAggregator; import org.apache.druid.query.expression.TimestampFloorExprMacro; import org.apache.druid.query.extraction.ExtractionFn; @@ -541,7 +539,7 @@ private static DimFilter toSimpleLeafFilter( } // Special handling for filters on FLOOR(__time TO granularity). - final Granularity queryGranularity = toQueryGranularity(lhsExpression, plannerContext.getExprMacroTable()); + final Granularity queryGranularity = toQueryGranularity(plannerContext, lhsExpression); if (queryGranularity != null) { // lhs is FLOOR(__time TO granularity); rhs must be a timestamp final long rhsMillis = Calcites.calciteDateTimeLiteralToJoda(rhs, plannerContext.getTimeZone()).getMillis(); @@ -659,9 +657,14 @@ private static DimFilter toExpressionLeafFilter( ) { final DruidExpression druidExpression = toDruidExpression(plannerContext, rowSignature, rexNode); - return druidExpression != null - ? new ExpressionDimFilter(druidExpression.getExpression(), plannerContext.getExprMacroTable()) - : null; + if (druidExpression != null) { + return new ExpressionDimFilter( + druidExpression.getExpression(), + plannerContext.getCachingExprParser().lazyParse(druidExpression.getExpression()) + ); + } else { + return null; + } } /** @@ -671,9 +674,9 @@ private static DimFilter toExpressionLeafFilter( * @return granularity or null if not possible */ @Nullable - public static Granularity toQueryGranularity(final DruidExpression expression, final ExprMacroTable macroTable) + public static Granularity toQueryGranularity(final PlannerContext plannerContext, final DruidExpression expression) { - final TimestampFloorExprMacro.TimestampFloorExpr expr = asTimestampFloorExpr(expression, macroTable); + final TimestampFloorExprMacro.TimestampFloorExpr expr = asTimestampFloorExpr(plannerContext, expression); if (expr == null) { return null; @@ -691,11 +694,11 @@ public static Granularity toQueryGranularity(final DruidExpression expression, f @Nullable public static TimestampFloorExprMacro.TimestampFloorExpr asTimestampFloorExpr( - final DruidExpression expression, - final ExprMacroTable macroTable + final PlannerContext plannerContext, + final DruidExpression expression ) { - final Expr expr = Parser.parse(expression.getExpression(), macroTable); + final Expr expr = plannerContext.getCachingExprParser().parse(expression.getExpression()); if (expr instanceof TimestampFloorExprMacro.TimestampFloorExpr) { return (TimestampFloorExprMacro.TimestampFloorExpr) expr; diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayContainsOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayContainsOperatorConversion.java index a4a9c0de8152..63906f576344 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayContainsOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayContainsOperatorConversion.java @@ -27,7 +27,6 @@ import org.apache.calcite.sql.type.SqlTypeFamily; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprEval; -import org.apache.druid.math.expr.Parser; import org.apache.druid.query.filter.AndDimFilter; import org.apache.druid.query.filter.DimFilter; import org.apache.druid.segment.column.RowSignature; @@ -93,7 +92,7 @@ public DimFilter toDruidFilter( final DruidExpression rightExpr = druidExpressions.get(1); if (leftExpr.isSimpleExtraction()) { - Expr expr = Parser.parse(rightExpr.getExpression(), plannerContext.getExprMacroTable()); + Expr expr = plannerContext.getCachingExprParser().parse(rightExpr.getExpression()); // To convert this expression filter into an And of Selector filters, we need to extract all array elements. // For now, we can optimize only when rightExpr is a literal because there is no way to extract the array elements // by traversing the Expr. Note that all implementations of Expr are defined as package-private classes in a diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayOverlapOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayOverlapOperatorConversion.java index cb1ddf526a56..655728b9f7c2 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayOverlapOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayOverlapOperatorConversion.java @@ -28,7 +28,6 @@ import org.apache.calcite.sql.type.SqlTypeFamily; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprEval; -import org.apache.druid.math.expr.Parser; import org.apache.druid.query.filter.DimFilter; import org.apache.druid.query.filter.InDimFilter; import org.apache.druid.segment.column.RowSignature; @@ -105,7 +104,7 @@ public DimFilter toDruidFilter( return toExpressionFilter(plannerContext, getDruidFunctionName(), druidExpressions); } - Expr expr = Parser.parse(complexExpr.getExpression(), plannerContext.getExprMacroTable()); + Expr expr = plannerContext.getCachingExprParser().parse(complexExpr.getExpression()); if (expr.isLiteral()) { // Evaluate the expression to take out the array elements. // We can safely pass null if the expression is literal. diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/BaseExpressionDimFilterOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/BaseExpressionDimFilterOperatorConversion.java index a1f6dc36bbbf..bdbb60c2cff5 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/BaseExpressionDimFilterOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/BaseExpressionDimFilterOperatorConversion.java @@ -50,7 +50,7 @@ protected static DimFilter toExpressionFilter( return new ExpressionDimFilter( filterExpr, - plannerContext.getExprMacroTable() + plannerContext.getCachingExprParser().lazyParse(filterExpr) ); } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/CastOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/CastOperatorConversion.java index 51a42b41140a..852309ffd126 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/CastOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/CastOperatorConversion.java @@ -133,9 +133,9 @@ public DruidExpression toDruidExpression( if (toType == SqlTypeName.DATE) { // Floor to day when casting to DATE. return TimeFloorOperatorConversion.applyTimestampFloor( + plannerContext, typeCastExpression, - new PeriodGranularity(Period.days(1), null, plannerContext.getTimeZone()), - plannerContext.getExprMacroTable() + new PeriodGranularity(Period.days(1), null, plannerContext.getTimeZone()) ); } else { return typeCastExpression; @@ -161,9 +161,9 @@ private static DruidExpression castCharToDateTime( if (toType == SqlTypeName.DATE) { return TimeFloorOperatorConversion.applyTimestampFloor( + plannerContext, timestampExpression, - new PeriodGranularity(Period.days(1), null, plannerContext.getTimeZone()), - plannerContext.getExprMacroTable() + new PeriodGranularity(Period.days(1), null, plannerContext.getTimeZone()) ); } else if (toType == SqlTypeName.TIMESTAMP) { return timestampExpression; diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/DateTruncOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/DateTruncOperatorConversion.java index f496e0ab9671..039c8d2afc8d 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/DateTruncOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/DateTruncOperatorConversion.java @@ -30,7 +30,6 @@ import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.Expr; -import org.apache.druid.math.expr.Parser; import org.apache.druid.segment.column.RowSignature; import org.apache.druid.sql.calcite.expression.DruidExpression; import org.apache.druid.sql.calcite.expression.OperatorConversions; @@ -90,9 +89,8 @@ public DruidExpression toDruidExpression( rexNode, inputExpressions -> { final DruidExpression arg = inputExpressions.get(1); - final Expr truncTypeExpr = Parser.parse( - inputExpressions.get(0).getExpression(), - plannerContext.getExprMacroTable() + final Expr truncTypeExpr = plannerContext.getCachingExprParser().parse( + inputExpressions.get(0).getExpression() ); if (!truncTypeExpr.isLiteral()) { diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/QueryLookupOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/QueryLookupOperatorConversion.java index 5c1665c7083c..cdbabe13820d 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/QueryLookupOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/QueryLookupOperatorConversion.java @@ -72,7 +72,9 @@ public DruidExpression toDruidExpression( StringUtils.toLowerCase(calciteOperator().getName()), inputExpressions -> { final DruidExpression arg = inputExpressions.get(0); - final Expr lookupNameExpr = inputExpressions.get(1).parse(plannerContext.getExprMacroTable()); + final Expr lookupNameExpr = plannerContext.getCachingExprParser().parse( + inputExpressions.get(1).getExpression() + ); if (arg.isSimpleExtraction() && lookupNameExpr.isLiteral()) { return arg.getSimpleExtraction().cascade( diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RegexpExtractOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RegexpExtractOperatorConversion.java index b6469718255e..5e92b1710187 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RegexpExtractOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RegexpExtractOperatorConversion.java @@ -66,9 +66,9 @@ public DruidExpression toDruidExpression( StringUtils.toLowerCase(calciteOperator().getName()), inputExpressions -> { final DruidExpression arg = inputExpressions.get(0); - final Expr patternExpr = inputExpressions.get(1).parse(plannerContext.getExprMacroTable()); + final Expr patternExpr = plannerContext.getCachingExprParser().parse(inputExpressions.get(1).getExpression()); final Expr indexExpr = inputExpressions.size() > 2 - ? inputExpressions.get(2).parse(plannerContext.getExprMacroTable()) + ? plannerContext.getCachingExprParser().parse(inputExpressions.get(2).getExpression()) : null; if (arg.isSimpleExtraction() && patternExpr.isLiteral() && (indexExpr == null || indexExpr.isLiteral())) { diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TimeFloorOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TimeFloorOperatorConversion.java index 87c07f25b7e5..710827afdc33 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TimeFloorOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TimeFloorOperatorConversion.java @@ -32,7 +32,6 @@ import org.apache.calcite.sql.type.SqlTypeFamily; import org.apache.calcite.sql.type.SqlTypeName; import org.apache.druid.java.util.common.granularity.PeriodGranularity; -import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.query.expression.TimestampFloorExprMacro; import org.apache.druid.segment.column.RowSignature; import org.apache.druid.sql.calcite.expression.DruidExpression; @@ -66,9 +65,9 @@ public class TimeFloorOperatorConversion implements SqlOperatorConversion * it's responsible for generating "timestamp_floor" calls. */ public static DruidExpression applyTimestampFloor( + final PlannerContext plannerContext, final DruidExpression input, - final PeriodGranularity granularity, - final ExprMacroTable macroTable + final PeriodGranularity granularity ) { Preconditions.checkNotNull(input, "input"); @@ -77,8 +76,8 @@ public static DruidExpression applyTimestampFloor( // Collapse floor chains if possible. Useful for constructs like CAST(FLOOR(__time TO QUARTER) AS DATE). if (granularity.getPeriod().equals(Period.days(1))) { final TimestampFloorExprMacro.TimestampFloorExpr floorExpr = Expressions.asTimestampFloorExpr( - input, - macroTable + plannerContext, + input ); if (floorExpr != null) { diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TruncateOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TruncateOperatorConversion.java index b4eae6661574..0bf6df7cd823 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TruncateOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TruncateOperatorConversion.java @@ -62,7 +62,7 @@ public DruidExpression toDruidExpression( inputExpressions -> { final DruidExpression arg = inputExpressions.get(0); final Expr digitsExpr = inputExpressions.size() > 1 - ? inputExpressions.get(1).parse(plannerContext.getExprMacroTable()) + ? plannerContext.getCachingExprParser().parse(inputExpressions.get(1).getExpression()) : null; final String factorString; diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CachingExprParser.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CachingExprParser.java new file mode 100644 index 000000000000..c219c1181b5a --- /dev/null +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CachingExprParser.java @@ -0,0 +1,79 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.sql.calcite.planner; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Supplier; +import org.apache.druid.math.expr.Expr; +import org.apache.druid.math.expr.ExprMacroTable; +import org.apache.druid.math.expr.Parser; + +import javax.annotation.Nullable; +import java.util.HashMap; +import java.util.Map; + +/** + * An expression parser that caches expressions in memory. + * This parser is created per SQL query and used by {@link DruidPlanner}. + */ +public class CachingExprParser +{ + private final ExprMacroTable macroTable; + private final boolean enabled; + private final Map exprCache = new HashMap<>(); + + public CachingExprParser(ExprMacroTable macroTable, PlannerConfig plannerConfig) + { + this.macroTable = macroTable; + this.enabled = plannerConfig.isUseParsedExprCache(); + } + + /** + * Parses the given expression and caches the result if caching is enabled. + * Returns null only when the given expression is null. + */ + @Nullable + public Expr parse(@Nullable String expression) + { + if (expression == null) { + return null; + } + if (enabled) { + return exprCache.computeIfAbsent(expression, k -> Parser.parse(k, macroTable)); + } else { + return Parser.parse(expression, macroTable); + } + } + + /** + * Returns an {@link Expr} supplier that lazily parses the given expression. + * The supplier can return null if the given expression is null. + */ + public Supplier lazyParse(@Nullable String expression) + { + return () -> parse(expression); + } + + @VisibleForTesting + public Map getExprCache() + { + return exprCache; + } +} diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModule.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModule.java index 6d90eb5076a6..db85317cc2f7 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModule.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModule.java @@ -19,6 +19,7 @@ package org.apache.druid.sql.calcite.planner; +import com.google.common.annotations.VisibleForTesting; import com.google.inject.Binder; import com.google.inject.Module; import org.apache.druid.guice.JsonConfigProvider; @@ -28,10 +29,13 @@ */ public class CalcitePlannerModule implements Module { + @VisibleForTesting + static final String PLANNER_CONFIG_PREFIX = "druid.sql.planner"; + @Override public void configure(Binder binder) { - JsonConfigProvider.bind(binder, "druid.sql.planner", PlannerConfig.class); + JsonConfigProvider.bind(binder, PLANNER_CONFIG_PREFIX, PlannerConfig.class); binder.bind(PlannerFactory.class); binder.bind(DruidOperatorTable.class); } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java index c181cb8c885a..28e03d127077 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java @@ -29,7 +29,6 @@ import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprEval; import org.apache.druid.math.expr.ExprType; -import org.apache.druid.math.expr.Parser; import org.apache.druid.segment.column.RowSignature; import org.apache.druid.sql.calcite.expression.DruidExpression; import org.apache.druid.sql.calcite.expression.Expressions; @@ -71,7 +70,7 @@ public void reduce( reducedValues.add(constExp); } else { final SqlTypeName sqlTypeName = constExp.getType().getSqlTypeName(); - final Expr expr = Parser.parse(druidExpression.getExpression(), plannerContext.getExprMacroTable()); + final Expr expr = plannerContext.getCachingExprParser().parse(druidExpression.getExpression()); final ExprEval exprResult = expr.eval( name -> { diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerConfig.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerConfig.java index 34ef9afe3652..b96bdc3f69c0 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerConfig.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerConfig.java @@ -31,6 +31,7 @@ public class PlannerConfig { public static final String CTX_KEY_USE_APPROXIMATE_COUNT_DISTINCT = "useApproximateCountDistinct"; public static final String CTX_KEY_USE_APPROXIMATE_TOPN = "useApproximateTopN"; + public static final String CTX_KEY_USE_PARSED_EXPR_CACHE = "useParsedExprCache"; @JsonProperty private Period metadataRefreshPeriod = new Period("PT1M"); @@ -59,6 +60,9 @@ public class PlannerConfig @JsonProperty private long metadataSegmentPollPeriod = 60000; + @JsonProperty + private boolean useParsedExprCache = false; + public long getMetadataSegmentPollPeriod() { return metadataSegmentPollPeriod; @@ -111,6 +115,11 @@ public boolean shouldSerializeComplexValues() return serializeComplexValues; } + public boolean isUseParsedExprCache() + { + return useParsedExprCache; + } + public PlannerConfig withOverrides(final Map context) { if (context == null) { @@ -136,6 +145,11 @@ public PlannerConfig withOverrides(final Map context) newConfig.metadataSegmentCacheEnable = isMetadataSegmentCacheEnable(); newConfig.metadataSegmentPollPeriod = getMetadataSegmentPollPeriod(); newConfig.serializeComplexValues = shouldSerializeComplexValues(); + newConfig.useParsedExprCache = getContextBoolean( + context, + CTX_KEY_USE_PARSED_EXPR_CACHE, + isUseParsedExprCache() + ); return newConfig; } @@ -174,6 +188,7 @@ public boolean equals(final Object o) awaitInitializationOnStart == that.awaitInitializationOnStart && metadataSegmentCacheEnable == that.metadataSegmentCacheEnable && metadataSegmentPollPeriod == that.metadataSegmentPollPeriod && + useParsedExprCache == that.useParsedExprCache && serializeComplexValues == that.serializeComplexValues && Objects.equals(metadataRefreshPeriod, that.metadataRefreshPeriod) && Objects.equals(sqlTimeZone, that.sqlTimeZone); @@ -182,7 +197,6 @@ public boolean equals(final Object o) @Override public int hashCode() { - return Objects.hash( metadataRefreshPeriod, maxTopNLimit, @@ -193,6 +207,7 @@ public int hashCode() sqlTimeZone, metadataSegmentCacheEnable, metadataSegmentPollPeriod, + useParsedExprCache, serializeComplexValues ); } @@ -207,9 +222,10 @@ public String toString() ", useApproximateTopN=" + useApproximateTopN + ", requireTimeCondition=" + requireTimeCondition + ", awaitInitializationOnStart=" + awaitInitializationOnStart + + ", sqlTimeZone=" + sqlTimeZone + ", metadataSegmentCacheEnable=" + metadataSegmentCacheEnable + ", metadataSegmentPollPeriod=" + metadataSegmentPollPeriod + - ", sqlTimeZone=" + sqlTimeZone + + ", useParsedExprCache=" + useParsedExprCache + ", serializeComplexValues=" + serializeComplexValues + '}'; } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java index 344023e58642..04a9f119e35e 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java @@ -68,6 +68,7 @@ public class PlannerContext private final DateTime localNow; private final Map queryContext; private final String sqlQueryId; + private final CachingExprParser cachingExprParser; private final List nativeQueryIds = new CopyOnWriteArrayList<>(); // bindings for dynamic parameters to bind during planning private List parameters = Collections.emptyList(); @@ -98,6 +99,7 @@ private PlannerContext( sqlQueryId = UUID.randomUUID().toString(); } this.sqlQueryId = sqlQueryId; + this.cachingExprParser = new CachingExprParser(macroTable, plannerConfig); } public static PlannerContext create( @@ -282,4 +284,9 @@ public void setResources(Set resources) { this.resources = Preconditions.checkNotNull(resources, "resources"); } + + public CachingExprParser getCachingExprParser() + { + return cachingExprParser; + } } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidJoinQueryRel.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidJoinQueryRel.java index 1b28a9830c8b..6ed654137128 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidJoinQueryRel.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidJoinQueryRel.java @@ -192,7 +192,7 @@ public DruidQuery toDruidQuery(final boolean finalizeAggregations) condition.getExpression(), toDruidJoinType(joinRel.getJoinType()), getDimFilter(getPlannerContext(), leftSignature, leftFilter), - getPlannerContext().getExprMacroTable() + getPlannerContext().getCachingExprParser().lazyParse(condition.getExpression()) ), prefixSignaturePair.rhs, getPlannerContext(), diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java index a12ba3444ca5..738f3987a698 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java @@ -792,8 +792,8 @@ public TimeseriesQuery toTimeseriesQuery() } else if (grouping.getDimensions().size() == 1) { final DimensionExpression dimensionExpression = Iterables.getOnlyElement(grouping.getDimensions()); queryGranularity = Expressions.toQueryGranularity( - dimensionExpression.getDruidExpression(), - plannerContext.getExprMacroTable() + plannerContext, + dimensionExpression.getDruidExpression() ); if (queryGranularity == null) { diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/Grouping.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/Grouping.java index 0a954729f703..5ed1d6bfb32f 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/Grouping.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/Grouping.java @@ -27,7 +27,7 @@ import org.apache.calcite.rel.core.Project; import org.apache.calcite.util.ImmutableBitSet; import org.apache.druid.java.util.common.ISE; -import org.apache.druid.math.expr.Parser; +import org.apache.druid.math.expr.Expr; import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.PostAggregator; import org.apache.druid.query.dimension.DimensionSpec; @@ -190,8 +190,8 @@ public Grouping applyProject(final PlannerContext plannerContext, final Project for (int i = 0; i < dimensions.size(); i++) { final DimensionExpression dimension = dimensions.get(i); - if (Parser.parse(dimension.getDruidExpression().getExpression(), plannerContext.getExprMacroTable()) - .isLiteral() && !aggregateProjectBits.get(i)) { + final Expr parsed = plannerContext.getCachingExprParser().parse(dimension.getDruidExpression().getExpression()); + if (parsed.isLiteral() && !aggregateProjectBits.get(i)) { newDimIndexes[i] = -1; } else { newDimIndexes[i] = newDimensions.size(); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/Projection.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/Projection.java index 631b338f6816..a4b711006b95 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/Projection.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/Projection.java @@ -184,7 +184,8 @@ private static void handlePostAggregatorExpression( postAggregatorVisitor.getOutputNamePrefix() + postAggregatorVisitor.getAndIncrementCounter(), postAggregatorExpression.getExpression(), null, - plannerContext.getExprMacroTable() + plannerContext.getExprMacroTable(), + plannerContext.getCachingExprParser().lazyParse(postAggregatorExpression.getExpression()) ); postAggregatorVisitor.addPostAgg(postAggregator); rowOrder.add(postAggregator.getName()); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/VirtualColumnRegistry.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/VirtualColumnRegistry.java index 7244c750574a..cf690a90e5ad 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/VirtualColumnRegistry.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/VirtualColumnRegistry.java @@ -86,9 +86,9 @@ public VirtualColumn getOrCreateVirtualColumnForExpression( if (!virtualColumnsByExpression.containsKey(expression.getExpression())) { final String virtualColumnName = virtualColumnPrefix + virtualColumnCounter++; final VirtualColumn virtualColumn = expression.toVirtualColumn( + plannerContext, virtualColumnName, - valueType, - plannerContext.getExprMacroTable() + valueType ); virtualColumnsByExpression.put( expression.getExpression(), diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/SqlPlannerExprCacheTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/SqlPlannerExprCacheTest.java new file mode 100644 index 000000000000..e85fb4afca9a --- /dev/null +++ b/sql/src/test/java/org/apache/druid/sql/calcite/SqlPlannerExprCacheTest.java @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.sql.calcite; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import org.apache.calcite.tools.RelConversionException; +import org.apache.druid.sql.SqlLifecycle; +import org.apache.druid.sql.calcite.planner.PlannerConfig; +import org.apache.druid.sql.calcite.planner.PlannerContext; +import org.apache.druid.sql.calcite.util.CalciteTests; +import org.junit.Assert; +import org.junit.Test; + +import java.util.Map; + +public class SqlPlannerExprCacheTest extends BaseCalciteQueryTest +{ + private static final PlannerConfig CACHE_DISABLING_CONFIG = new PlannerConfig(); + private static final PlannerConfig CACHE_ENABLING_CONFIG = new PlannerConfig().withOverrides( + ImmutableMap.of(PlannerConfig.CTX_KEY_USE_PARSED_EXPR_CACHE, true) + ); + private static final String DEFAULT_SQL = + "SELECT CAST(__time AS BIGINT), m1, ANY_VALUE(dim3, 100) FROM foo\n" + + "WHERE (CAST(TIME_FLOOR(__time, 'PT1H') AS BIGINT), m1) IN\n" + + " (\n" + + " SELECT CAST(TIME_FLOOR(__time, 'PT1H') AS BIGINT) + 0 AS t1, MIN(m1) AS t2\n" + + " FROM foo\n" + + " WHERE dim3 = 'b' AND __time BETWEEN '1994-04-29 00:00:00' AND '2020-01-11 00:00:00'\n" + + " GROUP BY 1\n" + + " )\n" + + "GROUP BY 1, 2\n"; + + @Test + public void testEnableCacheViaConfig() throws RelConversionException + { + final PlannerContext context = plan(CACHE_ENABLING_CONFIG, DEFAULT_SQL, ImmutableMap.of()); + Assert.assertEquals( + ImmutableSet.of( + "100", + "\"dim3\"", + "timestamp_parse('2020-01-11 00:00:00',null,'UTC')", + "timestamp_floor(\"__time\",'PT1H',null,'UTC')", + "(timestamp_floor(\"__time\",'PT1H',null,'UTC') + 0)", + "\"__time\"", + "timestamp_parse('1994-04-29 00:00:00',null,'UTC')" + ), + context.getCachingExprParser().getExprCache().keySet() + ); + } + + @Test + public void testEnableCacheViaQueryContext() throws RelConversionException + { + final PlannerContext context = plan( + CACHE_DISABLING_CONFIG, + DEFAULT_SQL, + ImmutableMap.of(PlannerConfig.CTX_KEY_USE_PARSED_EXPR_CACHE, true) + ); + Assert.assertEquals( + ImmutableSet.of( + "100", + "\"dim3\"", + "timestamp_parse('2020-01-11 00:00:00',null,'UTC')", + "timestamp_floor(\"__time\",'PT1H',null,'UTC')", + "(timestamp_floor(\"__time\",'PT1H',null,'UTC') + 0)", + "\"__time\"", + "timestamp_parse('1994-04-29 00:00:00',null,'UTC')" + ), + context.getCachingExprParser().getExprCache().keySet() + ); + } + + @Test + public void testDisableCacheViaConfig() throws RelConversionException + { + final PlannerContext context = plan(CACHE_DISABLING_CONFIG, DEFAULT_SQL, ImmutableMap.of()); + Assert.assertTrue(context.getCachingExprParser().getExprCache().isEmpty()); + } + + @Test + public void testDisableCacheViaQueryContext() throws RelConversionException + { + final PlannerContext context = plan( + CACHE_ENABLING_CONFIG, + DEFAULT_SQL, + ImmutableMap.of(PlannerConfig.CTX_KEY_USE_PARSED_EXPR_CACHE, false) + ); + Assert.assertTrue(context.getCachingExprParser().getExprCache().isEmpty()); + } + + private PlannerContext plan(PlannerConfig plannerConfig, String sql, Map queryContext) + throws RelConversionException + { + final SqlLifecycle lifecycle = getSqlLifecycleFactory( + plannerConfig, + CalciteTests.createOperatorTable(), + CalciteTests.createExprMacroTable(), + CalciteTests.TEST_AUTHORIZER_MAPPER, + CalciteTests.getJsonMapper() + ).factorize(); + lifecycle.initialize(sql, queryContext); + lifecycle.validateAndAuthorize(CalciteTests.REGULAR_USER_AUTH_RESULT); + return lifecycle.plan(); + } +} diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/planner/CachingExprParserTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/planner/CachingExprParserTest.java new file mode 100644 index 000000000000..fa158c203e5e --- /dev/null +++ b/sql/src/test/java/org/apache/druid/sql/calcite/planner/CachingExprParserTest.java @@ -0,0 +1,105 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.sql.calcite.planner; + +import com.google.common.base.Supplier; +import com.google.common.collect.ImmutableMap; +import org.apache.druid.math.expr.Expr; +import org.apache.druid.math.expr.ExprMacroTable; +import org.apache.druid.testing.InitializedNullHandlingTest; +import org.junit.Assert; +import org.junit.Test; + +public class CachingExprParserTest extends InitializedNullHandlingTest +{ + private static final ExprMacroTable MACRO_TABLE = ExprMacroTable.nil(); + private static final PlannerConfig CACHE_DISABLING_CONFIG = new PlannerConfig(); + private static final PlannerConfig CACHE_ENABLING_CONFIG = new PlannerConfig().withOverrides( + ImmutableMap.of(PlannerConfig.CTX_KEY_USE_PARSED_EXPR_CACHE, true) + ); + + @Test + public void testParseNullReturnNull() + { + CachingExprParser parser = new CachingExprParser(MACRO_TABLE, CACHE_DISABLING_CONFIG); + Assert.assertNull(parser.parse(null)); + Assert.assertTrue(parser.getExprCache().isEmpty()); + + parser = new CachingExprParser(MACRO_TABLE, CACHE_ENABLING_CONFIG); + Assert.assertNull(parser.parse(null)); + Assert.assertTrue(parser.getExprCache().isEmpty()); + } + + @Test + public void testLazyParseNullReturnSupplierOfNull() + { + CachingExprParser parser = new CachingExprParser(MACRO_TABLE, CACHE_DISABLING_CONFIG); + Supplier exprSupplier = parser.lazyParse(null); + Assert.assertNull(exprSupplier.get()); + Assert.assertTrue(parser.getExprCache().isEmpty()); + + parser = new CachingExprParser(MACRO_TABLE, CACHE_ENABLING_CONFIG); + exprSupplier = parser.lazyParse(null); + Assert.assertNull(exprSupplier.get()); + Assert.assertTrue(parser.getExprCache().isEmpty()); + } + + @Test + public void testParseWithCacheEnabled() + { + final CachingExprParser parser = new CachingExprParser(MACRO_TABLE, CACHE_ENABLING_CONFIG); + final String strExpr = "x + y"; + final Expr expr = parser.parse(strExpr); + Assert.assertNotNull(expr); + Assert.assertSame(expr, parser.parse(strExpr)); + Assert.assertSame(expr, parser.getExprCache().get(strExpr)); + } + + @Test + public void testParseWithCacheDisabled() + { + final CachingExprParser parser = new CachingExprParser(MACRO_TABLE, CACHE_DISABLING_CONFIG); + final String strExpr = "x + y"; + final Expr expr = parser.parse(strExpr); + Assert.assertNotNull(expr); + Assert.assertNotSame(expr, parser.parse(strExpr)); + Assert.assertTrue(parser.getExprCache().isEmpty()); + } + + @Test + public void testLazyParseWithCacheEnabled() + { + final CachingExprParser parser = new CachingExprParser(MACRO_TABLE, CACHE_ENABLING_CONFIG); + final String strExpr = "x + y"; + final Supplier exprSupplier = parser.lazyParse(strExpr); + Assert.assertSame(exprSupplier.get(), parser.parse(strExpr)); + Assert.assertSame(exprSupplier.get(), parser.getExprCache().get(strExpr)); + } + + @Test + public void testLazyParseWithCacheDisabled() + { + final CachingExprParser parser = new CachingExprParser(MACRO_TABLE, CACHE_DISABLING_CONFIG); + final String strExpr = "x + y"; + final Supplier exprSupplier = parser.lazyParse(strExpr); + Assert.assertNotSame(exprSupplier.get(), parser.parse(strExpr)); + Assert.assertTrue(parser.getExprCache().isEmpty()); + } +} diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModuleTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModuleTest.java index e081e1fd0617..d4660c174912 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModuleTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModuleTest.java @@ -29,6 +29,7 @@ import org.apache.calcite.schema.SchemaPlus; import org.apache.druid.guice.LazySingleton; import org.apache.druid.jackson.JacksonModule; +import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.server.QueryLifecycleFactory; import org.apache.druid.server.security.AuthorizerMapper; @@ -40,6 +41,8 @@ import org.easymock.EasyMock; import org.easymock.EasyMockRunner; import org.easymock.Mock; +import org.joda.time.DateTimeZone; +import org.joda.time.Period; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -47,6 +50,7 @@ import javax.validation.Validation; import javax.validation.Validator; +import java.util.Properties; import java.util.Set; @RunWith(EasyMockRunner.class) @@ -78,6 +82,7 @@ public class CalcitePlannerModuleTest extends CalciteTestBase private Set calciteSchemas; private CalcitePlannerModule target; + private Properties properties; private Injector injector; @Before @@ -92,6 +97,7 @@ public void setUp() aggregators = ImmutableSet.of(); operatorConversions = ImmutableSet.of(); target = new CalcitePlannerModule(); + properties = new Properties(); injector = Guice.createInjector( new JacksonModule(), binder -> { @@ -105,6 +111,7 @@ public void setUp() binder.bind(Key.get(new TypeLiteral>(){})).toInstance(aggregators); binder.bind(Key.get(new TypeLiteral>(){})).toInstance(operatorConversions); binder.bind(SchemaPlus.class).toInstance(rootSchema); + binder.bind(Properties.class).toInstance(properties); }, target ); @@ -129,9 +136,51 @@ public void testPlannerFactoryIsInjectable() } @Test - public void testPlannerConfigIsInjected() + public void testDefaultPlannerConfigIsInjected() { PlannerConfig plannerConfig = injector.getInstance(PlannerConfig.class); Assert.assertNotNull(plannerConfig); + Assert.assertEquals(new Period("PT1M"), plannerConfig.getMetadataRefreshPeriod()); + Assert.assertEquals(100000, plannerConfig.getMaxTopNLimit()); + Assert.assertTrue(plannerConfig.isUseApproximateCountDistinct()); + Assert.assertTrue(plannerConfig.isUseApproximateTopN()); + Assert.assertFalse(plannerConfig.isRequireTimeCondition()); + Assert.assertTrue(plannerConfig.isAwaitInitializationOnStart()); + Assert.assertEquals(DateTimeZone.UTC, plannerConfig.getSqlTimeZone()); + Assert.assertFalse(plannerConfig.isMetadataSegmentCacheEnable()); + Assert.assertEquals(60000, plannerConfig.getMetadataSegmentPollPeriod()); + Assert.assertFalse(plannerConfig.isUseParsedExprCache()); + } + + @Test + public void testInjectPlannerConfigWithCustomProperties() + { + properties.setProperty(getPlannerConfigKey("metadataRefreshPeriod"), "PT10M"); + properties.setProperty(getPlannerConfigKey("maxTopNLimit"), "10"); + properties.setProperty(getPlannerConfigKey("useApproximateCountDistinct"), "false"); + properties.setProperty(getPlannerConfigKey("useApproximateTopN"), "false"); + properties.setProperty(getPlannerConfigKey("requireTimeCondition"), "true"); + properties.setProperty(getPlannerConfigKey("awaitInitializationOnStart"), "false"); + properties.setProperty(getPlannerConfigKey("sqlTimeZone"), DateTimeZone.forID("Asia/Seoul").toString()); + properties.setProperty(getPlannerConfigKey("metadataSegmentCacheEnable"), "true"); + properties.setProperty(getPlannerConfigKey("metadataSegmentPollPeriod"), "20"); + properties.setProperty(getPlannerConfigKey("useParsedExprCache"), "true"); + PlannerConfig plannerConfig = injector.getInstance(PlannerConfig.class); + Assert.assertNotNull(plannerConfig); + Assert.assertEquals(new Period("PT10M"), plannerConfig.getMetadataRefreshPeriod()); + Assert.assertEquals(10, plannerConfig.getMaxTopNLimit()); + Assert.assertFalse(plannerConfig.isUseApproximateCountDistinct()); + Assert.assertFalse(plannerConfig.isUseApproximateTopN()); + Assert.assertTrue(plannerConfig.isRequireTimeCondition()); + Assert.assertFalse(plannerConfig.isAwaitInitializationOnStart()); + Assert.assertEquals(DateTimeZone.forID("Asia/Seoul"), plannerConfig.getSqlTimeZone()); + Assert.assertTrue(plannerConfig.isMetadataSegmentCacheEnable()); + Assert.assertEquals(20, plannerConfig.getMetadataSegmentPollPeriod()); + Assert.assertTrue(plannerConfig.isUseParsedExprCache()); + } + + private static String getPlannerConfigKey(String suffix) + { + return StringUtils.format("%s.%s", CalcitePlannerModule.PLANNER_CONFIG_PREFIX, suffix); } } From c9caaa0c81b2257239b439ca62574c3e6a1cd58b Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Fri, 12 Mar 2021 18:59:41 -0800 Subject: [PATCH 3/6] docs --- docs/configuration/index.md | 1 + docs/querying/sql.md | 1 + 2 files changed, 2 insertions(+) diff --git a/docs/configuration/index.md b/docs/configuration/index.md index e6650b1cd38a..9b768ec8b7ed 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -1680,6 +1680,7 @@ The Druid SQL server is configured through the following properties on the Broke |`druid.sql.planner.sqlTimeZone`|Sets the default time zone for the server, which will affect how time functions and timestamp literals behave. Should be a time zone name like "America/Los_Angeles" or offset like "-08:00".|UTC| |`druid.sql.planner.metadataSegmentCacheEnable`|Whether to keep a cache of published segments in broker. If true, broker polls coordinator in background to get segments from metadata store and maintains a local cache. If false, coordinator's REST API will be invoked when broker needs published segments info.|false| |`druid.sql.planner.metadataSegmentPollPeriod`|How often to poll coordinator for published segments list if `druid.sql.planner.metadataSegmentCacheEnable` is set to true. Poll period is in milliseconds. |60000| +|`druid.sql.planner.useParsedExprCache`|Whether to use a cache for parsed expressions. This cache is created per query and stored on heap memory. Enabling cache can be useful when planning time takes long.|false| > Previous versions of Druid had properties named `druid.sql.planner.maxQueryCount` and `druid.sql.planner.maxSemiJoinRowsInMemory`. > These properties are no longer available. Since Druid 0.18.0, you can use `druid.server.http.maxSubqueryRows` to control the maximum diff --git a/docs/querying/sql.md b/docs/querying/sql.md index 38625cb190a8..6d8aaebea1dd 100644 --- a/docs/querying/sql.md +++ b/docs/querying/sql.md @@ -1008,6 +1008,7 @@ Connection context can be specified as JDBC connection properties or as a "conte |`sqlTimeZone`|Sets the time zone for this connection, which will affect how time functions and timestamp literals behave. Should be a time zone name like "America/Los_Angeles" or offset like "-08:00".|druid.sql.planner.sqlTimeZone on the Broker (default: UTC)| |`useApproximateCountDistinct`|Whether to use an approximate cardinality algorithm for `COUNT(DISTINCT foo)`.|druid.sql.planner.useApproximateCountDistinct on the Broker (default: true)| |`useApproximateTopN`|Whether to use approximate [TopN queries](topnquery.md) when a SQL query could be expressed as such. If false, exact [GroupBy queries](groupbyquery.md) will be used instead.|druid.sql.planner.useApproximateTopN on the Broker (default: true)| +|`useParsedExprCache`|Whether to use a cache for parsed expressions. This cache is created per query and stored on heap memory. Enabling cache can be useful when planning time takes long.|druid.sql.planner.useParsedExprCache on the Broker (default: false)| ## Metadata tables From 7652a7ffc8de24388bd9d8382c33aafcd654b919 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sat, 13 Mar 2021 09:56:10 -0800 Subject: [PATCH 4/6] fix ci --- docs/querying/sql.md | 2 +- .../post/ExpressionPostAggregator.java | 20 ------------------- .../query/filter/ExpressionDimFilter.java | 12 ++++++----- .../segment/join/JoinConditionAnalysis.java | 2 +- .../sql/calcite/expression/Expressions.java | 1 + ...ExpressionDimFilterOperatorConversion.java | 1 + .../druid/sql/calcite/rel/Projection.java | 2 ++ .../planner/CalcitePlannerModuleTest.java | 5 +++-- 8 files changed, 16 insertions(+), 29 deletions(-) diff --git a/docs/querying/sql.md b/docs/querying/sql.md index 6d8aaebea1dd..9d982b0292be 100644 --- a/docs/querying/sql.md +++ b/docs/querying/sql.md @@ -1008,7 +1008,7 @@ Connection context can be specified as JDBC connection properties or as a "conte |`sqlTimeZone`|Sets the time zone for this connection, which will affect how time functions and timestamp literals behave. Should be a time zone name like "America/Los_Angeles" or offset like "-08:00".|druid.sql.planner.sqlTimeZone on the Broker (default: UTC)| |`useApproximateCountDistinct`|Whether to use an approximate cardinality algorithm for `COUNT(DISTINCT foo)`.|druid.sql.planner.useApproximateCountDistinct on the Broker (default: true)| |`useApproximateTopN`|Whether to use approximate [TopN queries](topnquery.md) when a SQL query could be expressed as such. If false, exact [GroupBy queries](groupbyquery.md) will be used instead.|druid.sql.planner.useApproximateTopN on the Broker (default: true)| -|`useParsedExprCache`|Whether to use a cache for parsed expressions. This cache is created per query and stored on heap memory. Enabling cache can be useful when planning time takes long.|druid.sql.planner.useParsedExprCache on the Broker (default: false)| +|`useParsedExprCache`|Whether to use a cache for parsed expressions. This cache is created per query and stored on heap memory. Enabling cache can be useful when planning time takes long.|`druid.sql.planner.useParsedExprCache` on the Broker (default: false)| ## Metadata tables 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 1a36a0212cc7..3684c2b9fc7d 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 @@ -97,26 +97,6 @@ public ExpressionPostAggregator( } public ExpressionPostAggregator( - final String name, - final String expression, - @Nullable final String ordering, - final ExprMacroTable macroTable, - final Supplier parsed - ) - { - this( - name, - expression, - ordering, - null, // in the future this will be computed by decorate - macroTable, - ImmutableMap.of(), - parsed, - Suppliers.memoize(() -> parsed.get().analyzeInputs().getRequiredBindings()) - ); - } - - private ExpressionPostAggregator( final String name, final String expression, @Nullable final String ordering, 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 01feea70caf3..5ea3e3aceeb3 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 @@ -51,15 +51,17 @@ public ExpressionDimFilter( @JacksonInject ExprMacroTable macroTable ) { - this.expression = expression; - this.filterTuning = filterTuning; - this.parsed = Suppliers.memoize(() -> Parser.parse(expression, macroTable)); + this(expression, filterTuning, Suppliers.memoize(() -> Parser.parse(expression, macroTable))); } - public ExpressionDimFilter(final String expression, final Supplier exprSupplier) + public ExpressionDimFilter( + final String expression, + @Nullable final FilterTuning filterTuning, + final Supplier exprSupplier + ) { this.expression = expression; - this.filterTuning = null; + this.filterTuning = filterTuning; this.parsed = exprSupplier; } diff --git a/processing/src/main/java/org/apache/druid/segment/join/JoinConditionAnalysis.java b/processing/src/main/java/org/apache/druid/segment/join/JoinConditionAnalysis.java index 4247311147f4..361504509911 100644 --- a/processing/src/main/java/org/apache/druid/segment/join/JoinConditionAnalysis.java +++ b/processing/src/main/java/org/apache/druid/segment/join/JoinConditionAnalysis.java @@ -109,7 +109,7 @@ public static JoinConditionAnalysis forExpression( final String condition, final String rightPrefix, final Supplier exprSupplier - ) + ) { final Expr conditionExpr = exprSupplier.get(); final List equiConditions = new ArrayList<>(); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java index f1a5fb02487c..b30615117996 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java @@ -660,6 +660,7 @@ private static DimFilter toExpressionLeafFilter( if (druidExpression != null) { return new ExpressionDimFilter( druidExpression.getExpression(), + null, plannerContext.getCachingExprParser().lazyParse(druidExpression.getExpression()) ); } else { diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/BaseExpressionDimFilterOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/BaseExpressionDimFilterOperatorConversion.java index bdbb60c2cff5..3f98ed4ee715 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/BaseExpressionDimFilterOperatorConversion.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/BaseExpressionDimFilterOperatorConversion.java @@ -50,6 +50,7 @@ protected static DimFilter toExpressionFilter( return new ExpressionDimFilter( filterExpr, + null, plannerContext.getCachingExprParser().lazyParse(filterExpr) ); } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/Projection.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/Projection.java index a4b711006b95..e1d60c62f420 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/Projection.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/Projection.java @@ -21,6 +21,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import org.apache.calcite.rel.core.Project; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rex.RexNode; @@ -185,6 +186,7 @@ private static void handlePostAggregatorExpression( postAggregatorExpression.getExpression(), null, plannerContext.getExprMacroTable(), + ImmutableMap.of(), plannerContext.getCachingExprParser().lazyParse(postAggregatorExpression.getExpression()) ); postAggregatorVisitor.addPostAgg(postAggregator); diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModuleTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModuleTest.java index d4660c174912..ce4d4c3d2e7e 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModuleTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/planner/CalcitePlannerModuleTest.java @@ -29,6 +29,7 @@ import org.apache.calcite.schema.SchemaPlus; import org.apache.druid.guice.LazySingleton; import org.apache.druid.jackson.JacksonModule; +import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.server.QueryLifecycleFactory; @@ -161,7 +162,7 @@ public void testInjectPlannerConfigWithCustomProperties() properties.setProperty(getPlannerConfigKey("useApproximateTopN"), "false"); properties.setProperty(getPlannerConfigKey("requireTimeCondition"), "true"); properties.setProperty(getPlannerConfigKey("awaitInitializationOnStart"), "false"); - properties.setProperty(getPlannerConfigKey("sqlTimeZone"), DateTimeZone.forID("Asia/Seoul").toString()); + properties.setProperty(getPlannerConfigKey("sqlTimeZone"), DateTimes.inferTzFromString("Asia/Seoul").toString()); properties.setProperty(getPlannerConfigKey("metadataSegmentCacheEnable"), "true"); properties.setProperty(getPlannerConfigKey("metadataSegmentPollPeriod"), "20"); properties.setProperty(getPlannerConfigKey("useParsedExprCache"), "true"); @@ -173,7 +174,7 @@ public void testInjectPlannerConfigWithCustomProperties() Assert.assertFalse(plannerConfig.isUseApproximateTopN()); Assert.assertTrue(plannerConfig.isRequireTimeCondition()); Assert.assertFalse(plannerConfig.isAwaitInitializationOnStart()); - Assert.assertEquals(DateTimeZone.forID("Asia/Seoul"), plannerConfig.getSqlTimeZone()); + Assert.assertEquals(DateTimes.inferTzFromString("Asia/Seoul"), plannerConfig.getSqlTimeZone()); Assert.assertTrue(plannerConfig.isMetadataSegmentCacheEnable()); Assert.assertEquals(20, plannerConfig.getMetadataSegmentPollPeriod()); Assert.assertTrue(plannerConfig.isUseParsedExprCache()); From ffe26d06a9e7bb2a5680ad57007934cf6cc71bcb Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sat, 13 Mar 2021 10:43:53 -0800 Subject: [PATCH 5/6] coverage --- .../org/apache/druid/query/JoinDataSource.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/JoinDataSource.java b/processing/src/main/java/org/apache/druid/query/JoinDataSource.java index d0da4e76254e..31ecd7e2caf9 100644 --- a/processing/src/main/java/org/apache/druid/query/JoinDataSource.java +++ b/processing/src/main/java/org/apache/druid/query/JoinDataSource.java @@ -24,11 +24,13 @@ 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 com.google.common.collect.ImmutableList; import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.Expr; import org.apache.druid.math.expr.ExprMacroTable; +import org.apache.druid.math.expr.Parser; import org.apache.druid.query.filter.DimFilter; import org.apache.druid.segment.join.JoinConditionAnalysis; import org.apache.druid.segment.join.JoinPrefixUtils; @@ -102,17 +104,14 @@ public static JoinDataSource create( @JacksonInject ExprMacroTable macroTable ) { - return new JoinDataSource( + return create( left, right, - StringUtils.nullToEmptyNonDruidDataString(rightPrefix), - JoinConditionAnalysis.forExpression( - Preconditions.checkNotNull(condition, "condition"), - StringUtils.nullToEmptyNonDruidDataString(rightPrefix), - macroTable - ), + rightPrefix, + condition, joinType, - leftFilter + leftFilter, + Suppliers.memoize(() -> Parser.parse(condition, macroTable)) ); } From f7c5692222e0707b566492c772bf2598d84ccd81 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sun, 28 Mar 2021 11:07:17 -0700 Subject: [PATCH 6/6] revert parser eof and assert --- core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 | 2 -- .../apache/druid/sql/calcite/filtration/BottomUpTransform.java | 3 +-- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 b/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 index a04f7a499db5..9b9eda9dfded 100644 --- a/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 +++ b/core/src/main/antlr4/org/apache/druid/math/expr/antlr/Expr.g4 @@ -15,8 +15,6 @@ grammar Expr; -start : expr EOF ; - expr : NULL # null | ('-'|'!') expr # unaryOpExpr | expr '^' expr # powOpExpr diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/filtration/BottomUpTransform.java b/sql/src/main/java/org/apache/druid/sql/calcite/filtration/BottomUpTransform.java index f594878b2947..9aacde425ab4 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/filtration/BottomUpTransform.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/filtration/BottomUpTransform.java @@ -36,8 +36,7 @@ public abstract class BottomUpTransform implements Function