This is an automated email from the ASF dual-hosted git repository.
abhishek pushed a commit to branch 29.0.0
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/29.0.0 by this push:
new e93c41457a0 Revert "Reduce amount of expression objects created during
evaluations (#15552)" (#15768)
e93c41457a0 is described below
commit e93c41457a0194b94bf5abf2db0708b320977f4e
Author: Zoltan Haindrich <[email protected]>
AuthorDate: Mon Jan 29 06:10:16 2024 +0100
Revert "Reduce amount of expression objects created during evaluations
(#15552)" (#15768)
This reverts commit 7552dc49fb7937b4de2ef26e9857b8d26003e4cc.
---
.../druid/benchmark/ExpressionFilterBenchmark.java | 2 -
.../benchmark/ExpressionSelectorBenchmark.java | 156 +--------------------
.../org/apache/druid/math/expr/ConstantExpr.java | 20 +--
.../java/org/apache/druid/math/expr/ExprEval.java | 6 +-
.../druid/math/expr/ExpressionTypeConversion.java | 3 -
.../java/org/apache/druid/math/expr/ExprTest.java | 7 +-
.../org/apache/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 e7d8ad00bc5..eea1804292a 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.Intervals;
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 d8e1dfc7646..fcc4cf24794 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.Intervals;
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.State;
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 class ExpressionSelectorBenchmark
blackhole.consume(results);
}
- @Benchmark
- public void caseSearched1(Blackhole blackhole)
- {
- final Sequence<Cursor> 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<Cursor> 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<Cursor> 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<Cursor> 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 8790b2ed614..fdf6f080ee9 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 @@ class BigIntegerExpr extends ConstantExpr<BigInteger>
class LongExpr extends ConstantExpr<Long>
{
- private final ExprEval expr;
-
LongExpr(Long value)
{
super(ExpressionType.LONG, Preconditions.checkNotNull(value, "value"));
- expr = ExprEval.ofLong(value);
}
@Override
@@ -174,7 +171,7 @@ class LongExpr extends ConstantExpr<Long>
@Override
public ExprEval eval(ObjectBinding bindings)
{
- return expr;
+ return ExprEval.ofLong(value);
}
@Override
@@ -243,12 +240,9 @@ class NullLongExpr extends ConstantExpr<Long>
class DoubleExpr extends ConstantExpr<Double>
{
- private final ExprEval expr;
-
DoubleExpr(Double value)
{
super(ExpressionType.DOUBLE, Preconditions.checkNotNull(value, "value"));
- expr = ExprEval.ofDouble(value);
}
@Override
@@ -260,7 +254,7 @@ class DoubleExpr extends ConstantExpr<Double>
@Override
public ExprEval eval(ObjectBinding bindings)
{
- return expr;
+ return ExprEval.ofDouble(value);
}
@Override
@@ -329,12 +323,9 @@ class NullDoubleExpr extends ConstantExpr<Double>
class StringExpr extends ConstantExpr<String>
{
- private final ExprEval expr;
-
StringExpr(@Nullable String value)
{
super(ExpressionType.STRING, NullHandling.emptyToNullIfNeeded(value));
- expr = ExprEval.of(value);
}
@Override
@@ -346,7 +337,7 @@ class StringExpr extends ConstantExpr<String>
@Override
public ExprEval eval(ObjectBinding bindings)
{
- return expr;
+ return ExprEval.of(value);
}
@Override
@@ -473,18 +464,15 @@ class ArrayExpr extends ConstantExpr<Object[]>
class ComplexExpr extends ConstantExpr<Object>
{
- 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 9c0f5e2736a..8b8cee07824 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 abstract class ExprEval<T>
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 abstract class ExprEval<T>
*/
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 abstract class ExprEval<T>
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 efe0328a837..ba576afa0c1 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 class ExpressionTypeConversion
{
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 f8d222e0838..3c089ac5b6a 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 class ExprTest
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 class ExprTest
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 class ExprTest
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 class ExprTest
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 d313749fef8..f8d663abc70 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 class OutputTypeTest extends
InitializedNullHandlingTest
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 class OutputTypeTest extends
InitializedNullHandlingTest
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
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
