diff --git a/processing/src/main/java/org/apache/druid/query/dimension/ListFilteredDimensionSpec.java b/processing/src/main/java/org/apache/druid/query/dimension/ListFilteredDimensionSpec.java index 777483b9e51..9c2f6a1e37c 100644 --- a/processing/src/main/java/org/apache/druid/query/dimension/ListFilteredDimensionSpec.java +++ b/processing/src/main/java/org/apache/druid/query/dimension/ListFilteredDimensionSpec.java @@ -21,7 +21,6 @@ package org.apache.druid.query.dimension; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Preconditions; -import com.google.common.base.Predicate; import com.google.common.base.Predicates; import it.unimi.dsi.fastutil.ints.Int2IntOpenHashMap; import org.apache.druid.common.config.NullHandling; @@ -30,8 +29,8 @@ import org.apache.druid.query.filter.DimFilterUtils; import org.apache.druid.segment.DimensionSelector; import org.apache.druid.segment.IdLookup; -import javax.annotation.Nullable; import java.nio.ByteBuffer; +import java.util.Objects; import java.util.Set; /** @@ -88,7 +87,7 @@ public class ListFilteredDimensionSpec extends BaseFilteredDimensionSpec private DimensionSelector filterWhiteList(DimensionSelector selector) { final int selectorCardinality = selector.getValueCardinality(); - if (selectorCardinality < 0 || (selector.idLookup() == null && !selector.nameLookupPossibleInAdvance())) { + if (selectorCardinality < 0 || !selector.nameLookupPossibleInAdvance()) { return new PredicateFilteredDimensionSelector(selector, Predicates.in(values)); } final int maxPossibleFilteredCardinality = values.size(); @@ -122,14 +121,7 @@ public class ListFilteredDimensionSpec extends BaseFilteredDimensionSpec if (selectorCardinality < 0 || !selector.nameLookupPossibleInAdvance()) { return new PredicateFilteredDimensionSelector( selector, - new Predicate() - { - @Override - public boolean apply(@Nullable String input) - { - return !values.contains(input); - } - } + input -> !values.contains(input) ); } final int maxPossibleFilteredCardinality = selectorCardinality; @@ -187,22 +179,16 @@ public class ListFilteredDimensionSpec extends BaseFilteredDimensionSpec if (o == null || getClass() != o.getClass()) { return false; } - ListFilteredDimensionSpec that = (ListFilteredDimensionSpec) o; - - if (isWhitelist != that.isWhitelist) { - return false; - } - return values.equals(that.values); - + return Objects.equals(getDelegate(), that.getDelegate()) + && isWhitelist == that.isWhitelist + && Objects.equals(values, that.values); } @Override public int hashCode() { - int result = values.hashCode(); - result = 31 * result + (isWhitelist ? 1 : 0); - return result; + return Objects.hash(getDelegate(), values, isWhitelist); } @Override diff --git a/processing/src/main/java/org/apache/druid/query/dimension/PredicateFilteredDimensionSelector.java b/processing/src/main/java/org/apache/druid/query/dimension/PredicateFilteredDimensionSelector.java index b607c9e2523..535442bdcc7 100644 --- a/processing/src/main/java/org/apache/druid/query/dimension/PredicateFilteredDimensionSelector.java +++ b/processing/src/main/java/org/apache/druid/query/dimension/PredicateFilteredDimensionSelector.java @@ -51,8 +51,9 @@ final class PredicateFilteredDimensionSelector extends AbstractDimensionSelector row.ensureSize(baseRowSize); int resultSize = 0; for (int i = 0; i < baseRowSize; i++) { - if (predicate.apply(selector.lookupName(baseRow.get(i)))) { - row.setValue(resultSize, i); + int id = baseRow.get(i); + if (predicate.apply(selector.lookupName(id))) { + row.setValue(resultSize, id); resultSize++; } } diff --git a/processing/src/test/java/org/apache/druid/query/dimension/ListFilteredDimensionSpecDimensionSelectorTest.java b/processing/src/test/java/org/apache/druid/query/dimension/ListFilteredDimensionSpecDimensionSelectorTest.java new file mode 100644 index 00000000000..b9befa2e630 --- /dev/null +++ b/processing/src/test/java/org/apache/druid/query/dimension/ListFilteredDimensionSpecDimensionSelectorTest.java @@ -0,0 +1,224 @@ +/* + * 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.query.dimension; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import it.unimi.dsi.fastutil.ints.Int2ObjectMap; +import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap; +import it.unimi.dsi.fastutil.objects.Object2IntMap; +import it.unimi.dsi.fastutil.objects.Object2IntOpenHashMap; +import org.apache.commons.lang3.mutable.MutableInt; +import org.apache.druid.java.util.common.NonnullPair; +import org.apache.druid.segment.DimensionSelector; +import org.apache.druid.segment.data.IndexedInts; +import org.apache.druid.testing.InitializedNullHandlingTest; +import org.junit.Assert; +import org.junit.Test; + +import java.util.List; +import java.util.function.Supplier; + +public class ListFilteredDimensionSpecDimensionSelectorTest extends InitializedNullHandlingTest +{ + private final ListFilteredDimensionSpec targetWithAllowList = new ListFilteredDimensionSpec( + new DefaultDimensionSpec("foo", "bar"), + ImmutableSet.of("val1_1", "val2_2", "val2_3"), + true + ); + private final ListFilteredDimensionSpec targetWithDenyList = new ListFilteredDimensionSpec( + new DefaultDimensionSpec("foo", "bar"), + ImmutableSet.of("val1_1", "val2_2", "val2_3"), + false + ); + private final List> data = ImmutableList.of( + ImmutableList.of("val1_1", "val1_2"), + ImmutableList.of("val2_1", "val2_2", "val2_3"), + ImmutableList.of("val3_1") + ); + + @Test + public void testNullDimensionSelectorReturnNull() + { + Assert.assertNull(targetWithAllowList.decorate((DimensionSelector) null)); + Assert.assertNull(targetWithDenyList.decorate((DimensionSelector) null)); + } + + @Test + public void testAllowListWhenDictionaryLookupIsAvailable() + { + testAllowList(false, true, true, ForwardingFilteredDimensionSelector.class); + } + + @Test + public void testAllowListWhenIdLookupIsNotAvailable() + { + testAllowList(false, false, true, ForwardingFilteredDimensionSelector.class); + } + + @Test + public void testAllowListWhenCardinalityIsUnknown() + { + testAllowList(true, true, true, PredicateFilteredDimensionSelector.class); + } + + @Test + public void testAllowListWhenNameLookupIsNotPossibleInAdvance() + { + testAllowList(false, true, false, PredicateFilteredDimensionSelector.class); + } + + @Test + public void testDenyListWhenDictionaryLookupIsAvailable() + { + testDenyList(false, true, true, ForwardingFilteredDimensionSelector.class); + } + + @Test + public void testDenyListWhenIdLookupIsNotAvailable() + { + testDenyList(false, false, true, ForwardingFilteredDimensionSelector.class); + } + + @Test + public void testDenyListWhenCardinalityIsUnknown() + { + testDenyList(true, true, true, PredicateFilteredDimensionSelector.class); + } + + @Test + public void testDenyListWhenNameLookupIsNotPossibleInAdvance() + { + testDenyList(false, true, false, PredicateFilteredDimensionSelector.class); + } + + private void testAllowList( + boolean unknownCardinality, + boolean validIdLookup, + boolean nameLookupPossibleInAdvance, + Class expectedDimensionSelectorClass + ) + { + RowSupplier rowSupplier = new RowSupplier(); + NonnullPair, Int2ObjectMap> dictionaries = createDictionaries(data); + DimensionSelector selector = targetWithAllowList.decorate( + new StringDimensionSelectorForTest( + rowSupplier, + dictionaries.lhs, + dictionaries.rhs, + unknownCardinality, + validIdLookup, + nameLookupPossibleInAdvance + ) + ); + Assert.assertSame(expectedDimensionSelectorClass, selector.getClass()); + assertAllowListFiltering(rowSupplier, selector); + } + + private void testDenyList( + boolean unknownCardinality, + boolean validIdLookup, + boolean nameLookupPossibleInAdvance, + Class expectedDimensionSelectorClass + ) + { + RowSupplier rowSupplier = new RowSupplier(); + NonnullPair, Int2ObjectMap> dictionaries = createDictionaries(data); + DimensionSelector selector = targetWithDenyList.decorate( + new StringDimensionSelectorForTest( + rowSupplier, + dictionaries.lhs, + dictionaries.rhs, + unknownCardinality, + validIdLookup, + nameLookupPossibleInAdvance + ) + ); + Assert.assertSame(expectedDimensionSelectorClass, selector.getClass()); + + assertDenyListFiltering(rowSupplier, selector); + } + + private NonnullPair, Int2ObjectMap> createDictionaries(List> values) + { + Object2IntMap dictionary = new Object2IntOpenHashMap<>(); + Int2ObjectMap reverseDictionary = new Int2ObjectOpenHashMap<>(); + MutableInt nextId = new MutableInt(0); + for (List multiValue : values) { + for (String value : multiValue) { + int dictId = dictionary.computeIntIfAbsent(value, k -> nextId.getAndIncrement()); + reverseDictionary.putIfAbsent(dictId, value); + } + } + return new NonnullPair<>(dictionary, reverseDictionary); + } + + private void assertAllowListFiltering(RowSupplier rowSupplier, DimensionSelector selector) + { + rowSupplier.set(data.get(0)); + IndexedInts encodedRow = selector.getRow(); + Assert.assertEquals(1, encodedRow.size()); + Assert.assertEquals("val1_1", selector.lookupName(encodedRow.get(0))); + + rowSupplier.set(data.get(1)); + encodedRow = selector.getRow(); + Assert.assertEquals(2, encodedRow.size()); + Assert.assertEquals("val2_2", selector.lookupName(encodedRow.get(0))); + Assert.assertEquals("val2_3", selector.lookupName(encodedRow.get(1))); + + rowSupplier.set(data.get(2)); + encodedRow = selector.getRow(); + Assert.assertEquals(0, encodedRow.size()); + } + + private void assertDenyListFiltering(RowSupplier rowSupplier, DimensionSelector selector) + { + rowSupplier.set(data.get(0)); + IndexedInts encodedRow = selector.getRow(); + Assert.assertEquals(1, encodedRow.size()); + Assert.assertEquals("val1_2", selector.lookupName(encodedRow.get(0))); + + rowSupplier.set(data.get(1)); + encodedRow = selector.getRow(); + Assert.assertEquals(1, encodedRow.size()); + Assert.assertEquals("val2_1", selector.lookupName(encodedRow.get(0))); + + rowSupplier.set(data.get(2)); + encodedRow = selector.getRow(); + Assert.assertEquals(1, encodedRow.size()); + Assert.assertEquals("val3_1", selector.lookupName(encodedRow.get(0))); + } + + private static class RowSupplier implements Supplier> + { + private List row; + + public void set(List row) + { + this.row = row; + } + + @Override + public List get() + { + return row; + } + } +} diff --git a/processing/src/test/java/org/apache/druid/query/dimension/ListFilteredDimensionSpecTest.java b/processing/src/test/java/org/apache/druid/query/dimension/ListFilteredDimensionSpecTest.java index 636da053212..7a7ee4d6a87 100644 --- a/processing/src/test/java/org/apache/druid/query/dimension/ListFilteredDimensionSpecTest.java +++ b/processing/src/test/java/org/apache/druid/query/dimension/ListFilteredDimensionSpecTest.java @@ -21,6 +21,7 @@ package org.apache.druid.query.dimension; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableSet; +import nl.jqno.equalsverifier.EqualsVerifier; import org.apache.druid.segment.DimensionSelector; import org.apache.druid.segment.TestHelper; import org.apache.druid.segment.data.IndexedInts; @@ -189,4 +190,10 @@ public class ListFilteredDimensionSpecTest Assert.assertEquals(0, selector.idLookup().lookupId("a")); Assert.assertEquals(24, selector.idLookup().lookupId("z")); } + + @Test + public void testEquals() + { + EqualsVerifier.forClass(ListFilteredDimensionSpec.class).withNonnullFields("values").usingGetClass().verify(); + } } diff --git a/processing/src/test/java/org/apache/druid/query/dimension/StringDimensionSelectorForTest.java b/processing/src/test/java/org/apache/druid/query/dimension/StringDimensionSelectorForTest.java new file mode 100644 index 00000000000..141ac655457 --- /dev/null +++ b/processing/src/test/java/org/apache/druid/query/dimension/StringDimensionSelectorForTest.java @@ -0,0 +1,126 @@ +/* + * 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.query.dimension; + +import com.google.common.base.Predicate; +import it.unimi.dsi.fastutil.ints.Int2ObjectMap; +import it.unimi.dsi.fastutil.objects.Object2IntMap; +import org.apache.druid.query.filter.ValueMatcher; +import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector; +import org.apache.druid.segment.AbstractDimensionSelector; +import org.apache.druid.segment.DimensionDictionarySelector; +import org.apache.druid.segment.DimensionSelectorUtils; +import org.apache.druid.segment.IdLookup; +import org.apache.druid.segment.data.ArrayBasedIndexedInts; +import org.apache.druid.segment.data.IndexedInts; + +import javax.annotation.Nullable; +import java.util.List; +import java.util.function.Supplier; + +public class StringDimensionSelectorForTest extends AbstractDimensionSelector +{ + private final Supplier> rowSupplier; + private final boolean unknownCardinality; + private final boolean validIdLookup; + private final boolean nameLookupPossibleInAdvance; + private final Object2IntMap dictionary; + private final Int2ObjectMap reverseDictionary; + + private final ArrayBasedIndexedInts currentRow = new ArrayBasedIndexedInts(); + + public StringDimensionSelectorForTest( + Supplier> rowSupplier, + Object2IntMap dictionary, + Int2ObjectMap reverseDictionary, + boolean unknownCardinality, + boolean validIdLookup, + boolean nameLookupPossibleInAdvance + ) + { + this.rowSupplier = rowSupplier; + this.unknownCardinality = unknownCardinality; + this.validIdLookup = validIdLookup; + this.nameLookupPossibleInAdvance = nameLookupPossibleInAdvance; + this.dictionary = dictionary; + this.reverseDictionary = reverseDictionary; + } + + @Override + public int getValueCardinality() + { + return unknownCardinality ? DimensionDictionarySelector.CARDINALITY_UNKNOWN : dictionary.size(); + } + + @Nullable + @Override + public String lookupName(int id) + { + return reverseDictionary.get(id); + } + + @Override + public boolean nameLookupPossibleInAdvance() + { + return nameLookupPossibleInAdvance; + } + + @Nullable + @Override + public IdLookup idLookup() + { + return validIdLookup ? dictionary::getInt : null; + } + + @Override + public IndexedInts getRow() + { + List multiValues = rowSupplier.get(); + currentRow.ensureSize(multiValues.size()); + for (int i = 0; i < multiValues.size(); i++) { + currentRow.setValue(i, dictionary.getInt(multiValues.get(i))); + } + currentRow.setSize(multiValues.size()); + return currentRow; + } + + @Override + public ValueMatcher makeValueMatcher(@Nullable String value) + { + return DimensionSelectorUtils.makeValueMatcherGeneric(this, value); + } + + @Override + public ValueMatcher makeValueMatcher(Predicate predicate) + { + return DimensionSelectorUtils.makeValueMatcherGeneric(this, predicate); + } + + @Override + public void inspectRuntimeShape(RuntimeShapeInspector inspector) + { + } + + @Override + public Class classOfObject() + { + return Object.class; + } +}