Skip to content

Commit

Permalink
remove extractionFn from equality, null, and range filters (#14612)
Browse files Browse the repository at this point in the history 
* remove extractionFn from equality, null, and range filters
changes:
* EqualityFilter, NullFilter, and RangeFilter no longer support extractionFn
* SQL planner will use ExpressionFilter in the small number of cases where an extractionFn would have been used if sqlUseBoundsAndSelectors is set to false instead of equality/null/range filters
* fix bugs and add tests with serde, equals, and cache key for null, equality, and range filters

* test coverage fixes bugs

* adjust

* adjust again

* so persnickety
  • Loading branch information
@clintropolis
clintropolis authored Jul 19, 2023
1 parent ae168c4 commit 68fd221
Show file tree
Hide file tree
Showing 25 changed files with 2,378 additions and 2,336 deletions.
2 changes: 1 addition & 1 deletion processing/src/main/java/org/apache/druid/math/expr/ExprEval.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1162,7 +1162,7 @@ public String asString()
} else if (value.length == 1) {
cacheStringValue(Evals.asString(value[0]));
} else {
cacheStringValue(Arrays.toString(value));
cacheStringValue(Arrays.deepToString(value));
}
}

Expand Down
96 changes: 38 additions & 58 deletions processing/src/main/java/org/apache/druid/query/filter/EqualityFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import org.apache.druid.math.expr.ExprEval;
import org.apache.druid.math.expr.ExpressionType;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.query.extraction.ExtractionFn;
import org.apache.druid.query.filter.vector.VectorValueMatcher;
import org.apache.druid.query.filter.vector.VectorValueMatcherColumnProcessorFactory;
import org.apache.druid.segment.BaseDoubleColumnValueSelector;
Expand All @@ -54,7 +53,6 @@
import org.apache.druid.segment.column.TypeSignature;
import org.apache.druid.segment.column.TypeStrategy;
import org.apache.druid.segment.column.ValueType;
import org.apache.druid.segment.filter.DimensionPredicateFilter;
import org.apache.druid.segment.filter.Filters;
import org.apache.druid.segment.filter.PredicateValueMatcherFactory;
import org.apache.druid.segment.filter.ValueMatchers;
Expand All @@ -67,6 +65,7 @@

import javax.annotation.Nullable;
import java.nio.ByteBuffer;
import java.util.Arrays;
import java.util.Comparator;
import java.util.Map;
import java.util.Objects;
Expand All @@ -78,8 +77,8 @@ public class EqualityFilter extends AbstractOptimizableDimFilter implements Filt
private final String column;
private final ColumnType matchValueType;
private final Object matchValue;
@Nullable
private final ExtractionFn extractionFn;
private final ExprEval<?> matchValueEval;

@Nullable
private final FilterTuning filterTuning;
private final DruidPredicateFactory predicateFactory;
Expand All @@ -89,7 +88,6 @@ public EqualityFilter(
@JsonProperty("column") String column,
@JsonProperty("matchValueType") ColumnType matchValueType,
@JsonProperty("matchValue") Object matchValue,
@JsonProperty("extractionFn") @Nullable ExtractionFn extractionFn,
@JsonProperty("filterTuning") @Nullable FilterTuning filterTuning
)
{
Expand All @@ -105,28 +103,25 @@ public EqualityFilter(
throw InvalidInput.exception("Invalid equality filter on column [%s], matchValue cannot be null", column);
}
this.matchValue = matchValue;
// remove once SQL planner no longer uses extractionFn
this.extractionFn = extractionFn;
this.matchValueEval = ExprEval.ofType(ExpressionType.fromColumnTypeStrict(matchValueType), matchValue);
this.filterTuning = filterTuning;
this.predicateFactory = new EqualityPredicateFactory(matchValue, matchValueType);
this.predicateFactory = new EqualityPredicateFactory(matchValueEval);
}

@Override
public byte[] getCacheKey()
{
final TypeStrategy<Object> typeStrategy = matchValueType.getStrategy();
final int size = typeStrategy.estimateSizeBytes(matchValue);
final TypeStrategy<Object> typeStrategy = matchValueEval.type().getStrategy();
final int size = typeStrategy.estimateSizeBytes(matchValueEval.value());
final ByteBuffer valueBuffer = ByteBuffer.allocate(size);
typeStrategy.write(valueBuffer, matchValue, size);
typeStrategy.write(valueBuffer, matchValueEval.value(), size);
return new CacheKeyBuilder(DimFilterUtils.EQUALS_CACHE_ID)
.appendByte(DimFilterUtils.STRING_SEPARATOR)
.appendString(column)
.appendByte(DimFilterUtils.STRING_SEPARATOR)
.appendString(matchValueType.asTypeString())
.appendByte(DimFilterUtils.STRING_SEPARATOR)
.appendByteArray(valueBuffer.array())
.appendByte(DimFilterUtils.STRING_SEPARATOR)
.appendByteArray(extractionFn == null ? new byte[0] : extractionFn.getCacheKey())
.build();
}

Expand All @@ -139,11 +134,7 @@ public DimFilter optimize()
@Override
public Filter toFilter()
{
if (extractionFn == null) {
return this;
} else {
return new DimensionPredicateFilter(column, predicateFactory, extractionFn, filterTuning);
}
return this;
}

@JsonProperty
Expand All @@ -164,14 +155,6 @@ public Object getMatchValue()
return matchValue;
}

@Nullable
@JsonProperty
@JsonInclude(JsonInclude.Include.NON_NULL)
public ExtractionFn getExtractionFn()
{
return extractionFn;
}

@Nullable
@JsonProperty
@JsonInclude(JsonInclude.Include.NON_NULL)
Expand All @@ -184,9 +167,9 @@ public FilterTuning getFilterTuning()
public String toString()
{
DimFilter.DimFilterToStringBuilder bob =
new DimFilter.DimFilterToStringBuilder().appendDimension(column, extractionFn)
new DimFilter.DimFilterToStringBuilder().appendDimension(column, null)
.append(" = ")
.append(matchValue);
.append(matchValueEval.value());

if (!ColumnType.STRING.equals(matchValueType)) {
bob.append(" (" + matchValueType.asTypeString() + ")");
Expand All @@ -210,39 +193,34 @@ public boolean equals(Object o)
if (!Objects.equals(matchValueType, that.matchValueType)) {
return false;
}
if (!Objects.equals(extractionFn, that.extractionFn)) {
return false;
}
if (!Objects.equals(filterTuning, that.filterTuning)) {
return false;
}
if (matchValueType.isArray()) {
// just use predicate to see if the values are the same
final ExprEval<?> thatValue = ExprEval.ofType(
ExpressionType.fromColumnType(that.matchValueType),
that.matchValue
);
final Predicate<Object[]> arrayPredicate = predicateFactory.makeArrayPredicate(matchValueType);
return arrayPredicate.apply(thatValue.asArray());
return Arrays.deepEquals(matchValueEval.asArray(), that.matchValueEval.asArray());
} else {
return Objects.equals(matchValue, that.matchValue);
return Objects.equals(matchValueEval.value(), that.matchValueEval.value());
}
}

@Override
public int hashCode()
{
return Objects.hash(column, matchValueType, matchValue, extractionFn, filterTuning);
return Objects.hash(column, matchValueType, matchValueEval.value(), filterTuning);
}

@Override
public RangeSet<String> getDimensionRangeSet(String dimension)
{
if (!Objects.equals(getColumn(), dimension) || getExtractionFn() != null) {
if (!Objects.equals(getColumn(), dimension)) {
return null;
}
RangeSet<String> retSet = TreeRangeSet.create();
retSet.add(Range.singleton(String.valueOf(matchValue)));
if (matchValueEval.isArray()) {
retSet.add(Range.singleton(Arrays.deepToString(matchValueEval.asArray())));
} else {
retSet.add(Range.singleton(matchValueEval.asString()));
}
return retSet;
}

Expand All @@ -261,14 +239,14 @@ public BitmapColumnIndex getBitmapColumnIndex(ColumnIndexSelector selector)

final ValueIndexes valueIndexes = indexSupplier.as(ValueIndexes.class);
if (valueIndexes != null) {
return valueIndexes.forValue(matchValue, matchValueType);
return valueIndexes.forValue(matchValueEval.value(), matchValueType);
}

if (matchValueType.isPrimitive()) {
final StringValueSetIndexes stringValueSetIndexes = indexSupplier.as(StringValueSetIndexes.class);
if (stringValueSetIndexes != null) {

return stringValueSetIndexes.forValue(String.valueOf(matchValue));
return stringValueSetIndexes.forValue(matchValueEval.asString());
}
}
// column exists, but has no indexes we can use
Expand All @@ -280,7 +258,7 @@ public ValueMatcher makeMatcher(ColumnSelectorFactory factory)
{
return ColumnProcessors.makeProcessor(
column,
new TypedConstantValueMatcherFactory(matchValue, matchValueType, predicateFactory),
new TypedConstantValueMatcherFactory(matchValueEval, predicateFactory),
factory
);
}
Expand All @@ -295,13 +273,13 @@ public VectorValueMatcher makeVectorMatcher(VectorColumnSelectorFactory factory)
column,
VectorValueMatcherColumnProcessorFactory.instance(),
factory
).makeMatcher(matchValue, matchValueType);
).makeMatcher(matchValueEval.value(), matchValueType);
}
return ColumnProcessors.makeVectorProcessor(
column,
VectorValueMatcherColumnProcessorFactory.instance(),
factory
).makeMatcher(new EqualityPredicateFactory(matchValue, matchValueType));
).makeMatcher(new EqualityPredicateFactory(matchValueEval));
}

@Override
Expand Down Expand Up @@ -345,15 +323,13 @@ public Filter rewriteRequiredColumns(Map<String, String> columnRewrites)
rewriteDimensionTo,
matchValueType,
matchValue,
extractionFn,
filterTuning
);
}

private static class EqualityPredicateFactory implements DruidPredicateFactory
{
private final ExprEval<?> matchValue;
private final ColumnType matchValueType;
private final Supplier<Predicate<String>> stringPredicateSupplier;
private final Supplier<DruidLongPredicate> longPredicateSupplier;
private final Supplier<DruidFloatPredicate> floatPredicateSupplier;
Expand All @@ -362,10 +338,9 @@ private static class EqualityPredicateFactory implements DruidPredicateFactory
private final Supplier<Predicate<Object[]>> typeDetectingArrayPredicateSupplier;
private final Supplier<Predicate<Object>> objectPredicateSupplier;

public EqualityPredicateFactory(Object matchValue, ColumnType matchValueType)
public EqualityPredicateFactory(ExprEval<?> matchValue)
{
this.matchValue = ExprEval.ofType(ExpressionType.fromColumnType(matchValueType), matchValue);
this.matchValueType = matchValueType;
this.matchValue = matchValue;
this.stringPredicateSupplier = makeStringPredicateSupplier();
this.longPredicateSupplier = makeLongPredicateSupplier();
this.floatPredicateSupplier = makeFloatPredicateSupplier();
Expand Down Expand Up @@ -469,7 +444,7 @@ private Supplier<DruidDoublePredicate> makeDoublePredicateSupplier()
private Supplier<Predicate<Object>> makeObjectPredicateSupplier()
{
return Suppliers.memoize(() -> {
if (matchValueType.equals(ColumnType.NESTED_DATA)) {
if (matchValue.type().equals(ExpressionType.NESTED_DATA)) {
return input -> Objects.equals(StructuredData.unwrap(input), StructuredData.unwrap(matchValue.value()));
}
return Predicates.equalTo(matchValue.valueOrDefault());
Expand Down Expand Up @@ -503,14 +478,20 @@ public boolean equals(Object o)
return false;
}
EqualityPredicateFactory that = (EqualityPredicateFactory) o;
return Objects.equals(matchValue, that.matchValue) && Objects.equals(matchValueType, that.matchValueType);
if (!Objects.equals(matchValue.type(), that.matchValue.type())) {
return false;
}
if (matchValue.isArray()) {
return Arrays.deepEquals(matchValue.asArray(), that.matchValue.asArray());
}
return Objects.equals(matchValue.value(), that.matchValue.value());
}


@Override
public int hashCode()
{
return Objects.hash(matchValue, matchValueType);
return Objects.hash(matchValue);
}
}

Expand All @@ -520,12 +501,11 @@ private static class TypedConstantValueMatcherFactory implements ColumnProcessor
private final PredicateValueMatcherFactory predicateMatcherFactory;

public TypedConstantValueMatcherFactory(
Object matchValue,
ColumnType matchValueType,
ExprEval<?> matchValue,
DruidPredicateFactory predicateFactory
)
{
this.matchValue = ExprEval.ofType(ExpressionType.fromColumnType(matchValueType), matchValue);
this.matchValue = matchValue;
this.predicateMatcherFactory = new PredicateValueMatcherFactory(predicateFactory);
}

Expand Down
Loading

0 comments on commit 68fd221

Please sign in to comment.