From 595ce60eb37d18508862a6cbe5e6768e7adabc43 Mon Sep 17 00:00:00 2001 From: leventov Date: Mon, 16 Jan 2017 17:18:36 -0600 Subject: [PATCH 01/12] * Add DimensionSelector.idLookup() and nameLookupPossibleInAdvance() to allow better inspection of features DimensionSelectors supports, and safer code working with DimensionSelectors in BaseTopNAlgorithm, BaseFilteredDimensionSpec, DimensionSelectorUtils; * Add PredicateFilteringDimensionSelector, to make BaseFilteredDimensionSpec to be able to decorate DimensionSelectors with unknown cardinality; * Add DimensionSelector.makeValueMatcher() (two kinds) for DimensionSelector-side specifics-aware optimization of ValueMatchers; * Optimize getRow() in BaseFilteredDimensionSpec's DimensionSelector, StringDimensionIndexer's DimensionSelector and SingleScanTimeDimSelector; * Use two static singletons, TrueValueMatcher and FalseValueMatcher, instead of BooleanValueMatcher; * Add NullStringObjectColumnSelector singleton and use it in MapVirtualColumn --- .../io/druid/segment/MapVirtualColumn.java | 75 +++++-- .../dimension/BaseFilteredDimensionSpec.java | 54 ----- .../ForwardingFilteredDimensionSelector.java | 200 +++++++++++++++++ .../dimension/ListFilteredDimensionSpec.java | 72 ++++-- .../PredicateFilteredDimensionSelector.java | 134 +++++++++++ .../dimension/RegexFilteredDimensionSpec.java | 34 ++- ...ingValueMatcherColumnSelectorStrategy.java | 106 +-------- .../RowBasedColumnSelectorFactory.java | 144 +++++++++++- .../epinephelinae/GroupByRowProcessor.java | 2 +- .../druid/query/topn/BaseTopNAlgorithm.java | 7 +- .../io/druid/segment/DimensionSelector.java | 29 ++- .../druid/segment/DimensionSelectorUtils.java | 210 ++++++++++++++++++ .../main/java/io/druid/segment/IdLookup.java | 31 +++ .../druid/segment/NullDimensionSelector.java | 33 ++- .../NullStringObjectColumnSelector.java | 46 ++++ .../segment/QueryableIndexStorageAdapter.java | 101 ++++++++- .../segment/SingleScanTimeDimSelector.java | 93 ++++---- .../druid/segment/StringDimensionIndexer.java | 152 +++++++++---- .../segment/data/ArrayBasedIndexedInts.java | 39 +++- .../druid/segment/data/SingleIndexedInt.java | 67 ++++++ .../io/druid/segment/filter/AndFilter.java | 2 +- .../segment/filter/BooleanValueMatcher.java | 13 +- .../segment/filter/FalseValueMatcher.java | 42 ++++ .../java/io/druid/segment/filter/Filters.java | 4 +- .../segment/filter/TrueValueMatcher.java | 42 ++++ .../IncrementalIndexStorageAdapter.java | 2 +- .../aggregation/FilteredAggregatorTest.java | 49 +++- .../CardinalityAggregatorTest.java | 34 ++- .../ListFilteredDimensionSpecTest.java | 12 +- .../RegexFilteredDimensionSpecTest.java | 4 +- .../dimension/TestDimensionSelector.java | 35 ++- .../segment/NullDimensionSelectorTest.java | 6 +- 32 files changed, 1532 insertions(+), 342 deletions(-) create mode 100644 processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java create mode 100644 processing/src/main/java/io/druid/query/dimension/PredicateFilteredDimensionSelector.java create mode 100644 processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java create mode 100644 processing/src/main/java/io/druid/segment/IdLookup.java create mode 100644 processing/src/main/java/io/druid/segment/NullStringObjectColumnSelector.java create mode 100644 processing/src/main/java/io/druid/segment/data/SingleIndexedInt.java create mode 100644 processing/src/main/java/io/druid/segment/filter/FalseValueMatcher.java create mode 100644 processing/src/main/java/io/druid/segment/filter/TrueValueMatcher.java diff --git a/extensions-contrib/virtual-columns/src/main/java/io/druid/segment/MapVirtualColumn.java b/extensions-contrib/virtual-columns/src/main/java/io/druid/segment/MapVirtualColumn.java index 75fe13ad6c82..c94cba1c0fe5 100644 --- a/extensions-contrib/virtual-columns/src/main/java/io/druid/segment/MapVirtualColumn.java +++ b/extensions-contrib/virtual-columns/src/main/java/io/druid/segment/MapVirtualColumn.java @@ -30,6 +30,7 @@ import java.nio.ByteBuffer; import java.util.Map; +import java.util.Objects; /** */ @@ -94,33 +95,65 @@ public Map get() }; } - final int keyId = keySelector.lookupId(dimension.substring(index + 1)); - - return new ObjectColumnSelector() - { - @Override - public Class classOfObject() - { - return String.class; + IdLookup keyIdLookup = keySelector.idLookup(); + if (keyIdLookup != null) { + final int keyId = keyIdLookup.lookupId(dimension.substring(index + 1)); + if (keyId < 0) { + return NullStringObjectColumnSelector.instance(); } - - @Override - public String get() + return new ObjectColumnSelector() { - final IndexedInts keyIndices = keySelector.getRow(); - final IndexedInts valueIndices = valueSelector.getRow(); - if (keyIndices == null || valueIndices == null) { + @Override + public Class classOfObject() + { + return String.class; + } + + @Override + public String get() + { + final IndexedInts keyIndices = keySelector.getRow(); + final IndexedInts valueIndices = valueSelector.getRow(); + if (keyIndices == null || valueIndices == null) { + return null; + } + final int limit = Math.min(keyIndices.size(), valueIndices.size()); + for (int i = 0; i < limit; i++) { + if (keyIndices.get(i) == keyId) { + return valueSelector.lookupName(valueIndices.get(i)); + } + } return null; } - final int limit = Math.min(keyIndices.size(), valueIndices.size()); - for (int i = 0; i < limit; i++) { - if (keyIndices.get(i) == keyId) { - return valueSelector.lookupName(valueIndices.get(i)); + }; + } else { + final String key = dimension.substring(index + 1); + return new ObjectColumnSelector() + { + @Override + public Class classOfObject() + { + return String.class; + } + + @Override + public String get() + { + final IndexedInts keyIndices = keySelector.getRow(); + final IndexedInts valueIndices = valueSelector.getRow(); + if (keyIndices == null || valueIndices == null) { + return null; + } + final int limit = Math.min(keyIndices.size(), valueIndices.size()); + for (int i = 0; i < limit; i++) { + if (Objects.equals(keySelector.lookupName(keyIndices.get(i)), key)) { + return valueSelector.lookupName(valueIndices.get(i)); + } } + return null; } - return null; - } - }; + }; + } } @Override diff --git a/processing/src/main/java/io/druid/query/dimension/BaseFilteredDimensionSpec.java b/processing/src/main/java/io/druid/query/dimension/BaseFilteredDimensionSpec.java index eb3a8010481c..50ff76e88bb7 100644 --- a/processing/src/main/java/io/druid/query/dimension/BaseFilteredDimensionSpec.java +++ b/processing/src/main/java/io/druid/query/dimension/BaseFilteredDimensionSpec.java @@ -22,13 +22,6 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Preconditions; import io.druid.query.extraction.ExtractionFn; -import io.druid.segment.DimensionSelector; -import io.druid.segment.data.IndexedInts; -import io.druid.segment.data.ListBasedIndexedInts; - -import java.util.ArrayList; -import java.util.List; -import java.util.Map; /** */ @@ -72,51 +65,4 @@ public boolean preservesOrdering() { return delegate.preservesOrdering(); } - - protected static DimensionSelector decorate( - final DimensionSelector selector, - final Map forwardMapping, - final int[] reverseMapping - ) - { - if (selector == null) { - return selector; - } - - return new DimensionSelector() - { - @Override - public IndexedInts getRow() - { - IndexedInts baseRow = selector.getRow(); - List result = new ArrayList<>(baseRow.size()); - - for (int i : baseRow) { - if (forwardMapping.containsKey(i)) { - result.add(forwardMapping.get(i)); - } - } - - return new ListBasedIndexedInts(result); - } - - @Override - public int getValueCardinality() - { - return forwardMapping.size(); - } - - @Override - public String lookupName(int id) - { - return selector.lookupName(reverseMapping[id]); - } - - @Override - public int lookupId(String name) - { - return forwardMapping.get(selector.lookupId(name)); - } - }; - } } diff --git a/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java b/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java new file mode 100644 index 000000000000..3f094ff1e9a8 --- /dev/null +++ b/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java @@ -0,0 +1,200 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets 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 io.druid.query.dimension; + +import com.google.common.base.Preconditions; +import com.google.common.base.Predicate; +import io.druid.java.util.common.IAE; +import io.druid.query.filter.ValueMatcher; +import io.druid.segment.DimensionSelector; +import io.druid.segment.DimensionSelectorUtils; +import io.druid.segment.IdLookup; +import io.druid.segment.data.ArrayBasedIndexedInts; +import io.druid.segment.data.IndexedInts; +import io.druid.segment.filter.BooleanValueMatcher; +import it.unimi.dsi.fastutil.ints.Int2IntMap; + +import javax.annotation.Nullable; +import java.util.BitSet; +import java.util.Objects; + +final class ForwardingFilteredDimensionSelector implements DimensionSelector, IdLookup +{ + private final DimensionSelector selector; + private final Int2IntMap forwardMapping; + private final int[] reverseMapping; + + /** + * @param selector must return true from {@link DimensionSelector#nameLookupPossibleInAdvance()} + * @param forwardMapping must have {@link Int2IntMap#defaultReturnValue(int)} configured to -1. + */ + ForwardingFilteredDimensionSelector(DimensionSelector selector, Int2IntMap forwardMapping, int[] reverseMapping) + { + this.selector = Preconditions.checkNotNull(selector); + if (!selector.nameLookupPossibleInAdvance()) { + throw new IAE("selector.nameLookupPossibleInAdvance() should return true"); + } + this.forwardMapping = Preconditions.checkNotNull(forwardMapping); + if (forwardMapping.defaultReturnValue() != -1) { + throw new IAE("forwardMapping.defaultReturnValue() should be -1"); + } + this.reverseMapping = Preconditions.checkNotNull(reverseMapping); + } + + @Override + public IndexedInts getRow() + { + IndexedInts baseRow = selector.getRow(); + int baseRowSize = baseRow.size(); + int[] result = new int[baseRowSize]; + int resultSize = 0; + for (int i = 0; i < baseRowSize; i++) { + int forwardedValue = forwardMapping.get(baseRow.get(i)); + if (forwardedValue >= 0) { + result[resultSize++] = forwardedValue; + } + } + return new ArrayBasedIndexedInts(result, resultSize); + } + + @Override + public ValueMatcher makeValueMatcher(final String value, final boolean matchNull) + { + IdLookup idLookup = idLookup(); + if (idLookup != null) { + final int valueId = idLookup.lookupId(value); + if (valueId >= 0 || matchNull) { + return new ValueMatcher() + { + @Override + public boolean matches() + { + final IndexedInts baseRow = selector.getRow(); + final int baseRowSize = baseRow.size(); + boolean nullRow = true; + for (int i = 0; i < baseRowSize; i++) { + int forwardedValue = forwardMapping.get(baseRow.get(i)); + if (forwardedValue >= 0) { + // Make the following check after the `forwardedValue >= 0` check, because if forwardedValue is -1 and + // valueId is -1, we don't want to return true from matches(). + if (forwardedValue == valueId) { + return true; + } + nullRow = false; + } + } + // null should match empty rows in multi-value columns + return nullRow && matchNull; + } + }; + } else { + return BooleanValueMatcher.of(false); + } + } else { + return new ValueMatcher() + { + @Override + public boolean matches() + { + final IndexedInts baseRow = selector.getRow(); + final int baseRowSize = baseRow.size(); + boolean nullRow = true; + for (int i = 0; i < baseRowSize; i++) { + int rowValueId = baseRow.get(i); + int forwardedValue = forwardMapping.get(rowValueId); + if (forwardedValue >= 0) { + if (Objects.equals(selector.lookupName(rowValueId), value)) { + return true; + } + nullRow = false; + } + } + // null should match empty rows in multi-value columns + return nullRow && matchNull; + } + }; + } + } + + @Override + public ValueMatcher makeValueMatcher(Predicate predicate, final boolean matchNull) + { + final BitSet valueIds = DimensionSelectorUtils.makePredicateMatchingSet(this, predicate); + return new ValueMatcher() + { + @Override + public boolean matches() + { + final IndexedInts baseRow = selector.getRow(); + final int baseRowSize = baseRow.size(); + boolean nullRow = true; + for (int i = 0; i < baseRowSize; ++i) { + int forwardedValue = forwardMapping.get(baseRow.get(i)); + if (forwardedValue >= 0) { + if (valueIds.get(forwardedValue)) { + return true; + } + nullRow = false; + } + } + // null should match empty rows in multi-value columns + return nullRow && matchNull; + } + }; + } + + @Override + public int getValueCardinality() + { + return forwardMapping.size(); + } + + @Override + public String lookupName(int id) + { + return selector.lookupName(reverseMapping[id]); + } + + @Override + public boolean nameLookupPossibleInAdvance() + { + return true; + } + + @Nullable + @Override + public IdLookup idLookup() + { + final IdLookup baseIdLookup = selector.idLookup(); + if (baseIdLookup != null) { + return this; + } else { + return null; + } + } + + @Override + public int lookupId(String name) + { + // Don't cache selector.idLookup(), because it is likely implemented as `return this`, in this case caching only + // makes access slower. + return forwardMapping.get(selector.idLookup().lookupId(name)); + } +} diff --git a/processing/src/main/java/io/druid/query/dimension/ListFilteredDimensionSpec.java b/processing/src/main/java/io/druid/query/dimension/ListFilteredDimensionSpec.java index 330d09242e78..c1234f6ebcff 100644 --- a/processing/src/main/java/io/druid/query/dimension/ListFilteredDimensionSpec.java +++ b/processing/src/main/java/io/druid/query/dimension/ListFilteredDimensionSpec.java @@ -21,14 +21,18 @@ 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 com.google.common.base.Strings; import io.druid.java.util.common.StringUtils; import io.druid.query.filter.DimFilterUtils; import io.druid.segment.DimensionSelector; +import io.druid.segment.IdLookup; +import it.unimi.dsi.fastutil.ints.Int2IntMap; +import it.unimi.dsi.fastutil.ints.Int2IntOpenHashMap; +import javax.annotation.Nullable; import java.nio.ByteBuffer; -import java.util.HashMap; -import java.util.Map; import java.util.Set; /** @@ -71,24 +75,31 @@ public boolean isWhitelist() public DimensionSelector decorate(final DimensionSelector selector) { if (selector == null) { - return selector; + return null; } - final int selectorCardinality = selector.getValueCardinality(); - if (selectorCardinality < 0) { - throw new UnsupportedOperationException("Cannot decorate a selector with no dictionary"); + if (isWhitelist) { + return filterWhiteList(selector); + } else { + return filterBlackList(selector); } + } - // Upper bound on cardinality of the filtered spec. - final int cardinality = isWhitelist ? values.size() : selectorCardinality; - + private DimensionSelector filterWhiteList(DimensionSelector selector) + { + final int selectorCardinality = selector.getValueCardinality(); + if (selectorCardinality < 0 || (selector.idLookup() == null && !selector.nameLookupPossibleInAdvance())) { + return new PredicateFilteredDimensionSelector(selector, Predicates.in(values)); + } + final int maxPossibleFilteredCardinality = values.size(); int count = 0; - final Map forwardMapping = new HashMap<>(cardinality); - final int[] reverseMapping = new int[cardinality]; - - if (isWhitelist) { + final Int2IntMap forwardMapping = new Int2IntOpenHashMap(maxPossibleFilteredCardinality); + forwardMapping.defaultReturnValue(-1); + final int[] reverseMapping = new int[maxPossibleFilteredCardinality]; + IdLookup idLookup = selector.idLookup(); + if (idLookup != null) { for (String value : values) { - int i = selector.lookupId(value); + int i = idLookup.lookupId(value); if (i >= 0) { forwardMapping.put(i, count); reverseMapping[count++] = i; @@ -96,14 +107,43 @@ public DimensionSelector decorate(final DimensionSelector selector) } } else { for (int i = 0; i < selectorCardinality; i++) { - if (!values.contains(Strings.nullToEmpty(selector.lookupName(i)))) { + if (values.contains(Strings.nullToEmpty(selector.lookupName(i)))) { forwardMapping.put(i, count); reverseMapping[count++] = i; } } } + return new ForwardingFilteredDimensionSelector(selector, forwardMapping, reverseMapping); + } - return BaseFilteredDimensionSpec.decorate(selector, forwardMapping, reverseMapping); + private DimensionSelector filterBlackList(DimensionSelector selector) + { + final int selectorCardinality = selector.getValueCardinality(); + if (selectorCardinality < 0 || !selector.nameLookupPossibleInAdvance()) { + return new PredicateFilteredDimensionSelector( + selector, + new Predicate() + { + @Override + public boolean apply(@Nullable String input) + { + return !values.contains(input); + } + } + ); + } + final int maxPossibleFilteredCardinality = selectorCardinality; + int count = 0; + final Int2IntMap forwardMapping = new Int2IntOpenHashMap(maxPossibleFilteredCardinality); + forwardMapping.defaultReturnValue(-1); + final int[] reverseMapping = new int[maxPossibleFilteredCardinality]; + for (int i = 0; i < selectorCardinality; i++) { + if (!values.contains(Strings.nullToEmpty(selector.lookupName(i)))) { + forwardMapping.put(i, count); + reverseMapping[count++] = i; + } + } + return new ForwardingFilteredDimensionSelector(selector, forwardMapping, reverseMapping); } @Override diff --git a/processing/src/main/java/io/druid/query/dimension/PredicateFilteredDimensionSelector.java b/processing/src/main/java/io/druid/query/dimension/PredicateFilteredDimensionSelector.java new file mode 100644 index 000000000000..999ba10621b7 --- /dev/null +++ b/processing/src/main/java/io/druid/query/dimension/PredicateFilteredDimensionSelector.java @@ -0,0 +1,134 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets 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 io.druid.query.dimension; + +import com.google.common.base.Predicate; +import io.druid.query.filter.ValueMatcher; +import io.druid.segment.DimensionSelector; +import io.druid.segment.IdLookup; +import io.druid.segment.data.ArrayBasedIndexedInts; +import io.druid.segment.data.IndexedInts; + +import javax.annotation.Nullable; +import java.util.Objects; + +final class PredicateFilteredDimensionSelector implements DimensionSelector +{ + private final DimensionSelector selector; + private final Predicate predicate; + + PredicateFilteredDimensionSelector(DimensionSelector selector, Predicate predicate) + { + this.selector = selector; + this.predicate = predicate; + } + + @Override + public IndexedInts getRow() + { + IndexedInts baseRow = selector.getRow(); + int baseRowSize = baseRow.size(); + int[] result = new int[baseRowSize]; + int resultSize = 0; + for (int i = 0; i < baseRowSize; i++) { + if (predicate.apply(selector.lookupName(baseRow.get(i)))) { + result[resultSize++] = i; + } + } + return new ArrayBasedIndexedInts(result, resultSize); + } + + @Override + public ValueMatcher makeValueMatcher(final String value, final boolean matchNull) + { + return new ValueMatcher() + { + @Override + public boolean matches() + { + final IndexedInts baseRow = selector.getRow(); + final int baseRowSize = baseRow.size(); + boolean nullRow = true; + for (int i = 0; i < baseRowSize; i++) { + String rowValue = lookupName(baseRow.get(i)); + if (predicate.apply(rowValue)) { + if (Objects.equals(rowValue, value)) { + return true; + } + nullRow = false; + } + } + // null should match empty rows in multi-value columns + return nullRow && matchNull; + } + }; + } + + @Override + public ValueMatcher makeValueMatcher(final Predicate matcherPredicate, final boolean matchNull) + { + return new ValueMatcher() + { + @Override + public boolean matches() + { + final IndexedInts baseRow = selector.getRow(); + final int baseRowSize = baseRow.size(); + boolean nullRow = true; + for (int i = 0; i < baseRowSize; ++i) { + String rowValue = lookupName(baseRow.get(i)); + if (predicate.apply(rowValue)) { + if (matcherPredicate.apply(rowValue)) { + return true; + } + nullRow = false; + } + } + // null should match empty rows in multi-value columns + return nullRow && matchNull; + } + }; + } + + @Override + public int getValueCardinality() + { + return selector.getValueCardinality(); + } + + @Override + public String lookupName(int id) + { + return selector.lookupName(id); + } + + @Override + public boolean nameLookupPossibleInAdvance() + { + return selector.nameLookupPossibleInAdvance(); + } + + @Nullable + @Override + public IdLookup idLookup() + { + return selector.idLookup(); + } +} diff --git a/processing/src/main/java/io/druid/query/dimension/RegexFilteredDimensionSpec.java b/processing/src/main/java/io/druid/query/dimension/RegexFilteredDimensionSpec.java index df71d959eb12..3cfd7361488a 100644 --- a/processing/src/main/java/io/druid/query/dimension/RegexFilteredDimensionSpec.java +++ b/processing/src/main/java/io/druid/query/dimension/RegexFilteredDimensionSpec.java @@ -21,14 +21,16 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Preconditions; +import com.google.common.base.Predicate; import com.google.common.base.Strings; import io.druid.java.util.common.StringUtils; import io.druid.query.filter.DimFilterUtils; import io.druid.segment.DimensionSelector; +import it.unimi.dsi.fastutil.ints.Int2IntMap; +import it.unimi.dsi.fastutil.ints.Int2IntOpenHashMap; +import javax.annotation.Nullable; import java.nio.ByteBuffer; -import java.util.HashMap; -import java.util.Map; import java.util.regex.Pattern; /** @@ -62,17 +64,27 @@ public String getPattern() public DimensionSelector decorate(final DimensionSelector selector) { if (selector == null) { - return selector; + return null; } - int count = 0; - final Map forwardMapping = new HashMap<>(); - final int selectorCardinality = selector.getValueCardinality(); - if (selectorCardinality < 0) { - throw new UnsupportedOperationException("Cannot decorate a selector with no dictionary"); + if (selectorCardinality < 0 || !selector.nameLookupPossibleInAdvance()) { + return new PredicateFilteredDimensionSelector( + selector, + new Predicate() + { + @Override + public boolean apply(@Nullable String input) + { + return compiledRegex.matcher(Strings.nullToEmpty(input)).matches(); + } + } + ); } + int count = 0; + final Int2IntMap forwardMapping = new Int2IntOpenHashMap(); + forwardMapping.defaultReturnValue(-1); for (int i = 0; i < selectorCardinality; i++) { if (compiledRegex.matcher(Strings.nullToEmpty(selector.lookupName(i))).matches()) { forwardMapping.put(i, count++); @@ -80,10 +92,10 @@ public DimensionSelector decorate(final DimensionSelector selector) } final int[] reverseMapping = new int[forwardMapping.size()]; - for (Map.Entry e : forwardMapping.entrySet()) { - reverseMapping[e.getValue().intValue()] = e.getKey().intValue(); + for (Int2IntMap.Entry e : forwardMapping.int2IntEntrySet()) { + reverseMapping[e.getIntValue()] = e.getIntKey(); } - return BaseFilteredDimensionSpec.decorate(selector, forwardMapping, reverseMapping); + return new ForwardingFilteredDimensionSelector(selector, forwardMapping, reverseMapping); } @Override diff --git a/processing/src/main/java/io/druid/query/filter/StringValueMatcherColumnSelectorStrategy.java b/processing/src/main/java/io/druid/query/filter/StringValueMatcherColumnSelectorStrategy.java index 88b21b314aa2..e4a8fa23bee9 100644 --- a/processing/src/main/java/io/druid/query/filter/StringValueMatcherColumnSelectorStrategy.java +++ b/processing/src/main/java/io/druid/query/filter/StringValueMatcherColumnSelectorStrategy.java @@ -22,12 +22,8 @@ import com.google.common.base.Predicate; import com.google.common.base.Strings; import io.druid.segment.DimensionSelector; -import io.druid.segment.data.IndexedInts; import io.druid.segment.filter.BooleanValueMatcher; -import java.util.BitSet; -import java.util.Objects; - public class StringValueMatcherColumnSelectorStrategy implements ValueMatcherColumnSelectorStrategy { @Override @@ -42,53 +38,9 @@ public ValueMatcher makeValueMatcher(final DimensionSelector selector, final Str if (cardinality == 0 || (cardinality == 1 && selector.lookupName(0) == null)) { // All values are null or empty rows (which match nulls anyway). No need to check each row. - return new BooleanValueMatcher(matchNull); - } else if (cardinality >= 0) { - // Dictionary-encoded dimension. Compare by id instead of by value to save time. - final int valueId = selector.lookupId(valueStr); - - return new ValueMatcher() - { - @Override - public boolean matches() - { - final IndexedInts row = selector.getRow(); - final int size = row.size(); - if (size == 0) { - // null should match empty rows in multi-value columns - return matchNull; - } else { - for (int i = 0; i < size; ++i) { - if (row.get(i) == valueId) { - return true; - } - } - return false; - } - } - }; + return BooleanValueMatcher.of(matchNull); } else { - // Not dictionary-encoded. Skip the optimization. - return new ValueMatcher() - { - @Override - public boolean matches() - { - final IndexedInts row = selector.getRow(); - final int size = row.size(); - if (size == 0) { - // null should match empty rows in multi-value columns - return matchNull; - } else { - for (int i = 0; i < size; ++i) { - if (Objects.equals(selector.lookupName(row.get(i)), valueStr)) { - return true; - } - } - return false; - } - } - }; + return selector.makeValueMatcher(valueStr, matchNull); } } @@ -104,58 +56,10 @@ public ValueMatcher makeValueMatcher( if (cardinality == 0 || (cardinality == 1 && selector.lookupName(0) == null)) { // All values are null or empty rows (which match nulls anyway). No need to check each row. - return new BooleanValueMatcher(matchNull); - } else if (cardinality >= 0) { - // Dictionary-encoded dimension. Check every value; build a bitset of matching ids. - final BitSet valueIds = new BitSet(cardinality); - for (int i = 0; i < cardinality; i++) { - if (predicate.apply(selector.lookupName(i))) { - valueIds.set(i); - } - } - - return new ValueMatcher() - { - @Override - public boolean matches() - { - final IndexedInts row = selector.getRow(); - final int size = row.size(); - if (size == 0) { - // null should match empty rows in multi-value columns - return matchNull; - } else { - for (int i = 0; i < size; ++i) { - if (valueIds.get(row.get(i))) { - return true; - } - } - return false; - } - } - }; + return BooleanValueMatcher.of(matchNull); } else { - // Not dictionary-encoded. Skip the optimization. - return new ValueMatcher() - { - @Override - public boolean matches() - { - final IndexedInts row = selector.getRow(); - final int size = row.size(); - if (size == 0) { - // null should match empty rows in multi-value columns - return matchNull; - } else { - for (int i = 0; i < size; ++i) { - if (predicate.apply(selector.lookupName(row.get(i)))) { - return true; - } - } - return false; - } - } - }; + return selector.makeValueMatcher(predicate, matchNull); } } + } diff --git a/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java b/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java index c13f9f2c8669..e2e7f2a72958 100644 --- a/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java +++ b/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java @@ -19,15 +19,18 @@ package io.druid.query.groupby; +import com.google.common.base.Predicate; import com.google.common.base.Strings; import com.google.common.base.Supplier; import com.google.common.collect.ImmutableMap; import io.druid.data.input.Row; import io.druid.query.dimension.DimensionSpec; import io.druid.query.extraction.ExtractionFn; +import io.druid.query.filter.ValueMatcher; import io.druid.segment.ColumnSelectorFactory; import io.druid.segment.DimensionSelector; import io.druid.segment.FloatColumnSelector; +import io.druid.segment.IdLookup; import io.druid.segment.LongColumnSelector; import io.druid.segment.ObjectColumnSelector; import io.druid.segment.column.Column; @@ -41,6 +44,7 @@ import javax.annotation.Nullable; import java.util.List; import java.util.Map; +import java.util.Objects; public class RowBasedColumnSelectorFactory implements ColumnSelectorFactory { @@ -108,6 +112,36 @@ public IndexedInts getRow() return ZeroIndexedInts.instance(); } + @Override + public ValueMatcher makeValueMatcher(final String value, boolean matchNull) + { + return new ValueMatcher() + { + @Override + public boolean matches() + { + String rowValue = extractionFn.apply(row.get().getTimestampFromEpoch()); + return Objects.equals(rowValue, value); + } + }; + } + + @Override + public ValueMatcher makeValueMatcher( + final Predicate predicate, boolean matchNull + ) + { + return new ValueMatcher() + { + @Override + public boolean matches() + { + String rowValue = extractionFn.apply(row.get().getTimestampFromEpoch()); + return predicate.apply(rowValue); + } + }; + } + @Override public int getValueCardinality() { @@ -121,9 +155,16 @@ public String lookupName(int id) } @Override - public int lookupId(String name) + public boolean nameLookupPossibleInAdvance() + { + return false; + } + + @Nullable + @Override + public IdLookup idLookup() { - throw new UnsupportedOperationException("lookupId"); + return null; } }; } else { @@ -136,6 +177,94 @@ public IndexedInts getRow() return RangeIndexedInts.create(dimensionValues != null ? dimensionValues.size() : 0); } + @Override + public ValueMatcher makeValueMatcher(final String value, final boolean matchNull) + { + if (extractionFn == null) { + return new ValueMatcher() + { + @Override + public boolean matches() + { + final List dimensionValues = row.get().getDimension(dimension); + if (dimensionValues == null || dimensionValues.isEmpty()) { + return matchNull; + } + + for (String dimensionValue : dimensionValues) { + if (Objects.equals(Strings.emptyToNull(dimensionValue), value)) { + return true; + } + } + return false; + } + }; + } else { + return new ValueMatcher() + { + @Override + public boolean matches() + { + final List dimensionValues = row.get().getDimension(dimension); + if (dimensionValues == null || dimensionValues.isEmpty()) { + return matchNull; + } + + for (String dimensionValue : dimensionValues) { + if (Objects.equals(extractionFn.apply(Strings.emptyToNull(dimensionValue)), value)) { + return true; + } + } + return false; + } + }; + } + } + + @Override + public ValueMatcher makeValueMatcher(final Predicate predicate, final boolean matchNull) + { + if (extractionFn == null) { + return new ValueMatcher() + { + @Override + public boolean matches() + { + final List dimensionValues = row.get().getDimension(dimension); + if (dimensionValues == null || dimensionValues.isEmpty()) { + return matchNull; + } + + for (String dimensionValue : dimensionValues) { + if (predicate.apply(Strings.emptyToNull(dimensionValue))) { + return true; + } + } + return false; + } + }; + } else { + return new ValueMatcher() + { + @Override + public boolean matches() + { + final List dimensionValues = row.get().getDimension(dimension); + if (dimensionValues == null || dimensionValues.isEmpty()) { + return matchNull; + } + + for (String dimensionValue : dimensionValues) { + if (predicate.apply(extractionFn.apply(Strings.emptyToNull(dimensionValue)))) { + return true; + } + } + return false; + } + }; + } + } + @Override public int getValueCardinality() { @@ -150,9 +279,16 @@ public String lookupName(int id) } @Override - public int lookupId(String name) + public boolean nameLookupPossibleInAdvance() + { + return false; + } + + @Nullable + @Override + public IdLookup idLookup() { - throw new UnsupportedOperationException("lookupId"); + return null; } }; } diff --git a/processing/src/main/java/io/druid/query/groupby/epinephelinae/GroupByRowProcessor.java b/processing/src/main/java/io/druid/query/groupby/epinephelinae/GroupByRowProcessor.java index c1c467923414..3eab688f2e70 100644 --- a/processing/src/main/java/io/druid/query/groupby/epinephelinae/GroupByRowProcessor.java +++ b/processing/src/main/java/io/druid/query/groupby/epinephelinae/GroupByRowProcessor.java @@ -97,7 +97,7 @@ public static Sequence process( rowSignature ); final ValueMatcher filterMatcher = filter == null - ? new BooleanValueMatcher(true) + ? BooleanValueMatcher.of(true) : filter.makeMatcher(columnSelectorFactory); final FilteredSequence filteredSequence = new FilteredSequence<>( diff --git a/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java b/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java index c32eb78e1f27..62871a221516 100644 --- a/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java +++ b/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java @@ -26,6 +26,7 @@ import io.druid.segment.Capabilities; import io.druid.segment.Cursor; import io.druid.segment.DimensionSelector; +import io.druid.segment.IdLookup; import java.util.Arrays; import java.util.Comparator; @@ -179,6 +180,7 @@ protected static abstract class BaseArrayProvider implements TopNMetricSpecBu private volatile int keepOnlyN; private final DimensionSelector dimSelector; + private final IdLookup idLookup; private final TopNQuery query; private final Capabilities capabilities; @@ -189,6 +191,7 @@ public BaseArrayProvider( ) { this.dimSelector = dimSelector; + this.idLookup = dimSelector.idLookup(); this.query = query; this.capabilities = capabilities; @@ -232,8 +235,8 @@ protected Pair computeStartEnd(int cardinality) { int startIndex = ignoreFirstN; - if (previousStop != null) { - int lookupId = dimSelector.lookupId(previousStop) + 1; + if (previousStop != null && idLookup != null) { + int lookupId = idLookup.lookupId(previousStop) + 1; if (lookupId < 0) { lookupId *= -1; } diff --git a/processing/src/main/java/io/druid/segment/DimensionSelector.java b/processing/src/main/java/io/druid/segment/DimensionSelector.java index 5bd2c2cec1c7..cca9681844ae 100644 --- a/processing/src/main/java/io/druid/segment/DimensionSelector.java +++ b/processing/src/main/java/io/druid/segment/DimensionSelector.java @@ -17,7 +17,13 @@ * under the License. */ -package io.druid.segment;import io.druid.segment.data.IndexedInts; +package io.druid.segment; + +import com.google.common.base.Predicate; +import io.druid.query.filter.ValueMatcher; +import io.druid.segment.data.IndexedInts; + +import javax.annotation.Nullable; /** */ @@ -34,6 +40,10 @@ public interface DimensionSelector extends ColumnValueSelector */ public IndexedInts getRow(); + ValueMatcher makeValueMatcher(String value, boolean matchNull); + + ValueMatcher makeValueMatcher(Predicate predicate, boolean matchNull); + /** * Value cardinality is the cardinality of the different occurring values. If there were 4 rows: * @@ -80,10 +90,19 @@ public interface DimensionSelector extends ColumnValueSelector public String lookupName(int id); /** - * The ID is the int id value of the field. + * Returns true if it is possible to {@link #lookupName(int)} by ids from 0 to {@link #getValueCardinality()} + * Before the rows with those ids are returns. * - * @param name field name to look up the id for - * @return the id for the given field name + *

Returns false if {@link #lookupName(int)} could be called only with ids, returned from the last call of {@link + * #getRow()} on this DimensionSelector, but not earlier and not later. If {@link #lookupName(int)} is called with an + * id, which is not present in the last {@link IndexedInts}, returned from {@link #getRow()}, result is undefined: + * exception could be thrown, or null returned, or some other random value. + */ + boolean nameLookupPossibleInAdvance(); + + /** + * Returns {@link IdLookup} if available for this DimensionSelector, or null. */ - public int lookupId(String name); + @Nullable + IdLookup idLookup(); } diff --git a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java new file mode 100644 index 000000000000..4c2f11454925 --- /dev/null +++ b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java @@ -0,0 +1,210 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets 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 io.druid.segment; + +import com.google.common.base.Predicate; +import io.druid.java.util.common.IAE; +import io.druid.query.filter.ValueMatcher; +import io.druid.segment.data.IndexedInts; +import io.druid.segment.filter.BooleanValueMatcher; + +import java.util.BitSet; +import java.util.Objects; + +public final class DimensionSelectorUtils +{ + + private DimensionSelectorUtils() + { + } + + public static ValueMatcher makeRowBasedValueMatcher( + DimensionSelector selector, + String value, + boolean matchNull + ) + { + IdLookup idLookup = selector.idLookup(); + if (idLookup != null) { + return makeDictionaryEncodedRowBasedValueMatcher(selector, idLookup.lookupId(value), matchNull); + } else { + return makeNonDictionaryEncodedIndexedIntsBasedValueMatcher(selector, value, matchNull); + } + } + + private static ValueMatcher makeDictionaryEncodedRowBasedValueMatcher( + final DimensionSelector selector, + final int valueId, + final boolean matchNull + ) + { + if (valueId >= 0) { + return new ValueMatcher() + { + @Override + public boolean matches() + { + final IndexedInts row = selector.getRow(); + final int size = row.size(); + if (size == 0) { + // null should match empty rows in multi-value columns + return matchNull; + } else { + for (int i = 0; i < size; ++i) { + if (row.get(i) == valueId) { + return true; + } + } + return false; + } + } + }; + } else { + if (matchNull) { + return new ValueMatcher() + { + @Override + public boolean matches() + { + final IndexedInts row = selector.getRow(); + final int size = row.size(); + return size == 0; + } + }; + } else { + return BooleanValueMatcher.of(false); + } + } + } + + private static ValueMatcher makeNonDictionaryEncodedIndexedIntsBasedValueMatcher( + final DimensionSelector selector, + final String value, + final boolean matchNull + ) + { + return new ValueMatcher() + { + @Override + public boolean matches() + { + final IndexedInts row = selector.getRow(); + final int size = row.size(); + if (size == 0) { + // null should match empty rows in multi-value columns + return matchNull; + } else { + for (int i = 0; i < size; ++i) { + if (Objects.equals(selector.lookupName(row.get(i)), value)) { + return true; + } + } + return false; + } + } + }; + } + + public static ValueMatcher makeRowBasedValueMatcher( + final DimensionSelector selector, + final Predicate predicate, + final boolean matchNull + ) + { + int cardinality = selector.getValueCardinality(); + if (cardinality >= 0) { + return makeDictionaryEncodedRowBasedValueMatcher(selector, predicate, matchNull); + } else { + return makeNonDictionaryEncodedRowBasedValueMatcher(selector, predicate, matchNull); + } + } + + private static ValueMatcher makeDictionaryEncodedRowBasedValueMatcher( + final DimensionSelector selector, + Predicate predicate, + final boolean matchNull + ) + { + final BitSet predicateMatchingValueIds = makePredicateMatchingSet(selector, predicate); + return new ValueMatcher() + { + @Override + public boolean matches() + { + final IndexedInts row = selector.getRow(); + final int size = row.size(); + if (size == 0) { + // null should match empty rows in multi-value columns + return matchNull; + } else { + for (int i = 0; i < size; ++i) { + if (predicateMatchingValueIds.get(row.get(i))) { + return true; + } + } + return false; + } + } + }; + } + + private static ValueMatcher makeNonDictionaryEncodedRowBasedValueMatcher( + final DimensionSelector selector, + final Predicate predicate, + final boolean matchNull + ) + { + return new ValueMatcher() + { + @Override + public boolean matches() + { + final IndexedInts row = selector.getRow(); + final int size = row.size(); + if (size == 0) { + // null should match empty rows in multi-value columns + return matchNull; + } else { + for (int i = 0; i < size; ++i) { + if (predicate.apply(selector.lookupName(row.get(i)))) { + return true; + } + } + return false; + } + } + }; + } + + public static BitSet makePredicateMatchingSet(DimensionSelector selector, Predicate predicate) + { + if (!selector.nameLookupPossibleInAdvance()) { + throw new IAE("selector.nameLookupPossibleInAdvance() should return true"); + } + int cardinality = selector.getValueCardinality(); + BitSet valueIds = new BitSet(cardinality); + for (int i = 0; i < cardinality; i++) { + if (predicate.apply(selector.lookupName(i))) { + valueIds.set(i); + } + } + return valueIds; + } +} diff --git a/processing/src/main/java/io/druid/segment/IdLookup.java b/processing/src/main/java/io/druid/segment/IdLookup.java new file mode 100644 index 000000000000..52a5e16e73ab --- /dev/null +++ b/processing/src/main/java/io/druid/segment/IdLookup.java @@ -0,0 +1,31 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets 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 io.druid.segment; + +/** + * "Mixin" for {@link DimensionSelector}. + */ +public interface IdLookup +{ + /** + * Inverse of {@link DimensionSelector#lookupName(int)}. + */ + int lookupId(String name); +} diff --git a/processing/src/main/java/io/druid/segment/NullDimensionSelector.java b/processing/src/main/java/io/druid/segment/NullDimensionSelector.java index f5cbe064686e..e78b2f272d0a 100644 --- a/processing/src/main/java/io/druid/segment/NullDimensionSelector.java +++ b/processing/src/main/java/io/druid/segment/NullDimensionSelector.java @@ -19,11 +19,16 @@ package io.druid.segment; +import com.google.common.base.Predicate; import com.google.common.base.Strings; +import io.druid.query.filter.ValueMatcher; import io.druid.segment.data.IndexedInts; import io.druid.segment.data.ZeroIndexedInts; +import io.druid.segment.filter.BooleanValueMatcher; -public class NullDimensionSelector implements DimensionSelector +import javax.annotation.Nullable; + +public class NullDimensionSelector implements DimensionSelector, IdLookup { @Override public IndexedInts getRow() @@ -31,6 +36,18 @@ public IndexedInts getRow() return ZeroIndexedInts.instance(); } + @Override + public ValueMatcher makeValueMatcher(String value, boolean matchNull) + { + return BooleanValueMatcher.of(value == null); + } + + @Override + public ValueMatcher makeValueMatcher(Predicate predicate, boolean matchNull) + { + return BooleanValueMatcher.of(predicate.apply(null)); + } + @Override public int getValueCardinality() { @@ -40,9 +57,23 @@ public int getValueCardinality() @Override public String lookupName(int id) { + assert id == 0 : "id = " + id; return null; } + @Override + public boolean nameLookupPossibleInAdvance() + { + return true; + } + + @Nullable + @Override + public IdLookup idLookup() + { + return this; + } + @Override public int lookupId(String name) { diff --git a/processing/src/main/java/io/druid/segment/NullStringObjectColumnSelector.java b/processing/src/main/java/io/druid/segment/NullStringObjectColumnSelector.java new file mode 100644 index 000000000000..63c92d988d9c --- /dev/null +++ b/processing/src/main/java/io/druid/segment/NullStringObjectColumnSelector.java @@ -0,0 +1,46 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets 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 io.druid.segment; + +public final class NullStringObjectColumnSelector implements ObjectColumnSelector +{ + private static final NullStringObjectColumnSelector INSTANCE = new NullStringObjectColumnSelector(); + + public static NullStringObjectColumnSelector instance() + { + return INSTANCE; + } + + private NullStringObjectColumnSelector() + { + } + + @Override + public Class classOfObject() + { + return String.class; + } + + @Override + public String get() + { + return null; + } +} diff --git a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java index 4750ba771dad..ea21705cb755 100644 --- a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java @@ -20,6 +20,7 @@ package io.druid.segment; import com.google.common.base.Function; +import com.google.common.base.Predicate; import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; @@ -59,8 +60,10 @@ import java.io.Closeable; import java.io.IOException; import java.util.ArrayList; +import java.util.BitSet; import java.util.List; import java.util.Map; +import java.util.Objects; /** */ @@ -463,7 +466,7 @@ private DimensionSelector makeDimensionSelectorUndecorated( if (column == null) { return NULL_DIMENSION_SELECTOR; } else if (columnDesc.getCapabilities().hasMultipleValues()) { - return new DimensionSelector() + class MultiValueDimensionSelector implements DimensionSelector, IdLookup { @Override public IndexedInts getRow() @@ -471,6 +474,18 @@ public IndexedInts getRow() return column.getMultiValueRow(cursorOffset.getOffset()); } + @Override + public ValueMatcher makeValueMatcher(String value, boolean matchNull) + { + return DimensionSelectorUtils.makeRowBasedValueMatcher(this, value, matchNull); + } + + @Override + public ValueMatcher makeValueMatcher(Predicate predicate, boolean matchNull) + { + return DimensionSelectorUtils.makeRowBasedValueMatcher(this, predicate, matchNull); + } + @Override public int getValueCardinality() { @@ -486,6 +501,19 @@ public String lookupName(int id) extractionFn.apply(value); } + @Override + public boolean nameLookupPossibleInAdvance() + { + return true; + } + + @Nullable + @Override + public IdLookup idLookup() + { + return extractionFn == null ? this : null; + } + @Override public int lookupId(String name) { @@ -496,9 +524,10 @@ public int lookupId(String name) } return column.lookupId(name); } - }; + } + return new MultiValueDimensionSelector(); } else { - return new DimensionSelector() + class SingleValueDimensionSelector implements DimensionSelector, IdLookup { @Override public IndexedInts getRow() @@ -538,6 +567,54 @@ public void close() throws IOException }; } + @Override + public ValueMatcher makeValueMatcher(final String value, boolean matchNull) + { + if (extractionFn == null) { + final int valueId = lookupId(value); + if (valueId >= 0) { + return new ValueMatcher() + { + @Override + public boolean matches() + { + return column.getSingleValueRow(cursorOffset.getOffset()) == valueId; + } + }; + } else { + return BooleanValueMatcher.of(false); + } + } else { + return new ValueMatcher() + { + @Override + public boolean matches() + { + String rowValue = lookupName(column.getSingleValueRow(cursorOffset.getOffset())); + return Objects.equals(rowValue, value); + } + }; + } + } + + @Override + public ValueMatcher makeValueMatcher(final Predicate predicate, boolean matchNull) + { + final BitSet predicateMatchingValueIds = DimensionSelectorUtils.makePredicateMatchingSet( + this, + predicate + ); + return new ValueMatcher() + { + @Override + public boolean matches() + { + int rowValueId = column.getSingleValueRow(cursorOffset.getOffset()); + return predicateMatchingValueIds.get(rowValueId); + } + }; + } + @Override public int getValueCardinality() { @@ -551,6 +628,19 @@ public String lookupName(int id) return extractionFn == null ? value : extractionFn.apply(value); } + @Override + public boolean nameLookupPossibleInAdvance() + { + return true; + } + + @Nullable + @Override + public IdLookup idLookup() + { + return extractionFn == null ? this : null; + } + @Override public int lookupId(String name) { @@ -561,7 +651,8 @@ public int lookupId(String name) } return column.lookupId(name); } - }; + } + return new SingleValueDimensionSelector(); } } @@ -992,7 +1083,7 @@ public ValueMatcher makeRowOffsetMatcher(final ImmutableBitmap rowBitmap) rowBitmap.iterator(); if (!iter.hasNext()) { - return new BooleanValueMatcher(false); + return BooleanValueMatcher.of(false); } if (descending) { diff --git a/processing/src/main/java/io/druid/segment/SingleScanTimeDimSelector.java b/processing/src/main/java/io/druid/segment/SingleScanTimeDimSelector.java index fe7c06e5baf4..74786ee38a8a 100644 --- a/processing/src/main/java/io/druid/segment/SingleScanTimeDimSelector.java +++ b/processing/src/main/java/io/druid/segment/SingleScanTimeDimSelector.java @@ -19,14 +19,15 @@ package io.druid.segment; -import com.google.common.collect.Maps; +import com.google.common.base.Predicate; import io.druid.query.extraction.ExtractionFn; +import io.druid.query.filter.ValueMatcher; import io.druid.segment.data.IndexedInts; -import it.unimi.dsi.fastutil.ints.IntIterator; -import it.unimi.dsi.fastutil.ints.IntIterators; +import io.druid.segment.data.SingleIndexedInt; -import java.io.IOException; -import java.util.Map; +import javax.annotation.Nullable; +import java.util.ArrayList; +import java.util.List; import java.util.Objects; public class SingleScanTimeDimSelector implements DimensionSelector @@ -35,7 +36,7 @@ public class SingleScanTimeDimSelector implements DimensionSelector private final LongColumnSelector selector; private final boolean descending; - private final Map timeValues = Maps.newHashMap(); + private final List timeValues = new ArrayList<>(); private String currentValue = null; private long currentTimestamp = Long.MIN_VALUE; private int index = -1; @@ -58,6 +59,37 @@ public SingleScanTimeDimSelector(LongColumnSelector selector, ExtractionFn extra @Override public IndexedInts getRow() + { + return new SingleIndexedInt(getDimensionValueIndex()); + } + + @Override + public ValueMatcher makeValueMatcher(final String value, boolean matchNull) + { + return new ValueMatcher() + { + @Override + public boolean matches() + { + return Objects.equals(lookupName(getDimensionValueIndex()), value); + } + }; + } + + @Override + public ValueMatcher makeValueMatcher(final Predicate predicate, boolean matchNull) + { + return new ValueMatcher() + { + @Override + public boolean matches() + { + return predicate.apply(lookupName(getDimensionValueIndex())); + } + }; + } + + private int getDimensionValueIndex() { // if this the first timestamp, apply and cache extraction function result final long timestamp = selector.get(); @@ -65,7 +97,7 @@ public IndexedInts getRow() currentTimestamp = timestamp; currentValue = extractionFn.apply(timestamp); ++index; - timeValues.put(index, currentValue); + timeValues.add(currentValue); } // if this is a new timestamp, apply and cache extraction function result // since timestamps are assumed grouped and scanned once, we only need to @@ -85,7 +117,7 @@ else if (timestamp != currentTimestamp) { if (!Objects.equals(value, currentValue)) { currentValue = value; ++index; - timeValues.put(index, currentValue); + timeValues.add(currentValue); } // Note: this could be further optimized by checking if the new value is one we have // previously seen, but would require keeping track of both the current and the maximum index @@ -93,39 +125,7 @@ else if (timestamp != currentTimestamp) { // otherwise, if the current timestamp is the same as the previous timestamp, // keep using the same dimension value index - final int dimensionValueIndex = index; - return new IndexedInts() - { - @Override - public int size() - { - return 1; - } - - @Override - public int get(int i) - { - return dimensionValueIndex; - } - - @Override - public IntIterator iterator() - { - return IntIterators.singleton(dimensionValueIndex); - } - - @Override - public void fill(int index, int[] toFill) - { - throw new UnsupportedOperationException("fill not supported"); - } - - @Override - public void close() throws IOException - { - - } - }; + return index; } @Override @@ -145,8 +145,15 @@ public String lookupName(int id) } @Override - public int lookupId(String name) + public boolean nameLookupPossibleInAdvance() + { + return false; + } + + @Nullable + @Override + public IdLookup idLookup() { - throw new UnsupportedOperationException("time column does not support lookups"); + return null; } } diff --git a/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java b/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java index ae450b066f43..3bdce30f2057 100644 --- a/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java +++ b/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java @@ -20,6 +20,7 @@ package io.druid.segment; import com.google.common.base.Function; +import com.google.common.base.Predicate; import com.google.common.base.Strings; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -29,23 +30,25 @@ import io.druid.data.input.impl.DimensionSchema.MultiValueHandling; import io.druid.query.dimension.DimensionSpec; import io.druid.query.extraction.ExtractionFn; +import io.druid.query.filter.ValueMatcher; +import io.druid.segment.data.ArrayBasedIndexedInts; import io.druid.segment.data.Indexed; import io.druid.segment.data.IndexedInts; import io.druid.segment.data.IndexedIterable; +import io.druid.segment.filter.BooleanValueMatcher; import io.druid.segment.incremental.IncrementalIndex; import io.druid.segment.incremental.IncrementalIndexStorageAdapter; -import it.unimi.dsi.fastutil.ints.IntArrayList; -import it.unimi.dsi.fastutil.ints.IntIterator; -import it.unimi.dsi.fastutil.ints.IntList; -import it.unimi.dsi.fastutil.ints.IntLists; +import it.unimi.dsi.fastutil.ints.IntArrays; -import java.io.IOException; +import javax.annotation.Nullable; import java.util.ArrayList; import java.util.Arrays; +import java.util.BitSet; import java.util.Comparator; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; public class StringDimensionIndexer implements DimensionIndexer { @@ -385,7 +388,7 @@ public DimensionSelector makeColumnValueSelector( final int dimIndex = desc.getIndex(); final int maxId = getCardinality(); - return new DimensionSelector() + class IndexerDimensionSelector implements DimensionSelector, IdLookup { @Override public IndexedInts getRow() @@ -399,59 +402,118 @@ public IndexedInts getRow() indices = null; } - IntList valsTmp = null; + int[] row = null; + int rowSize = 0; if (indices == null || indices.length == 0) { final int nullId = getEncodedValue(null, false); if (nullId > -1) { if (nullId < maxId) { - valsTmp = IntLists.singleton(nullId); + row = new int[] {nullId}; + rowSize = 1; } else { - valsTmp = IntLists.EMPTY_LIST; + // Choose to use ArrayBasedIndexedInts later, instead of EmptyIndexedInts, for monomorphism + row = IntArrays.EMPTY_ARRAY; + rowSize = 0; } } } - if (valsTmp == null && indices != null && indices.length > 0) { - valsTmp = new IntArrayList(indices.length); - for (int i = 0; i < indices.length; i++) { - int id = indices[i]; + if (row == null && indices != null && indices.length > 0) { + row = new int[indices.length]; + for (int id : indices) { if (id < maxId) { - valsTmp.add(id); + row[rowSize++] = id; } } } - final IntList vals = valsTmp == null ? IntLists.EMPTY_LIST : valsTmp; - return new IndexedInts() - { - @Override - public int size() - { - return vals.size(); - } + return rowSize > 0 ? new ArrayBasedIndexedInts(row, rowSize) : ArrayBasedIndexedInts.empty(); + } - @Override - public int get(int index) - { - return vals.get(index); + @Override + public ValueMatcher makeValueMatcher(final String value, final boolean matchNull) + { + if (extractionFn == null) { + final int valueId = lookupId(value); + if (valueId >= 0 || matchNull) { + return new ValueMatcher() + { + @Override + public boolean matches() + { + Object[] dims = currEntry.getKey().getDims(); + if (dimIndex >= dims.length) { + return matchNull; + } + + int[] dimsInt = (int[]) dims[dimIndex]; + if (dimsInt == null || dimsInt.length == 0) { + return matchNull; + } + + for (int id : dimsInt) { + if (id == valueId) { + return true; + } + } + return false; + } + }; + } else { + return BooleanValueMatcher.of(false); } - - @Override - public IntIterator iterator() + } else { + return new ValueMatcher() { - return vals.iterator(); - } + @Override + public boolean matches() + { + Object[] dims = currEntry.getKey().getDims(); + if (dimIndex >= dims.length) { + return matchNull; + } + + int[] dimsInt = (int[]) dims[dimIndex]; + if (dimsInt == null || dimsInt.length == 0) { + return matchNull; + } + + for (int id : dimsInt) { + if (Objects.equals(extractionFn.apply(getActualValue(id, false)), value)) { + return true; + } + } + return false; + } + }; + } + } + @Override + public ValueMatcher makeValueMatcher(final Predicate predicate, final boolean matchNull) + { + final BitSet predicateMatchingValueIds = DimensionSelectorUtils.makePredicateMatchingSet(this, predicate); + return new ValueMatcher() + { @Override - public void fill(int index, int[] toFill) + public boolean matches() { - throw new UnsupportedOperationException("fill not supported"); - } + Object[] dims = currEntry.getKey().getDims(); + if (dimIndex >= dims.length) { + return matchNull; + } - @Override - public void close() throws IOException - { + int[] dimsInt = (int[]) dims[dimIndex]; + if (dimsInt == null || dimsInt.length == 0) { + return matchNull; + } + for (int id : dimsInt) { + if (predicateMatchingValueIds.get(id)) { + return true; + } + } + return false; } }; } @@ -469,6 +531,19 @@ public String lookupName(int id) return extractionFn == null ? strValue : extractionFn.apply(strValue); } + @Override + public boolean nameLookupPossibleInAdvance() + { + return true; + } + + @Nullable + @Override + public IdLookup idLookup() + { + return extractionFn == null ? this : null; + } + @Override public int lookupId(String name) { @@ -479,7 +554,8 @@ public int lookupId(String name) } return getEncodedValue(name, false); } - }; + } + return new IndexerDimensionSelector(); } @Override diff --git a/processing/src/main/java/io/druid/segment/data/ArrayBasedIndexedInts.java b/processing/src/main/java/io/druid/segment/data/ArrayBasedIndexedInts.java index bdaec28727d1..50ba47094de4 100644 --- a/processing/src/main/java/io/druid/segment/data/ArrayBasedIndexedInts.java +++ b/processing/src/main/java/io/druid/segment/data/ArrayBasedIndexedInts.java @@ -19,34 +19,65 @@ package io.druid.segment.data; +import io.druid.java.util.common.IAE; +import it.unimi.dsi.fastutil.ints.IntArrays; import it.unimi.dsi.fastutil.ints.IntIterator; +import it.unimi.dsi.fastutil.ints.IntIterators; import java.io.IOException; /** */ -public class ArrayBasedIndexedInts implements IndexedInts +public final class ArrayBasedIndexedInts implements IndexedInts { + private static final ArrayBasedIndexedInts EMPTY = new ArrayBasedIndexedInts(IntArrays.EMPTY_ARRAY); + + /** + * Returns empty ArrayBasedIndexedInts, size = 0. This is useful instead of {@link EmptyIndexedInts} where + * monomorphism is a concern. + */ + public static ArrayBasedIndexedInts empty() + { + return EMPTY; + } + private final int[] expansion; + private final int size; - public ArrayBasedIndexedInts(int[] expansion) {this.expansion = expansion;} + public ArrayBasedIndexedInts(int[] expansion) + { + this.expansion = expansion; + this.size = expansion.length; + } + + public ArrayBasedIndexedInts(int[] expansion, int size) + { + this.expansion = expansion; + if (size < 0 || size > expansion.length) { + throw new IAE("Size[%s] should be between 0 and %s", size, expansion.length); + } + this.size = size; + } @Override public int size() { - return expansion.length; + return size; } @Override public int get(int index) { + if (index >= size) { + throw new IndexOutOfBoundsException("index: " + index + ", size: " + size); + } return expansion[index]; } @Override public IntIterator iterator() { - return new IndexedIntsIterator(this); + return IntIterators.wrap(expansion, 0, size); } @Override diff --git a/processing/src/main/java/io/druid/segment/data/SingleIndexedInt.java b/processing/src/main/java/io/druid/segment/data/SingleIndexedInt.java new file mode 100644 index 000000000000..2799cf9b31a7 --- /dev/null +++ b/processing/src/main/java/io/druid/segment/data/SingleIndexedInt.java @@ -0,0 +1,67 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets 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 io.druid.segment.data; + +import it.unimi.dsi.fastutil.ints.IntIterator; +import it.unimi.dsi.fastutil.ints.IntIterators; + +import java.io.IOException; + +public final class SingleIndexedInt implements IndexedInts +{ + private final int value; + + public SingleIndexedInt(int value) + { + this.value = value; + } + + @Override + public int size() + { + return 1; + } + + @Override + public int get(int i) + { + if (i != 0) { + throw new IllegalArgumentException(i + " != 0"); + } + return value; + } + + @Override + public IntIterator iterator() + { + return IntIterators.singleton(value); + } + + @Override + public void fill(int index, int[] toFill) + { + throw new UnsupportedOperationException("fill not supported"); + } + + @Override + public void close() throws IOException + { + } +} diff --git a/processing/src/main/java/io/druid/segment/filter/AndFilter.java b/processing/src/main/java/io/druid/segment/filter/AndFilter.java index 93ac4fc58c2a..2c9ad19343ae 100644 --- a/processing/src/main/java/io/druid/segment/filter/AndFilter.java +++ b/processing/src/main/java/io/druid/segment/filter/AndFilter.java @@ -80,7 +80,7 @@ public ImmutableBitmap getBitmapIndex(BitmapIndexSelector selector) public ValueMatcher makeMatcher(ColumnSelectorFactory factory) { if (filters.size() == 0) { - return new BooleanValueMatcher(false); + return BooleanValueMatcher.of(false); } final ValueMatcher[] matchers = new ValueMatcher[filters.size()]; diff --git a/processing/src/main/java/io/druid/segment/filter/BooleanValueMatcher.java b/processing/src/main/java/io/druid/segment/filter/BooleanValueMatcher.java index a34dadf01323..c52bedcfa451 100644 --- a/processing/src/main/java/io/druid/segment/filter/BooleanValueMatcher.java +++ b/processing/src/main/java/io/druid/segment/filter/BooleanValueMatcher.java @@ -23,17 +23,14 @@ /** */ -public class BooleanValueMatcher implements ValueMatcher +public final class BooleanValueMatcher { - private final boolean matches; - - public BooleanValueMatcher(final boolean matches) { - this.matches = matches; + public static ValueMatcher of(boolean matches) + { + return matches ? TrueValueMatcher.instance() : FalseValueMatcher.instance(); } - @Override - public boolean matches() + private BooleanValueMatcher() { - return matches; } } diff --git a/processing/src/main/java/io/druid/segment/filter/FalseValueMatcher.java b/processing/src/main/java/io/druid/segment/filter/FalseValueMatcher.java new file mode 100644 index 000000000000..189dae6a6332 --- /dev/null +++ b/processing/src/main/java/io/druid/segment/filter/FalseValueMatcher.java @@ -0,0 +1,42 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets 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 io.druid.segment.filter; + +import io.druid.query.filter.ValueMatcher; + +final class FalseValueMatcher implements ValueMatcher +{ + private static final FalseValueMatcher INSTANCE = new FalseValueMatcher(); + + public static FalseValueMatcher instance() + { + return INSTANCE; + } + + private FalseValueMatcher() + { + } + + @Override + public boolean matches() + { + return false; + } +} diff --git a/processing/src/main/java/io/druid/segment/filter/Filters.java b/processing/src/main/java/io/druid/segment/filter/Filters.java index aa94464b8470..d71d1ba8dec1 100644 --- a/processing/src/main/java/io/druid/segment/filter/Filters.java +++ b/processing/src/main/java/io/druid/segment/filter/Filters.java @@ -283,12 +283,12 @@ public static ValueMatcher getLongValueMatcher( ) { if (Strings.isNullOrEmpty(value)) { - return new BooleanValueMatcher(false); + return BooleanValueMatcher.of(false); } final Long longValue = GuavaUtils.tryParseLong(value); if (longValue == null) { - return new BooleanValueMatcher(false); + return BooleanValueMatcher.of(false); } return new ValueMatcher() diff --git a/processing/src/main/java/io/druid/segment/filter/TrueValueMatcher.java b/processing/src/main/java/io/druid/segment/filter/TrueValueMatcher.java new file mode 100644 index 000000000000..288e43870824 --- /dev/null +++ b/processing/src/main/java/io/druid/segment/filter/TrueValueMatcher.java @@ -0,0 +1,42 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets 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 io.druid.segment.filter; + +import io.druid.query.filter.ValueMatcher; + +final class TrueValueMatcher implements ValueMatcher +{ + private static final TrueValueMatcher INSTANCE = new TrueValueMatcher(); + + public static TrueValueMatcher instance() + { + return INSTANCE; + } + + private TrueValueMatcher() + { + } + + @Override + public boolean matches() + { + return true; + } +} diff --git a/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java b/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java index 364445a0f1fd..44d67fc686f6 100644 --- a/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java @@ -558,7 +558,7 @@ public ColumnCapabilities getColumnCapabilities(String columnName) private ValueMatcher makeFilterMatcher(final Filter filter, final Cursor cursor) { return filter == null - ? new BooleanValueMatcher(true) + ? BooleanValueMatcher.of(true) : filter.makeMatcher(cursor); } diff --git a/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java index f3a86bab175f..1194652596c1 100644 --- a/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java @@ -19,6 +19,7 @@ package io.druid.query.aggregation; +import com.google.common.base.Predicate; import com.google.common.collect.Lists; import io.druid.js.JavaScriptConfig; import io.druid.query.dimension.DimensionSpec; @@ -34,11 +35,14 @@ import io.druid.query.filter.RegexDimFilter; import io.druid.query.filter.SearchQueryDimFilter; import io.druid.query.filter.SelectorDimFilter; +import io.druid.query.filter.ValueMatcher; import io.druid.query.ordering.StringComparators; import io.druid.query.search.search.ContainsSearchQuerySpec; import io.druid.segment.ColumnSelectorFactory; import io.druid.segment.DimensionSelector; +import io.druid.segment.DimensionSelectorUtils; import io.druid.segment.FloatColumnSelector; +import io.druid.segment.IdLookup; import io.druid.segment.LongColumnSelector; import io.druid.segment.ObjectColumnSelector; import io.druid.segment.column.ColumnCapabilities; @@ -49,6 +53,7 @@ import org.junit.Assert; import org.junit.Test; +import javax.annotation.Nullable; import java.util.Arrays; public class FilteredAggregatorTest @@ -105,6 +110,18 @@ public IndexedInts getRow() } } + @Override + public ValueMatcher makeValueMatcher(String value, boolean matchNull) + { + return DimensionSelectorUtils.makeRowBasedValueMatcher(this, value, matchNull); + } + + @Override + public ValueMatcher makeValueMatcher(Predicate predicate, boolean matchNull) + { + return DimensionSelectorUtils.makeRowBasedValueMatcher(this, predicate, matchNull); + } + @Override public int getValueCardinality() { @@ -125,16 +142,30 @@ public String lookupName(int id) } @Override - public int lookupId(String name) + public boolean nameLookupPossibleInAdvance() { - switch (name) { - case "a": - return 0; - case "b": - return 1; - default: - throw new IllegalArgumentException(); - } + return true; + } + + @Nullable + @Override + public IdLookup idLookup() + { + return new IdLookup() + { + @Override + public int lookupId(String name) + { + switch (name) { + case "a": + return 0; + case "b": + return 1; + default: + throw new IllegalArgumentException(); + } + } + }; } } ); diff --git a/processing/src/test/java/io/druid/query/aggregation/cardinality/CardinalityAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/cardinality/CardinalityAggregatorTest.java index aa8560513f4c..e99707d110f2 100644 --- a/processing/src/test/java/io/druid/query/aggregation/cardinality/CardinalityAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/cardinality/CardinalityAggregatorTest.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Function; +import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Iterators; @@ -41,7 +42,10 @@ import io.druid.query.extraction.ExtractionFn; import io.druid.query.extraction.JavaScriptExtractionFn; import io.druid.query.extraction.RegexDimExtractionFn; +import io.druid.query.filter.ValueMatcher; import io.druid.segment.DimensionSelector; +import io.druid.segment.DimensionSelectorUtils; +import io.druid.segment.IdLookup; import io.druid.segment.data.IndexedInts; import it.unimi.dsi.fastutil.ints.IntIterator; import it.unimi.dsi.fastutil.ints.IntIterators; @@ -157,6 +161,18 @@ public void close() throws IOException }; } + @Override + public ValueMatcher makeValueMatcher(String value, boolean matchNull) + { + return DimensionSelectorUtils.makeRowBasedValueMatcher(this, value, matchNull); + } + + @Override + public ValueMatcher makeValueMatcher(Predicate predicate, boolean matchNull) + { + return DimensionSelectorUtils.makeRowBasedValueMatcher(this, predicate, matchNull); + } + @Override public int getValueCardinality() { @@ -171,9 +187,23 @@ public String lookupName(int i) } @Override - public int lookupId(String s) + public boolean nameLookupPossibleInAdvance() { - return ids.get(s); + return true; + } + + @Nullable + @Override + public IdLookup idLookup() + { + return new IdLookup() + { + @Override + public int lookupId(String s) + { + return ids.get(s); + } + }; } } diff --git a/processing/src/test/java/io/druid/query/dimension/ListFilteredDimensionSpecTest.java b/processing/src/test/java/io/druid/query/dimension/ListFilteredDimensionSpecTest.java index ac9ee2f97303..ef1ba66aabac 100644 --- a/processing/src/test/java/io/druid/query/dimension/ListFilteredDimensionSpecTest.java +++ b/processing/src/test/java/io/druid/query/dimension/ListFilteredDimensionSpecTest.java @@ -134,8 +134,8 @@ public void testDecoratorWithWhitelist() Assert.assertEquals("c", selector.lookupName(0)); Assert.assertEquals("g", selector.lookupName(1)); - Assert.assertEquals(0, selector.lookupId("c")); - Assert.assertEquals(1, selector.lookupId("g")); + Assert.assertEquals(0, selector.idLookup().lookupId("c")); + Assert.assertEquals(1, selector.idLookup().lookupId("g")); } @Test @@ -158,8 +158,8 @@ public void testDecoratorWithBlacklist() Assert.assertEquals("a", selector.lookupName(0)); Assert.assertEquals("z", selector.lookupName(23)); - Assert.assertEquals(0, selector.lookupId("a")); - Assert.assertEquals(23, selector.lookupId("z")); + Assert.assertEquals(0, selector.idLookup().lookupId("a")); + Assert.assertEquals(23, selector.idLookup().lookupId("z")); } @Test @@ -186,7 +186,7 @@ public void testDecoratorWithBlacklistUsingNonPresentValues() Assert.assertEquals("a", selector.lookupName(0)); Assert.assertEquals("z", selector.lookupName(24)); - Assert.assertEquals(0, selector.lookupId("a")); - Assert.assertEquals(24, selector.lookupId("z")); + Assert.assertEquals(0, selector.idLookup().lookupId("a")); + Assert.assertEquals(24, selector.idLookup().lookupId("z")); } } diff --git a/processing/src/test/java/io/druid/query/dimension/RegexFilteredDimensionSpecTest.java b/processing/src/test/java/io/druid/query/dimension/RegexFilteredDimensionSpecTest.java index 61d55cba7629..623fc3b3a5f4 100644 --- a/processing/src/test/java/io/druid/query/dimension/RegexFilteredDimensionSpecTest.java +++ b/processing/src/test/java/io/druid/query/dimension/RegexFilteredDimensionSpecTest.java @@ -96,7 +96,7 @@ public void testDecorator() Assert.assertEquals("c", selector.lookupName(0)); Assert.assertEquals("g", selector.lookupName(1)); - Assert.assertEquals(0, selector.lookupId("c")); - Assert.assertEquals(1, selector.lookupId("g")); + Assert.assertEquals(0, selector.idLookup().lookupId("c")); + Assert.assertEquals(1, selector.idLookup().lookupId("g")); } } diff --git a/processing/src/test/java/io/druid/query/dimension/TestDimensionSelector.java b/processing/src/test/java/io/druid/query/dimension/TestDimensionSelector.java index 89c7d14d7e5e..fc73defe53a7 100644 --- a/processing/src/test/java/io/druid/query/dimension/TestDimensionSelector.java +++ b/processing/src/test/java/io/druid/query/dimension/TestDimensionSelector.java @@ -19,10 +19,16 @@ package io.druid.query.dimension; +import com.google.common.base.Predicate; +import io.druid.query.filter.ValueMatcher; import io.druid.segment.DimensionSelector; +import io.druid.segment.DimensionSelectorUtils; +import io.druid.segment.IdLookup; import io.druid.segment.data.ArrayBasedIndexedInts; import io.druid.segment.data.IndexedInts; +import javax.annotation.Nullable; + /** * Test dimension selector that has cardinality=26 * encoding 0 -> a, 1 -> b, ... @@ -43,6 +49,18 @@ public IndexedInts getRow() return new ArrayBasedIndexedInts(new int[]{2, 4, 6}); } + @Override + public ValueMatcher makeValueMatcher(String value, boolean matchNull) + { + return DimensionSelectorUtils.makeRowBasedValueMatcher(this, value, matchNull); + } + + @Override + public ValueMatcher makeValueMatcher(Predicate predicate, boolean matchNull) + { + return DimensionSelectorUtils.makeRowBasedValueMatcher(this, predicate, matchNull); + } + @Override public int getValueCardinality() { @@ -56,9 +74,22 @@ public String lookupName(int id) } @Override - public int lookupId(String name) + public boolean nameLookupPossibleInAdvance() { - return name.charAt(0) - 'a'; + return true; } + @Nullable + @Override + public IdLookup idLookup() + { + return new IdLookup() + { + @Override + public int lookupId(String name) + { + return name.charAt(0) - 'a'; + } + }; + } } diff --git a/processing/src/test/java/io/druid/segment/NullDimensionSelectorTest.java b/processing/src/test/java/io/druid/segment/NullDimensionSelectorTest.java index aab2dff99c64..da8b7c837120 100644 --- a/processing/src/test/java/io/druid/segment/NullDimensionSelectorTest.java +++ b/processing/src/test/java/io/druid/segment/NullDimensionSelectorTest.java @@ -53,8 +53,8 @@ public void testLookupName() throws Exception { @Test public void testLookupId() throws Exception { - Assert.assertEquals(0, selector.lookupId(null)); - Assert.assertEquals(0, selector.lookupId("")); - Assert.assertEquals(-1, selector.lookupId("billy")); + Assert.assertEquals(0, selector.idLookup().lookupId(null)); + Assert.assertEquals(0, selector.idLookup().lookupId("")); + Assert.assertEquals(-1, selector.idLookup().lookupId("billy")); } } From e6cf49a49541fa0dc554d6d1774913a46d45d715 Mon Sep 17 00:00:00 2001 From: leventov Date: Tue, 17 Jan 2017 15:56:29 -0600 Subject: [PATCH 02/12] Rename DimensionSelectorUtils.makeNonDictionaryEncodedIndexedIntsBasedValueMatcher to makeNonDictionaryEncodedRowBasedValueMatcher --- .../main/java/io/druid/segment/DimensionSelectorUtils.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java index 4c2f11454925..c6f7d1d041cb 100644 --- a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java +++ b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java @@ -45,7 +45,7 @@ public static ValueMatcher makeRowBasedValueMatcher( if (idLookup != null) { return makeDictionaryEncodedRowBasedValueMatcher(selector, idLookup.lookupId(value), matchNull); } else { - return makeNonDictionaryEncodedIndexedIntsBasedValueMatcher(selector, value, matchNull); + return makeNonDictionaryEncodedRowBasedValueMatcher(selector, value, matchNull); } } @@ -94,7 +94,7 @@ public boolean matches() } } - private static ValueMatcher makeNonDictionaryEncodedIndexedIntsBasedValueMatcher( + private static ValueMatcher makeNonDictionaryEncodedRowBasedValueMatcher( final DimensionSelector selector, final String value, final boolean matchNull From 1ba84255f664c91c16afea90b57ed197fa805629 Mon Sep 17 00:00:00 2001 From: leventov Date: Tue, 17 Jan 2017 16:53:46 -0600 Subject: [PATCH 03/12] Make ArrayBasedIndexedInts constructor private, replace it's usages with of() static factory method --- .../ForwardingFilteredDimensionSelector.java | 2 +- .../PredicateFilteredDimensionSelector.java | 2 +- .../druid/segment/StringDimensionIndexer.java | 2 +- .../segment/data/ArrayBasedIndexedInts.java | 35 ++++++++++--------- .../aggregation/FilteredAggregatorTest.java | 4 +-- .../dimension/TestDimensionSelector.java | 2 +- .../CompressedVSizeIndexedV3WriterTest.java | 2 +- 7 files changed, 25 insertions(+), 24 deletions(-) diff --git a/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java b/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java index 3f094ff1e9a8..d0840e394ea2 100644 --- a/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java +++ b/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java @@ -71,7 +71,7 @@ public IndexedInts getRow() result[resultSize++] = forwardedValue; } } - return new ArrayBasedIndexedInts(result, resultSize); + return ArrayBasedIndexedInts.of(result, resultSize); } @Override diff --git a/processing/src/main/java/io/druid/query/dimension/PredicateFilteredDimensionSelector.java b/processing/src/main/java/io/druid/query/dimension/PredicateFilteredDimensionSelector.java index 999ba10621b7..9f7682ec8124 100644 --- a/processing/src/main/java/io/druid/query/dimension/PredicateFilteredDimensionSelector.java +++ b/processing/src/main/java/io/druid/query/dimension/PredicateFilteredDimensionSelector.java @@ -52,7 +52,7 @@ public IndexedInts getRow() result[resultSize++] = i; } } - return new ArrayBasedIndexedInts(result, resultSize); + return ArrayBasedIndexedInts.of(result, resultSize); } @Override diff --git a/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java b/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java index 3bdce30f2057..aeecd52aa84c 100644 --- a/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java +++ b/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java @@ -427,7 +427,7 @@ public IndexedInts getRow() } } - return rowSize > 0 ? new ArrayBasedIndexedInts(row, rowSize) : ArrayBasedIndexedInts.empty(); + return ArrayBasedIndexedInts.of(row, rowSize); } @Override diff --git a/processing/src/main/java/io/druid/segment/data/ArrayBasedIndexedInts.java b/processing/src/main/java/io/druid/segment/data/ArrayBasedIndexedInts.java index 50ba47094de4..71ad4025d0aa 100644 --- a/processing/src/main/java/io/druid/segment/data/ArrayBasedIndexedInts.java +++ b/processing/src/main/java/io/druid/segment/data/ArrayBasedIndexedInts.java @@ -30,32 +30,33 @@ */ public final class ArrayBasedIndexedInts implements IndexedInts { - private static final ArrayBasedIndexedInts EMPTY = new ArrayBasedIndexedInts(IntArrays.EMPTY_ARRAY); + private static final ArrayBasedIndexedInts EMPTY = new ArrayBasedIndexedInts(IntArrays.EMPTY_ARRAY, 0); - /** - * Returns empty ArrayBasedIndexedInts, size = 0. This is useful instead of {@link EmptyIndexedInts} where - * monomorphism is a concern. - */ - public static ArrayBasedIndexedInts empty() + public static ArrayBasedIndexedInts of(int[] expansion) { - return EMPTY; + if (expansion.length == 0) { + return EMPTY; + } + return new ArrayBasedIndexedInts(expansion, expansion.length); } - private final int[] expansion; - private final int size; - - public ArrayBasedIndexedInts(int[] expansion) + public static ArrayBasedIndexedInts of(int[] expansion, int size) { - this.expansion = expansion; - this.size = expansion.length; + if (size == 0) { + return EMPTY; + } + if (size < 0 || size > expansion.length) { + throw new IAE("Size[%s] should be between 0 and %s", size, expansion.length); + } + return new ArrayBasedIndexedInts(expansion, size); } - public ArrayBasedIndexedInts(int[] expansion, int size) + private final int[] expansion; + private final int size; + + private ArrayBasedIndexedInts(int[] expansion, int size) { this.expansion = expansion; - if (size < 0 || size > expansion.length) { - throw new IAE("Size[%s] should be between 0 and %s", size, expansion.length); - } this.size = size; } diff --git a/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java index 1194652596c1..ffbfed8def64 100644 --- a/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java @@ -104,9 +104,9 @@ public DimensionSelector makeDimensionSelector(DimensionSpec dimensionSpec) public IndexedInts getRow() { if (selector.getIndex() % 3 == 2) { - return new ArrayBasedIndexedInts(new int[]{1}); + return ArrayBasedIndexedInts.of(new int[]{1}); } else { - return new ArrayBasedIndexedInts(new int[]{0}); + return ArrayBasedIndexedInts.of(new int[]{0}); } } diff --git a/processing/src/test/java/io/druid/query/dimension/TestDimensionSelector.java b/processing/src/test/java/io/druid/query/dimension/TestDimensionSelector.java index fc73defe53a7..1b19e7951c9e 100644 --- a/processing/src/test/java/io/druid/query/dimension/TestDimensionSelector.java +++ b/processing/src/test/java/io/druid/query/dimension/TestDimensionSelector.java @@ -46,7 +46,7 @@ private TestDimensionSelector() @Override public IndexedInts getRow() { - return new ArrayBasedIndexedInts(new int[]{2, 4, 6}); + return ArrayBasedIndexedInts.of(new int[]{2, 4, 6}); } @Override diff --git a/processing/src/test/java/io/druid/segment/data/CompressedVSizeIndexedV3WriterTest.java b/processing/src/test/java/io/druid/segment/data/CompressedVSizeIndexedV3WriterTest.java index 2eb34001a902..52aeaa4ebb3c 100644 --- a/processing/src/test/java/io/druid/segment/data/CompressedVSizeIndexedV3WriterTest.java +++ b/processing/src/test/java/io/druid/segment/data/CompressedVSizeIndexedV3WriterTest.java @@ -123,7 +123,7 @@ private void checkSerializedSizeAndData(int offsetChunkFactor, int valueChunkFac @Override public IndexedInts apply(@Nullable final int[] input) { - return new ArrayBasedIndexedInts(input); + return ArrayBasedIndexedInts.of(input); } } ), offsetChunkFactor, maxValue, byteOrder, compressionStrategy From 4ca2b9bbc7feb5e09efe735fe3caaeaa67145abd Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 18 Jan 2017 00:05:24 -0600 Subject: [PATCH 04/12] Cache baseIdLookup in ForwardingFilteredDimensionSelector --- .../ForwardingFilteredDimensionSelector.java | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java b/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java index d0840e394ea2..e752b599a871 100644 --- a/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java +++ b/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java @@ -38,6 +38,7 @@ final class ForwardingFilteredDimensionSelector implements DimensionSelector, IdLookup { private final DimensionSelector selector; + private final IdLookup baseIdLookup; private final Int2IntMap forwardMapping; private final int[] reverseMapping; @@ -51,6 +52,7 @@ final class ForwardingFilteredDimensionSelector implements DimensionSelector, Id if (!selector.nameLookupPossibleInAdvance()) { throw new IAE("selector.nameLookupPossibleInAdvance() should return true"); } + this.baseIdLookup = selector.idLookup(); this.forwardMapping = Preconditions.checkNotNull(forwardMapping); if (forwardMapping.defaultReturnValue() != -1) { throw new IAE("forwardMapping.defaultReturnValue() should be -1"); @@ -182,19 +184,12 @@ public boolean nameLookupPossibleInAdvance() @Override public IdLookup idLookup() { - final IdLookup baseIdLookup = selector.idLookup(); - if (baseIdLookup != null) { - return this; - } else { - return null; - } + return baseIdLookup != null ? this : null; } @Override public int lookupId(String name) { - // Don't cache selector.idLookup(), because it is likely implemented as `return this`, in this case caching only - // makes access slower. - return forwardMapping.get(selector.idLookup().lookupId(name)); + return forwardMapping.get(baseIdLookup.lookupId(name)); } } From 4f1e2223d4420f0d3ffd33231650102c38acbb93 Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 18 Jan 2017 00:17:38 -0600 Subject: [PATCH 05/12] Fix a bug in DimensionSelectorUtils.makeRowBasedValueMatcher(selector, predicate, matchNull) --- .../src/main/java/io/druid/segment/DimensionSelectorUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java index c6f7d1d041cb..e9c0679a1584 100644 --- a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java +++ b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java @@ -129,7 +129,7 @@ public static ValueMatcher makeRowBasedValueMatcher( ) { int cardinality = selector.getValueCardinality(); - if (cardinality >= 0) { + if (cardinality >= 0 && selector.nameLookupPossibleInAdvance()) { return makeDictionaryEncodedRowBasedValueMatcher(selector, predicate, matchNull); } else { return makeNonDictionaryEncodedRowBasedValueMatcher(selector, predicate, matchNull); From 7b25cf714f15d022c5c831c39294462bc207f89e Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 18 Jan 2017 00:20:22 -0600 Subject: [PATCH 06/12] Employ precomputed BitSet optimization in DimensionSelector.makeValueMatcher(value, matchNull) when lookupId() is not available, but cardinality is known and lookupName() is available --- .../ForwardingFilteredDimensionSelector.java | 26 +++--------------- .../druid/segment/DimensionSelectorUtils.java | 4 +++ .../segment/QueryableIndexStorageAdapter.java | 12 ++------- .../druid/segment/StringDimensionIndexer.java | 27 +++---------------- 4 files changed, 12 insertions(+), 57 deletions(-) diff --git a/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java b/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java index e752b599a871..7857526c766a 100644 --- a/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java +++ b/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java @@ -21,6 +21,7 @@ import com.google.common.base.Preconditions; import com.google.common.base.Predicate; +import com.google.common.base.Predicates; import io.druid.java.util.common.IAE; import io.druid.query.filter.ValueMatcher; import io.druid.segment.DimensionSelector; @@ -33,7 +34,6 @@ import javax.annotation.Nullable; import java.util.BitSet; -import java.util.Objects; final class ForwardingFilteredDimensionSelector implements DimensionSelector, IdLookup { @@ -110,28 +110,8 @@ public boolean matches() return BooleanValueMatcher.of(false); } } else { - return new ValueMatcher() - { - @Override - public boolean matches() - { - final IndexedInts baseRow = selector.getRow(); - final int baseRowSize = baseRow.size(); - boolean nullRow = true; - for (int i = 0; i < baseRowSize; i++) { - int rowValueId = baseRow.get(i); - int forwardedValue = forwardMapping.get(rowValueId); - if (forwardedValue >= 0) { - if (Objects.equals(selector.lookupName(rowValueId), value)) { - return true; - } - nullRow = false; - } - } - // null should match empty rows in multi-value columns - return nullRow && matchNull; - } - }; + // Employ precomputed BitSet optimization + return makeValueMatcher(Predicates.equalTo(value), matchNull); } } diff --git a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java index e9c0679a1584..c00265af0087 100644 --- a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java +++ b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java @@ -20,6 +20,7 @@ package io.druid.segment; import com.google.common.base.Predicate; +import com.google.common.base.Predicates; import io.druid.java.util.common.IAE; import io.druid.query.filter.ValueMatcher; import io.druid.segment.data.IndexedInts; @@ -44,6 +45,9 @@ public static ValueMatcher makeRowBasedValueMatcher( IdLookup idLookup = selector.idLookup(); if (idLookup != null) { return makeDictionaryEncodedRowBasedValueMatcher(selector, idLookup.lookupId(value), matchNull); + } else if (selector.getValueCardinality() >= 0 && selector.nameLookupPossibleInAdvance()) { + // Employ precomputed BitSet optimization + return makeRowBasedValueMatcher(selector, Predicates.equalTo(value), matchNull); } else { return makeNonDictionaryEncodedRowBasedValueMatcher(selector, value, matchNull); } diff --git a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java index ea21705cb755..f72926d15a6f 100644 --- a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java @@ -63,7 +63,6 @@ import java.util.BitSet; import java.util.List; import java.util.Map; -import java.util.Objects; /** */ @@ -585,15 +584,8 @@ public boolean matches() return BooleanValueMatcher.of(false); } } else { - return new ValueMatcher() - { - @Override - public boolean matches() - { - String rowValue = lookupName(column.getSingleValueRow(cursorOffset.getOffset())); - return Objects.equals(rowValue, value); - } - }; + // Employ precomputed BitSet optimization + return makeValueMatcher(Predicates.equalTo(value), matchNull); } } diff --git a/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java b/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java index aeecd52aa84c..89c6db3087ec 100644 --- a/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java +++ b/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java @@ -21,6 +21,7 @@ import com.google.common.base.Function; import com.google.common.base.Predicate; +import com.google.common.base.Predicates; import com.google.common.base.Strings; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -48,7 +49,6 @@ import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.Objects; public class StringDimensionIndexer implements DimensionIndexer { @@ -463,29 +463,8 @@ public boolean matches() return BooleanValueMatcher.of(false); } } else { - return new ValueMatcher() - { - @Override - public boolean matches() - { - Object[] dims = currEntry.getKey().getDims(); - if (dimIndex >= dims.length) { - return matchNull; - } - - int[] dimsInt = (int[]) dims[dimIndex]; - if (dimsInt == null || dimsInt.length == 0) { - return matchNull; - } - - for (int id : dimsInt) { - if (Objects.equals(extractionFn.apply(getActualValue(id, false)), value)) { - return true; - } - } - return false; - } - }; + // Employ precomputed BitSet optimization + return makeValueMatcher(Predicates.equalTo(value), matchNull); } } From 90b1870c9a12e93119700b2f4877d6dfa8035857 Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 18 Jan 2017 00:22:42 -0600 Subject: [PATCH 07/12] Doc fixes --- .../main/java/io/druid/segment/DimensionSelector.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/processing/src/main/java/io/druid/segment/DimensionSelector.java b/processing/src/main/java/io/druid/segment/DimensionSelector.java index cca9681844ae..2218b50e4e42 100644 --- a/processing/src/main/java/io/druid/segment/DimensionSelector.java +++ b/processing/src/main/java/io/druid/segment/DimensionSelector.java @@ -91,12 +91,12 @@ public interface DimensionSelector extends ColumnValueSelector /** * Returns true if it is possible to {@link #lookupName(int)} by ids from 0 to {@link #getValueCardinality()} - * Before the rows with those ids are returns. + * before the rows with those ids are returned. * - *

Returns false if {@link #lookupName(int)} could be called only with ids, returned from the last call of {@link - * #getRow()} on this DimensionSelector, but not earlier and not later. If {@link #lookupName(int)} is called with an - * id, which is not present in the last {@link IndexedInts}, returned from {@link #getRow()}, result is undefined: - * exception could be thrown, or null returned, or some other random value. + *

Returns false if {@link #lookupName(int)} could be called only with ids, returned from the most recent call of + * {@link #getRow()} on this DimensionSelector, but not earlier and not later. If {@link #lookupName(int)} is called + * with an id, which is not present in the most recent {@link IndexedInts}, returned from {@link #getRow()}, result is + * undefined: exception could be thrown, or null returned, or some other random value. */ boolean nameLookupPossibleInAdvance(); From 1e33611639b3cafd8029fb8597a2b5a0e71c031b Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 25 Jan 2017 00:11:14 -0600 Subject: [PATCH 08/12] Addressed comments --- .../ForwardingFilteredDimensionSelector.java | 11 ++-- .../PredicateFilteredDimensionSelector.java | 7 ++- ...ingValueMatcherColumnSelectorStrategy.java | 27 +++------ .../RowBasedColumnSelectorFactory.java | 15 +++-- .../druid/query/topn/BaseTopNAlgorithm.java | 8 ++- .../io/druid/segment/DimensionSelector.java | 7 ++- .../druid/segment/DimensionSelectorUtils.java | 55 ++++++++++--------- .../druid/segment/NullDimensionSelector.java | 4 +- .../segment/QueryableIndexStorageAdapter.java | 14 ++--- .../segment/SingleScanTimeDimSelector.java | 4 +- .../druid/segment/StringDimensionIndexer.java | 13 +++-- .../aggregation/FilteredAggregatorTest.java | 8 +-- .../CardinalityAggregatorTest.java | 8 +-- .../dimension/TestDimensionSelector.java | 8 +-- 14 files changed, 95 insertions(+), 94 deletions(-) diff --git a/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java b/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java index 7857526c766a..ffb9e56f5d4f 100644 --- a/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java +++ b/processing/src/main/java/io/druid/query/dimension/ForwardingFilteredDimensionSelector.java @@ -77,12 +77,12 @@ public IndexedInts getRow() } @Override - public ValueMatcher makeValueMatcher(final String value, final boolean matchNull) + public ValueMatcher makeValueMatcher(final String value) { IdLookup idLookup = idLookup(); if (idLookup != null) { final int valueId = idLookup.lookupId(value); - if (valueId >= 0 || matchNull) { + if (valueId >= 0 || value == null) { return new ValueMatcher() { @Override @@ -103,7 +103,7 @@ public boolean matches() } } // null should match empty rows in multi-value columns - return nullRow && matchNull; + return nullRow && value == null; } }; } else { @@ -111,14 +111,15 @@ public boolean matches() } } else { // Employ precomputed BitSet optimization - return makeValueMatcher(Predicates.equalTo(value), matchNull); + return makeValueMatcher(Predicates.equalTo(value)); } } @Override - public ValueMatcher makeValueMatcher(Predicate predicate, final boolean matchNull) + public ValueMatcher makeValueMatcher(Predicate predicate) { final BitSet valueIds = DimensionSelectorUtils.makePredicateMatchingSet(this, predicate); + final boolean matchNull = predicate.apply(null); return new ValueMatcher() { @Override diff --git a/processing/src/main/java/io/druid/query/dimension/PredicateFilteredDimensionSelector.java b/processing/src/main/java/io/druid/query/dimension/PredicateFilteredDimensionSelector.java index 9f7682ec8124..a46accc04914 100644 --- a/processing/src/main/java/io/druid/query/dimension/PredicateFilteredDimensionSelector.java +++ b/processing/src/main/java/io/druid/query/dimension/PredicateFilteredDimensionSelector.java @@ -56,7 +56,7 @@ public IndexedInts getRow() } @Override - public ValueMatcher makeValueMatcher(final String value, final boolean matchNull) + public ValueMatcher makeValueMatcher(final String value) { return new ValueMatcher() { @@ -76,14 +76,15 @@ public boolean matches() } } // null should match empty rows in multi-value columns - return nullRow && matchNull; + return nullRow && value == null; } }; } @Override - public ValueMatcher makeValueMatcher(final Predicate matcherPredicate, final boolean matchNull) + public ValueMatcher makeValueMatcher(final Predicate matcherPredicate) { + final boolean matchNull = predicate.apply(null); return new ValueMatcher() { @Override diff --git a/processing/src/main/java/io/druid/query/filter/StringValueMatcherColumnSelectorStrategy.java b/processing/src/main/java/io/druid/query/filter/StringValueMatcherColumnSelectorStrategy.java index e4a8fa23bee9..2772bf78befb 100644 --- a/processing/src/main/java/io/druid/query/filter/StringValueMatcherColumnSelectorStrategy.java +++ b/processing/src/main/java/io/druid/query/filter/StringValueMatcherColumnSelectorStrategy.java @@ -27,20 +27,13 @@ public class StringValueMatcherColumnSelectorStrategy implements ValueMatcherColumnSelectorStrategy { @Override - public ValueMatcher makeValueMatcher(final DimensionSelector selector, final String value) + public ValueMatcher makeValueMatcher(final DimensionSelector selector, String value) { - final String valueStr = Strings.emptyToNull(value); - - // if matching against null, rows with size 0 should also match - final boolean matchNull = Strings.isNullOrEmpty(valueStr); - - final int cardinality = selector.getValueCardinality(); - - if (cardinality == 0 || (cardinality == 1 && selector.lookupName(0) == null)) { - // All values are null or empty rows (which match nulls anyway). No need to check each row. - return BooleanValueMatcher.of(matchNull); + value = Strings.emptyToNull(value); + if (selector.getValueCardinality() == 0) { + return BooleanValueMatcher.of(value == null); } else { - return selector.makeValueMatcher(valueStr, matchNull); + return selector.makeValueMatcher(value); } } @@ -51,14 +44,10 @@ public ValueMatcher makeValueMatcher( ) { final Predicate predicate = predicateFactory.makeStringPredicate(); - final int cardinality = selector.getValueCardinality(); - final boolean matchNull = predicate.apply(null); - - if (cardinality == 0 || (cardinality == 1 && selector.lookupName(0) == null)) { - // All values are null or empty rows (which match nulls anyway). No need to check each row. - return BooleanValueMatcher.of(matchNull); + if (selector.getValueCardinality() == 0) { + return BooleanValueMatcher.of(predicate.apply(null)); } else { - return selector.makeValueMatcher(predicate, matchNull); + return selector.makeValueMatcher(predicate); } } diff --git a/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java b/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java index e2e7f2a72958..959aeeea5996 100644 --- a/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java +++ b/processing/src/main/java/io/druid/query/groupby/RowBasedColumnSelectorFactory.java @@ -113,7 +113,7 @@ public IndexedInts getRow() } @Override - public ValueMatcher makeValueMatcher(final String value, boolean matchNull) + public ValueMatcher makeValueMatcher(final String value) { return new ValueMatcher() { @@ -127,9 +127,7 @@ public boolean matches() } @Override - public ValueMatcher makeValueMatcher( - final Predicate predicate, boolean matchNull - ) + public ValueMatcher makeValueMatcher(final Predicate predicate) { return new ValueMatcher() { @@ -178,7 +176,7 @@ public IndexedInts getRow() } @Override - public ValueMatcher makeValueMatcher(final String value, final boolean matchNull) + public ValueMatcher makeValueMatcher(final String value) { if (extractionFn == null) { return new ValueMatcher() @@ -188,7 +186,7 @@ public boolean matches() { final List dimensionValues = row.get().getDimension(dimension); if (dimensionValues == null || dimensionValues.isEmpty()) { - return matchNull; + return value == null; } for (String dimensionValue : dimensionValues) { @@ -207,7 +205,7 @@ public boolean matches() { final List dimensionValues = row.get().getDimension(dimension); if (dimensionValues == null || dimensionValues.isEmpty()) { - return matchNull; + return value == null; } for (String dimensionValue : dimensionValues) { @@ -222,8 +220,9 @@ public boolean matches() } @Override - public ValueMatcher makeValueMatcher(final Predicate predicate, final boolean matchNull) + public ValueMatcher makeValueMatcher(final Predicate predicate) { + final boolean matchNull = predicate.apply(null); if (extractionFn == null) { return new ValueMatcher() { diff --git a/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java b/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java index 62871a221516..b63d830fa236 100644 --- a/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java +++ b/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java @@ -19,6 +19,7 @@ package io.druid.query.topn; +import io.druid.java.util.common.IAE; import io.druid.java.util.common.Pair; import io.druid.query.aggregation.Aggregator; import io.druid.query.aggregation.AggregatorFactory; @@ -192,6 +193,9 @@ public BaseArrayProvider( { this.dimSelector = dimSelector; this.idLookup = dimSelector.idLookup(); + if (idLookup == null) { + throw new IAE("Only DimensionSelectors which support idLookup() are supported yet"); + } this.query = query; this.capabilities = capabilities; @@ -201,7 +205,7 @@ public BaseArrayProvider( keepOnlyN = dimSelector.getValueCardinality(); if (keepOnlyN < 0) { - throw new UnsupportedOperationException("Cannot operate on a dimension with no dictionary"); + throw new IAE("Cannot operate on a dimension with no dictionary"); } } @@ -235,7 +239,7 @@ protected Pair computeStartEnd(int cardinality) { int startIndex = ignoreFirstN; - if (previousStop != null && idLookup != null) { + if (previousStop != null) { int lookupId = idLookup.lookupId(previousStop) + 1; if (lookupId < 0) { lookupId *= -1; diff --git a/processing/src/main/java/io/druid/segment/DimensionSelector.java b/processing/src/main/java/io/druid/segment/DimensionSelector.java index 2218b50e4e42..516649e8d730 100644 --- a/processing/src/main/java/io/druid/segment/DimensionSelector.java +++ b/processing/src/main/java/io/druid/segment/DimensionSelector.java @@ -40,9 +40,12 @@ public interface DimensionSelector extends ColumnValueSelector */ public IndexedInts getRow(); - ValueMatcher makeValueMatcher(String value, boolean matchNull); + /** + * @param value nullable dimension value + */ + ValueMatcher makeValueMatcher(String value); - ValueMatcher makeValueMatcher(Predicate predicate, boolean matchNull); + ValueMatcher makeValueMatcher(Predicate predicate); /** * Value cardinality is the cardinality of the different occurring values. If there were 4 rows: diff --git a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java index c00265af0087..4ade259a100e 100644 --- a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java +++ b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java @@ -36,24 +36,26 @@ private DimensionSelectorUtils() { } - public static ValueMatcher makeRowBasedValueMatcher( - DimensionSelector selector, - String value, - boolean matchNull - ) + /** + * Generic implementation of {@link DimensionSelector#makeValueMatcher(String)}, uses {@link + * DimensionSelector#getRow()} of the given {@link DimensionSelector}. "Lazy" DimensionSelectors could delegate + * {@code makeValueMatcher()} to this method, but encouraged to implement {@code makeValueMatcher()} themselves, + * bypassing the {@link IndexedInts} abstraction. + */ + public static ValueMatcher makeValueMatcherGeneric(DimensionSelector selector, String value) { IdLookup idLookup = selector.idLookup(); if (idLookup != null) { - return makeDictionaryEncodedRowBasedValueMatcher(selector, idLookup.lookupId(value), matchNull); + return makeDictionaryEncodedValueMatcherGeneric(selector, idLookup.lookupId(value), value == null); } else if (selector.getValueCardinality() >= 0 && selector.nameLookupPossibleInAdvance()) { // Employ precomputed BitSet optimization - return makeRowBasedValueMatcher(selector, Predicates.equalTo(value), matchNull); + return makeDictionaryEncodedValueMatcherGeneric(selector, Predicates.equalTo(value)); } else { - return makeNonDictionaryEncodedRowBasedValueMatcher(selector, value, matchNull); + return makeNonDictionaryEncodedValueMatcherGeneric(selector, value); } } - private static ValueMatcher makeDictionaryEncodedRowBasedValueMatcher( + private static ValueMatcher makeDictionaryEncodedValueMatcherGeneric( final DimensionSelector selector, final int valueId, final boolean matchNull @@ -98,10 +100,9 @@ public boolean matches() } } - private static ValueMatcher makeNonDictionaryEncodedRowBasedValueMatcher( + private static ValueMatcher makeNonDictionaryEncodedValueMatcherGeneric( final DimensionSelector selector, - final String value, - final boolean matchNull + final String value ) { return new ValueMatcher() @@ -113,7 +114,7 @@ public boolean matches() final int size = row.size(); if (size == 0) { // null should match empty rows in multi-value columns - return matchNull; + return value == null; } else { for (int i = 0; i < size; ++i) { if (Objects.equals(selector.lookupName(row.get(i)), value)) { @@ -126,27 +127,29 @@ public boolean matches() }; } - public static ValueMatcher makeRowBasedValueMatcher( - final DimensionSelector selector, - final Predicate predicate, - final boolean matchNull - ) + /** + * Generic implementation of {@link DimensionSelector#makeValueMatcher(Predicate)}, uses {@link + * DimensionSelector#getRow()} of the given {@link DimensionSelector}. "Lazy" DimensionSelectors could delegate + * {@code makeValueMatcher()} to this method, but encouraged to implement {@code makeValueMatcher()} themselves, + * bypassing the {@link IndexedInts} abstraction. + */ + public static ValueMatcher makeValueMatcherGeneric(DimensionSelector selector, Predicate predicate) { int cardinality = selector.getValueCardinality(); if (cardinality >= 0 && selector.nameLookupPossibleInAdvance()) { - return makeDictionaryEncodedRowBasedValueMatcher(selector, predicate, matchNull); + return makeDictionaryEncodedValueMatcherGeneric(selector, predicate); } else { - return makeNonDictionaryEncodedRowBasedValueMatcher(selector, predicate, matchNull); + return makeNonDictionaryEncodedValueMatcherGeneric(selector, predicate); } } - private static ValueMatcher makeDictionaryEncodedRowBasedValueMatcher( + private static ValueMatcher makeDictionaryEncodedValueMatcherGeneric( final DimensionSelector selector, - Predicate predicate, - final boolean matchNull + Predicate predicate ) { final BitSet predicateMatchingValueIds = makePredicateMatchingSet(selector, predicate); + final boolean matchNull = predicate.apply(null); return new ValueMatcher() { @Override @@ -169,12 +172,12 @@ public boolean matches() }; } - private static ValueMatcher makeNonDictionaryEncodedRowBasedValueMatcher( + private static ValueMatcher makeNonDictionaryEncodedValueMatcherGeneric( final DimensionSelector selector, - final Predicate predicate, - final boolean matchNull + final Predicate predicate ) { + final boolean matchNull = predicate.apply(null); return new ValueMatcher() { @Override diff --git a/processing/src/main/java/io/druid/segment/NullDimensionSelector.java b/processing/src/main/java/io/druid/segment/NullDimensionSelector.java index e78b2f272d0a..cdbd371bf04e 100644 --- a/processing/src/main/java/io/druid/segment/NullDimensionSelector.java +++ b/processing/src/main/java/io/druid/segment/NullDimensionSelector.java @@ -37,13 +37,13 @@ public IndexedInts getRow() } @Override - public ValueMatcher makeValueMatcher(String value, boolean matchNull) + public ValueMatcher makeValueMatcher(String value) { return BooleanValueMatcher.of(value == null); } @Override - public ValueMatcher makeValueMatcher(Predicate predicate, boolean matchNull) + public ValueMatcher makeValueMatcher(Predicate predicate) { return BooleanValueMatcher.of(predicate.apply(null)); } diff --git a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java index f72926d15a6f..dc3d4804a68a 100644 --- a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java @@ -474,15 +474,15 @@ public IndexedInts getRow() } @Override - public ValueMatcher makeValueMatcher(String value, boolean matchNull) + public ValueMatcher makeValueMatcher(String value) { - return DimensionSelectorUtils.makeRowBasedValueMatcher(this, value, matchNull); + return DimensionSelectorUtils.makeValueMatcherGeneric(this, value); } @Override - public ValueMatcher makeValueMatcher(Predicate predicate, boolean matchNull) + public ValueMatcher makeValueMatcher(Predicate predicate) { - return DimensionSelectorUtils.makeRowBasedValueMatcher(this, predicate, matchNull); + return DimensionSelectorUtils.makeValueMatcherGeneric(this, predicate); } @Override @@ -567,7 +567,7 @@ public void close() throws IOException } @Override - public ValueMatcher makeValueMatcher(final String value, boolean matchNull) + public ValueMatcher makeValueMatcher(final String value) { if (extractionFn == null) { final int valueId = lookupId(value); @@ -585,12 +585,12 @@ public boolean matches() } } else { // Employ precomputed BitSet optimization - return makeValueMatcher(Predicates.equalTo(value), matchNull); + return makeValueMatcher(Predicates.equalTo(value)); } } @Override - public ValueMatcher makeValueMatcher(final Predicate predicate, boolean matchNull) + public ValueMatcher makeValueMatcher(final Predicate predicate) { final BitSet predicateMatchingValueIds = DimensionSelectorUtils.makePredicateMatchingSet( this, diff --git a/processing/src/main/java/io/druid/segment/SingleScanTimeDimSelector.java b/processing/src/main/java/io/druid/segment/SingleScanTimeDimSelector.java index 74786ee38a8a..43ffefa3be8f 100644 --- a/processing/src/main/java/io/druid/segment/SingleScanTimeDimSelector.java +++ b/processing/src/main/java/io/druid/segment/SingleScanTimeDimSelector.java @@ -64,7 +64,7 @@ public IndexedInts getRow() } @Override - public ValueMatcher makeValueMatcher(final String value, boolean matchNull) + public ValueMatcher makeValueMatcher(final String value) { return new ValueMatcher() { @@ -77,7 +77,7 @@ public boolean matches() } @Override - public ValueMatcher makeValueMatcher(final Predicate predicate, boolean matchNull) + public ValueMatcher makeValueMatcher(final Predicate predicate) { return new ValueMatcher() { diff --git a/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java b/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java index 89c6db3087ec..0d3aeedd5e96 100644 --- a/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java +++ b/processing/src/main/java/io/druid/segment/StringDimensionIndexer.java @@ -431,11 +431,11 @@ public IndexedInts getRow() } @Override - public ValueMatcher makeValueMatcher(final String value, final boolean matchNull) + public ValueMatcher makeValueMatcher(final String value) { if (extractionFn == null) { final int valueId = lookupId(value); - if (valueId >= 0 || matchNull) { + if (valueId >= 0 || value == null) { return new ValueMatcher() { @Override @@ -443,12 +443,12 @@ public boolean matches() { Object[] dims = currEntry.getKey().getDims(); if (dimIndex >= dims.length) { - return matchNull; + return value == null; } int[] dimsInt = (int[]) dims[dimIndex]; if (dimsInt == null || dimsInt.length == 0) { - return matchNull; + return value == null; } for (int id : dimsInt) { @@ -464,14 +464,15 @@ public boolean matches() } } else { // Employ precomputed BitSet optimization - return makeValueMatcher(Predicates.equalTo(value), matchNull); + return makeValueMatcher(Predicates.equalTo(value)); } } @Override - public ValueMatcher makeValueMatcher(final Predicate predicate, final boolean matchNull) + public ValueMatcher makeValueMatcher(final Predicate predicate) { final BitSet predicateMatchingValueIds = DimensionSelectorUtils.makePredicateMatchingSet(this, predicate); + final boolean matchNull = predicate.apply(null); return new ValueMatcher() { @Override diff --git a/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java index ffbfed8def64..9691ebff8903 100644 --- a/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/FilteredAggregatorTest.java @@ -111,15 +111,15 @@ public IndexedInts getRow() } @Override - public ValueMatcher makeValueMatcher(String value, boolean matchNull) + public ValueMatcher makeValueMatcher(String value) { - return DimensionSelectorUtils.makeRowBasedValueMatcher(this, value, matchNull); + return DimensionSelectorUtils.makeValueMatcherGeneric(this, value); } @Override - public ValueMatcher makeValueMatcher(Predicate predicate, boolean matchNull) + public ValueMatcher makeValueMatcher(Predicate predicate) { - return DimensionSelectorUtils.makeRowBasedValueMatcher(this, predicate, matchNull); + return DimensionSelectorUtils.makeValueMatcherGeneric(this, predicate); } @Override diff --git a/processing/src/test/java/io/druid/query/aggregation/cardinality/CardinalityAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/cardinality/CardinalityAggregatorTest.java index e99707d110f2..af1381d779c0 100644 --- a/processing/src/test/java/io/druid/query/aggregation/cardinality/CardinalityAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/cardinality/CardinalityAggregatorTest.java @@ -162,15 +162,15 @@ public void close() throws IOException } @Override - public ValueMatcher makeValueMatcher(String value, boolean matchNull) + public ValueMatcher makeValueMatcher(String value) { - return DimensionSelectorUtils.makeRowBasedValueMatcher(this, value, matchNull); + return DimensionSelectorUtils.makeValueMatcherGeneric(this, value); } @Override - public ValueMatcher makeValueMatcher(Predicate predicate, boolean matchNull) + public ValueMatcher makeValueMatcher(Predicate predicate) { - return DimensionSelectorUtils.makeRowBasedValueMatcher(this, predicate, matchNull); + return DimensionSelectorUtils.makeValueMatcherGeneric(this, predicate); } @Override diff --git a/processing/src/test/java/io/druid/query/dimension/TestDimensionSelector.java b/processing/src/test/java/io/druid/query/dimension/TestDimensionSelector.java index 1b19e7951c9e..2498b98183ee 100644 --- a/processing/src/test/java/io/druid/query/dimension/TestDimensionSelector.java +++ b/processing/src/test/java/io/druid/query/dimension/TestDimensionSelector.java @@ -50,15 +50,15 @@ public IndexedInts getRow() } @Override - public ValueMatcher makeValueMatcher(String value, boolean matchNull) + public ValueMatcher makeValueMatcher(String value) { - return DimensionSelectorUtils.makeRowBasedValueMatcher(this, value, matchNull); + return DimensionSelectorUtils.makeValueMatcherGeneric(this, value); } @Override - public ValueMatcher makeValueMatcher(Predicate predicate, boolean matchNull) + public ValueMatcher makeValueMatcher(Predicate predicate) { - return DimensionSelectorUtils.makeRowBasedValueMatcher(this, predicate, matchNull); + return DimensionSelectorUtils.makeValueMatcherGeneric(this, predicate); } @Override From 8bb793f111e79fed92c9f4334335e73a357f9c5e Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 25 Jan 2017 00:35:58 -0600 Subject: [PATCH 09/12] Fix --- .../src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java b/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java index b63d830fa236..214be2b44241 100644 --- a/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java +++ b/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java @@ -193,7 +193,7 @@ public BaseArrayProvider( { this.dimSelector = dimSelector; this.idLookup = dimSelector.idLookup(); - if (idLookup == null) { + if (idLookup == null && capabilities.dimensionValuesSorted()) { throw new IAE("Only DimensionSelectors which support idLookup() are supported yet"); } this.query = query; From 258f94e2ce0b0cb295af04cee40e1f336821061a Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 25 Jan 2017 12:35:09 -0600 Subject: [PATCH 10/12] Fix --- .../main/java/io/druid/query/topn/BaseTopNAlgorithm.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java b/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java index 214be2b44241..3ed0334475f2 100644 --- a/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java +++ b/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java @@ -193,9 +193,6 @@ public BaseArrayProvider( { this.dimSelector = dimSelector; this.idLookup = dimSelector.idLookup(); - if (idLookup == null && capabilities.dimensionValuesSorted()) { - throw new IAE("Only DimensionSelectors which support idLookup() are supported yet"); - } this.query = query; this.capabilities = capabilities; @@ -240,6 +237,9 @@ protected Pair computeStartEnd(int cardinality) int startIndex = ignoreFirstN; if (previousStop != null) { + if (idLookup == null) { + throw new IAE("Only DimensionSelectors which support idLookup() are supported yet"); + } int lookupId = idLookup.lookupId(previousStop) + 1; if (lookupId < 0) { lookupId *= -1; From af519bbea99fe33db9276ed8a35bac467ef05b16 Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 25 Jan 2017 12:45:02 -0600 Subject: [PATCH 11/12] Adjust javadoc of DimensionSelector.nameLookupPossibleInAdvance() for SingleScanTimeDimSelector --- .../java/io/druid/segment/DimensionSelector.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/processing/src/main/java/io/druid/segment/DimensionSelector.java b/processing/src/main/java/io/druid/segment/DimensionSelector.java index 516649e8d730..9341b2255adb 100644 --- a/processing/src/main/java/io/druid/segment/DimensionSelector.java +++ b/processing/src/main/java/io/druid/segment/DimensionSelector.java @@ -96,10 +96,15 @@ public interface DimensionSelector extends ColumnValueSelector * Returns true if it is possible to {@link #lookupName(int)} by ids from 0 to {@link #getValueCardinality()} * before the rows with those ids are returned. * - *

Returns false if {@link #lookupName(int)} could be called only with ids, returned from the most recent call of - * {@link #getRow()} on this DimensionSelector, but not earlier and not later. If {@link #lookupName(int)} is called - * with an id, which is not present in the most recent {@link IndexedInts}, returned from {@link #getRow()}, result is - * undefined: exception could be thrown, or null returned, or some other random value. + *

Returns false if {@link #lookupName(int)} could be called with ids, returned from the most recent call of {@link + * #getRow()} on this DimensionSelector, but not earlier. If {@link #getValueCardinality()} of this DimensionSelector + * additionally returns {@link #CARDINALITY_UNKNOWN}, {@code lookupName()} couldn't be called with ids, returned by + * not the most recent call of {@link #getRow()}, i. e. names for ids couldn't be looked up "later". If {@link + * #getValueCardinality()} returns a non-negative number, {@code lookupName()} could be called with any ids, returned + * from {@code #getRow()} since the creation of this DimensionSelector. + * + *

If {@link #lookupName(int)} is called with an ineligible id, result is undefined: exception could be thrown, or + * null returned, or some other random value. */ boolean nameLookupPossibleInAdvance(); From e2a3888c96b829b70dd92afdfb377585d22a6e0c Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 25 Jan 2017 13:13:51 -0600 Subject: [PATCH 12/12] throw UnsupportedOperationException instead of IAE in BaseTopNAlgorithm --- .../src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java b/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java index 3ed0334475f2..678cfe688370 100644 --- a/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java +++ b/processing/src/main/java/io/druid/query/topn/BaseTopNAlgorithm.java @@ -238,7 +238,7 @@ protected Pair computeStartEnd(int cardinality) if (previousStop != null) { if (idLookup == null) { - throw new IAE("Only DimensionSelectors which support idLookup() are supported yet"); + throw new UnsupportedOperationException("Only DimensionSelectors which support idLookup() are supported yet"); } int lookupId = idLookup.lookupId(previousStop) + 1; if (lookupId < 0) {