From 01feb8acc566973d77c50da7b55757ce3d4a416d Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Fri, 26 Jan 2024 12:19:58 +0000 Subject: [PATCH] Revert "Reduce amount of expression objects created during evaluations (#15552)" This reverts commit 7552dc49fb7937b4de2ef26e9857b8d26003e4cc. --- .../benchmark/ExpressionFilterBenchmark.java | 2 - .../ExpressionSelectorBenchmark.java | 156 +----------------- .../apache/druid/math/expr/ConstantExpr.java | 20 +-- .../org/apache/druid/math/expr/ExprEval.java | 6 +- .../math/expr/ExpressionTypeConversion.java | 3 - .../org/apache/druid/math/expr/ExprTest.java | 7 +- .../druid/math/expr/OutputTypeTest.java | 11 -- 7 files changed, 12 insertions(+), 193 deletions(-) diff --git a/benchmarks/src/test/java/org/apache/druid/benchmark/ExpressionFilterBenchmark.java b/benchmarks/src/test/java/org/apache/druid/benchmark/ExpressionFilterBenchmark.java index e7d8ad00bc5e..eea1804292ab 100644 --- a/benchmarks/src/test/java/org/apache/druid/benchmark/ExpressionFilterBenchmark.java +++ b/benchmarks/src/test/java/org/apache/druid/benchmark/ExpressionFilterBenchmark.java @@ -25,7 +25,6 @@ import org.apache.druid.java.util.common.granularity.Granularities; import org.apache.druid.java.util.common.guava.Sequence; import org.apache.druid.java.util.common.io.Closer; -import org.apache.druid.math.expr.ExpressionProcessing; import org.apache.druid.query.expression.TestExprMacroTable; import org.apache.druid.query.filter.AndDimFilter; import org.apache.druid.query.filter.DimFilter; @@ -71,7 +70,6 @@ public class ExpressionFilterBenchmark { static { NullHandling.initializeForTests(); - ExpressionProcessing.initializeForTests(); } @Param({"1000000"}) diff --git a/benchmarks/src/test/java/org/apache/druid/benchmark/ExpressionSelectorBenchmark.java b/benchmarks/src/test/java/org/apache/druid/benchmark/ExpressionSelectorBenchmark.java index d8e1dfc76464..fcc4cf24794a 100644 --- a/benchmarks/src/test/java/org/apache/druid/benchmark/ExpressionSelectorBenchmark.java +++ b/benchmarks/src/test/java/org/apache/druid/benchmark/ExpressionSelectorBenchmark.java @@ -25,10 +25,8 @@ import org.apache.druid.java.util.common.granularity.Granularities; import org.apache.druid.java.util.common.guava.Sequence; import org.apache.druid.java.util.common.io.Closer; -import org.apache.druid.math.expr.ExpressionProcessing; import org.apache.druid.query.dimension.DefaultDimensionSpec; import org.apache.druid.query.dimension.ExtractionDimensionSpec; -import org.apache.druid.query.expression.LookupEnabledTestExprMacroTable; import org.apache.druid.query.expression.TestExprMacroTable; import org.apache.druid.query.extraction.StrlenExtractionFn; import org.apache.druid.query.extraction.TimeFormatExtractionFn; @@ -61,21 +59,21 @@ import org.openjdk.jmh.annotations.TearDown; import org.openjdk.jmh.annotations.Warmup; import org.openjdk.jmh.infra.Blackhole; + import java.util.BitSet; import java.util.List; import java.util.concurrent.TimeUnit; @State(Scope.Benchmark) @Fork(value = 1) -@Warmup(iterations = 3, time = 3) -@Measurement(iterations = 10, time = 3) +@Warmup(iterations = 15) +@Measurement(iterations = 30) @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MILLISECONDS) public class ExpressionSelectorBenchmark { static { NullHandling.initializeForTests(); - ExpressionProcessing.initializeForTests(); } @Param({"1000000"}) @@ -453,154 +451,6 @@ public void stringConcatAndCompareOnLong(Blackhole blackhole) blackhole.consume(results); } - @Benchmark - public void caseSearched1(Blackhole blackhole) - { - final Sequence cursors = new QueryableIndexStorageAdapter(index).makeCursors( - null, - index.getDataInterval(), - VirtualColumns.create( - ImmutableList.of( - new ExpressionVirtualColumn( - "v", - "case_searched(s == 'asd' || isnull(s) || s == 'xxx', 1, s == 'foo' || s == 'bar', 2, 3)", - ColumnType.LONG, - TestExprMacroTable.INSTANCE - ) - ) - ), - Granularities.ALL, - false, - null - ); - - final List results = cursors - .map(cursor -> { - final ColumnValueSelector selector = cursor.getColumnSelectorFactory().makeColumnValueSelector("v"); - consumeLong(cursor, selector, blackhole); - return null; - }) - .toList(); - - blackhole.consume(results); - } - - @Benchmark - public void caseSearched2(Blackhole blackhole) - { - final Sequence cursors = new QueryableIndexStorageAdapter(index).makeCursors( - null, - index.getDataInterval(), - VirtualColumns.create( - ImmutableList.of( - new ExpressionVirtualColumn( - "v", - "case_searched(s == 'asd' || isnull(s) || n == 1, 1, n == 2, 2, 3)", - ColumnType.LONG, - TestExprMacroTable.INSTANCE - ) - ) - ), - Granularities.ALL, - false, - null - ); - - final List results = cursors - .map(cursor -> { - final ColumnValueSelector selector = cursor.getColumnSelectorFactory().makeColumnValueSelector("v"); - consumeLong(cursor, selector, blackhole); - return null; - }) - .toList(); - - blackhole.consume(results); - } - - @Benchmark - public void caseSearchedWithLookup(Blackhole blackhole) - { - final Sequence cursors = new QueryableIndexStorageAdapter(index).makeCursors( - null, - index.getDataInterval(), - VirtualColumns.create( - ImmutableList.of( - new ExpressionVirtualColumn( - "v", - "case_searched(n == 1001, -1, " - + "lookup(s, 'lookyloo') == 'asd1', 1, " - + "lookup(s, 'lookyloo') == 'asd2', 2, " - + "lookup(s, 'lookyloo') == 'asd3', 3, " - + "lookup(s, 'lookyloo') == 'asd4', 4, " - + "lookup(s, 'lookyloo') == 'asd5', 5, " - + "-2)", - ColumnType.LONG, - LookupEnabledTestExprMacroTable.INSTANCE - ) - ) - ), - Granularities.ALL, - false, - null - ); - - final List results = cursors - .map(cursor -> { - final ColumnValueSelector selector = cursor.getColumnSelectorFactory().makeColumnValueSelector("v"); - consumeLong(cursor, selector, blackhole); - return null; - }) - .toList(); - - blackhole.consume(results); - } - - @Benchmark - public void caseSearchedWithLookup2(Blackhole blackhole) - { - final Sequence cursors = new QueryableIndexStorageAdapter(index).makeCursors( - null, - index.getDataInterval(), - VirtualColumns.create( - ImmutableList.of( - new ExpressionVirtualColumn( - "ll", - "lookup(s, 'lookyloo')", - ColumnType.STRING, - LookupEnabledTestExprMacroTable.INSTANCE - ), - new ExpressionVirtualColumn( - "v", - "case_searched(n == 1001, -1, " - + "ll == 'asd1', 1, " - + "ll == 'asd2', 2, " - + "ll == 'asd3', 3, " - + "ll == 'asd4', 4, " - + "ll == 'asd5', 5, " - + "-2)", - ColumnType.LONG, - LookupEnabledTestExprMacroTable.INSTANCE - ) - ) - ), - Granularities.ALL, - false, - null - ); - - final List results = cursors - .map(cursor -> { - final ColumnValueSelector selector = cursor.getColumnSelectorFactory().makeColumnValueSelector("v"); - consumeLong(cursor, selector, blackhole); - return null; - }) - .toList(); - - blackhole.consume(results); - } - - - private void consumeDimension(final Cursor cursor, final DimensionSelector selector, final Blackhole blackhole) { if (selector.getValueCardinality() >= 0) { diff --git a/processing/src/main/java/org/apache/druid/math/expr/ConstantExpr.java b/processing/src/main/java/org/apache/druid/math/expr/ConstantExpr.java index 8790b2ed6145..fdf6f080ee9c 100644 --- a/processing/src/main/java/org/apache/druid/math/expr/ConstantExpr.java +++ b/processing/src/main/java/org/apache/druid/math/expr/ConstantExpr.java @@ -157,12 +157,9 @@ public int hashCode() class LongExpr extends ConstantExpr { - private final ExprEval expr; - LongExpr(Long value) { super(ExpressionType.LONG, Preconditions.checkNotNull(value, "value")); - expr = ExprEval.ofLong(value); } @Override @@ -174,7 +171,7 @@ public String toString() @Override public ExprEval eval(ObjectBinding bindings) { - return expr; + return ExprEval.ofLong(value); } @Override @@ -243,12 +240,9 @@ public String toString() class DoubleExpr extends ConstantExpr { - private final ExprEval expr; - DoubleExpr(Double value) { super(ExpressionType.DOUBLE, Preconditions.checkNotNull(value, "value")); - expr = ExprEval.ofDouble(value); } @Override @@ -260,7 +254,7 @@ public String toString() @Override public ExprEval eval(ObjectBinding bindings) { - return expr; + return ExprEval.ofDouble(value); } @Override @@ -329,12 +323,9 @@ public String toString() class StringExpr extends ConstantExpr { - private final ExprEval expr; - StringExpr(@Nullable String value) { super(ExpressionType.STRING, NullHandling.emptyToNullIfNeeded(value)); - expr = ExprEval.of(value); } @Override @@ -346,7 +337,7 @@ public String toString() @Override public ExprEval eval(ObjectBinding bindings) { - return expr; + return ExprEval.of(value); } @Override @@ -473,18 +464,15 @@ public String toString() class ComplexExpr extends ConstantExpr { - private final ExprEval expr; - protected ComplexExpr(ExpressionType outputType, @Nullable Object value) { super(outputType, value); - expr = ExprEval.ofComplex(outputType, value); } @Override public ExprEval eval(ObjectBinding bindings) { - return expr; + return ExprEval.ofComplex(outputType, value); } @Override diff --git a/processing/src/main/java/org/apache/druid/math/expr/ExprEval.java b/processing/src/main/java/org/apache/druid/math/expr/ExprEval.java index 9c0f5e2736ac..8b8cee07824f 100644 --- a/processing/src/main/java/org/apache/druid/math/expr/ExprEval.java +++ b/processing/src/main/java/org/apache/druid/math/expr/ExprEval.java @@ -366,7 +366,7 @@ public static ExprEval ofBoolean(boolean value, ExpressionType type) case DOUBLE: return ExprEval.of(Evals.asDouble(value)); case LONG: - return ofLongBoolean(value); + return ExprEval.of(Evals.asLong(value)); case STRING: return ExprEval.of(String.valueOf(value)); default: @@ -379,7 +379,7 @@ public static ExprEval ofBoolean(boolean value, ExpressionType type) */ public static ExprEval ofLongBoolean(boolean value) { - return value ? LongExprEval.TRUE : LongExprEval.FALSE; + return ExprEval.of(Evals.asLong(value)); } public static ExprEval ofComplex(ExpressionType outputType, @Nullable Object value) @@ -932,8 +932,6 @@ public Expr toExpr() private static class LongExprEval extends NumericExprEval { - private static final LongExprEval TRUE = new LongExprEval(Evals.asLong(true)); - private static final LongExprEval FALSE = new LongExprEval(Evals.asLong(false)); private static final LongExprEval OF_NULL = new LongExprEval(null); private LongExprEval(@Nullable Number value) diff --git a/processing/src/main/java/org/apache/druid/math/expr/ExpressionTypeConversion.java b/processing/src/main/java/org/apache/druid/math/expr/ExpressionTypeConversion.java index efe0328a8375..ba576afa0c15 100644 --- a/processing/src/main/java/org/apache/druid/math/expr/ExpressionTypeConversion.java +++ b/processing/src/main/java/org/apache/druid/math/expr/ExpressionTypeConversion.java @@ -55,9 +55,6 @@ public static ExpressionType autoDetect(ExprEval eval, ExprEval otherEval) { ExpressionType type = eval.type(); ExpressionType otherType = otherEval.type(); - if (type == otherType && type.getType().isPrimitive()) { - return type; - } if (Types.is(type, ExprType.STRING) && Types.is(otherType, ExprType.STRING)) { return ExpressionType.STRING; } diff --git a/processing/src/test/java/org/apache/druid/math/expr/ExprTest.java b/processing/src/test/java/org/apache/druid/math/expr/ExprTest.java index f8d222e08383..3c089ac5b6af 100644 --- a/processing/src/test/java/org/apache/druid/math/expr/ExprTest.java +++ b/processing/src/test/java/org/apache/druid/math/expr/ExprTest.java @@ -139,7 +139,7 @@ public void testEqualsContractForUnaryMinusExpr() public void testEqualsContractForStringExpr() { EqualsVerifier.forClass(StringExpr.class) - .withIgnoredFields("outputType", "expr") + .withIgnoredFields("outputType") .withPrefabValues(ExpressionType.class, ExpressionType.STRING, ExpressionType.DOUBLE) .usingGetClass() .verify(); @@ -149,7 +149,7 @@ public void testEqualsContractForStringExpr() public void testEqualsContractForDoubleExpr() { EqualsVerifier.forClass(DoubleExpr.class) - .withIgnoredFields("outputType", "expr") + .withIgnoredFields("outputType") .withPrefabValues(ExpressionType.class, ExpressionType.DOUBLE, ExpressionType.LONG) .usingGetClass() .verify(); @@ -159,7 +159,7 @@ public void testEqualsContractForDoubleExpr() public void testEqualsContractForLongExpr() { EqualsVerifier.forClass(LongExpr.class) - .withIgnoredFields("outputType", "expr") + .withIgnoredFields("outputType") .withPrefabValues(ExpressionType.class, ExpressionType.LONG, ExpressionType.STRING) .usingGetClass() .verify(); @@ -187,7 +187,6 @@ public void testEqualsContractForComplexExpr() ExpressionTypeFactory.getInstance().ofComplex("bar") ) .withNonnullFields("outputType") - .withIgnoredFields("expr") .usingGetClass() .verify(); } diff --git a/processing/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java b/processing/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java index d313749fef8b..f8d663abc70b 100644 --- a/processing/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java +++ b/processing/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java @@ -453,8 +453,6 @@ public void testEvalAutoConversion() final ExprEval longEval = ExprEval.of(1L); final ExprEval doubleEval = ExprEval.of(1.0); final ExprEval arrayEval = ExprEval.ofLongArray(new Long[]{1L, 2L, 3L}); - final ExprEval complexEval = ExprEval.ofComplex(ExpressionType.UNKNOWN_COMPLEX, new Object()); - final ExprEval complexEval2 = ExprEval.ofComplex(new ExpressionType(ExprType.COMPLEX, null, null), new Object()); // only long stays long Assert.assertEquals(ExpressionType.LONG, ExpressionTypeConversion.autoDetect(longEval, longEval)); @@ -481,15 +479,6 @@ public void testEvalAutoConversion() Assert.assertEquals(ExpressionType.DOUBLE, ExpressionTypeConversion.autoDetect(nullStringEval, arrayEval)); Assert.assertEquals(ExpressionType.DOUBLE, ExpressionTypeConversion.autoDetect(doubleEval, arrayEval)); Assert.assertEquals(ExpressionType.DOUBLE, ExpressionTypeConversion.autoDetect(longEval, arrayEval)); - - Assert.assertEquals(ExpressionType.DOUBLE, ExpressionTypeConversion.autoDetect(longEval, complexEval)); - Assert.assertEquals(ExpressionType.DOUBLE, ExpressionTypeConversion.autoDetect(doubleEval, complexEval)); - Assert.assertEquals(ExpressionType.DOUBLE, ExpressionTypeConversion.autoDetect(arrayEval, complexEval)); - Assert.assertEquals(ExpressionType.DOUBLE, ExpressionTypeConversion.autoDetect(complexEval, complexEval)); - Assert.assertEquals( - ExpressionTypeConversion.autoDetect(complexEval, complexEval), - ExpressionTypeConversion.autoDetect(complexEval2, complexEval) - ); } @Test