Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "Reduce amount of expression objects created during evaluations (#15552)" #15768

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Revert "Reduce amount of expression objects created during evaluations (#15552)" #15768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions benchmarks/src/test/java/org/apache/druid/benchmark/ExpressionFilterBenchmark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -71,7 +70,6 @@ public class ExpressionFilterBenchmark
{
static {
NullHandling.initializeForTests();
ExpressionProcessing.initializeForTests();
}

@Param({"1000000"})
Expand Down
156 changes: 3 additions & 153 deletions benchmarks/src/test/java/org/apache/druid/benchmark/ExpressionSelectorBenchmark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"})
Expand Down Expand Up @@ -453,154 +451,6 @@ public void stringConcatAndCompareOnLong(Blackhole blackhole)
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) {
Expand Down
20 changes: 4 additions & 16 deletions processing/src/main/java/org/apache/druid/math/expr/ConstantExpr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,12 +157,9 @@ public int hashCode()

class LongExpr extends ConstantExpr<Long>
{
private final ExprEval expr;

LongExpr(Long value)
{
super(ExpressionType.LONG, Preconditions.checkNotNull(value, "value"));
expr = ExprEval.ofLong(value);
}

@Override
Expand All @@ -174,7 +171,7 @@ public String toString()
@Override
public ExprEval eval(ObjectBinding bindings)
{
return expr;
return ExprEval.ofLong(value);
}

@Override
Expand Down Expand Up @@ -243,12 +240,9 @@ public String toString()

class DoubleExpr extends ConstantExpr<Double>
{
private final ExprEval expr;

DoubleExpr(Double value)
{
super(ExpressionType.DOUBLE, Preconditions.checkNotNull(value, "value"));
expr = ExprEval.ofDouble(value);
}

@Override
Expand All @@ -260,7 +254,7 @@ public String toString()
@Override
public ExprEval eval(ObjectBinding bindings)
{
return expr;
return ExprEval.ofDouble(value);
}

@Override
Expand Down Expand Up @@ -329,12 +323,9 @@ public String toString()

class StringExpr extends ConstantExpr<String>
{
private final ExprEval expr;

StringExpr(@Nullable String value)
{
super(ExpressionType.STRING, NullHandling.emptyToNullIfNeeded(value));
expr = ExprEval.of(value);
}

@Override
Expand All @@ -346,7 +337,7 @@ public String toString()
@Override
public ExprEval eval(ObjectBinding bindings)
{
return expr;
return ExprEval.of(value);
}

@Override
Expand Down Expand Up @@ -473,18 +464,15 @@ public String toString()

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
Expand Down
6 changes: 2 additions & 4 deletions processing/src/main/java/org/apache/druid/math/expr/ExprEval.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 0 additions & 3 deletions processing/src/main/java/org/apache/druid/math/expr/ExpressionTypeConversion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
7 changes: 3 additions & 4 deletions processing/src/test/java/org/apache/druid/math/expr/ExprTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -187,7 +187,6 @@ public void testEqualsContractForComplexExpr()
ExpressionTypeFactory.getInstance().ofComplex("bar")
)
.withNonnullFields("outputType")
.withIgnoredFields("expr")
.usingGetClass()
.verify();
}
Expand Down
11 changes: 0 additions & 11 deletions processing/src/test/java/org/apache/druid/math/expr/OutputTypeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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
Expand Down
Loading