From 0e8208cc2cbdb807bb76e544e8a02b4e7962368f Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Thu, 24 Aug 2017 16:54:57 -0700 Subject: [PATCH 1/7] Fix dimension selectors with extractionFns on missing columns. This patch properly applies the requested extractionFn to missing columns. It's important when the extractionFn maps null to something other than null. --- .../druid/query/search/SearchQueryRunner.java | 29 ++++-- ...or.java => ConstantDimensionSelector.java} | 45 ++++++--- .../segment/QueryableIndexStorageAdapter.java | 11 ++- .../java/io/druid/segment/VirtualColumns.java | 9 +- .../IncrementalIndexStorageAdapter.java | 6 +- .../druid/query/topn/TopNQueryRunnerTest.java | 56 +++++++++++ .../ConstantDimensionSelectorTest.java | 97 +++++++++++++++++++ .../segment/NullDimensionSelectorTest.java | 65 ------------- 8 files changed, 222 insertions(+), 96 deletions(-) rename processing/src/main/java/io/druid/segment/{NullDimensionSelector.java => ConstantDimensionSelector.java} (64%) create mode 100644 processing/src/test/java/io/druid/segment/ConstantDimensionSelectorTest.java delete mode 100644 processing/src/test/java/io/druid/segment/NullDimensionSelectorTest.java diff --git a/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java b/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java index 7670d5003d78..e3e0e363958f 100644 --- a/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java +++ b/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java @@ -44,7 +44,6 @@ import io.druid.segment.DoubleColumnSelector; import io.druid.segment.FloatColumnSelector; import io.druid.segment.LongColumnSelector; -import io.druid.segment.NullDimensionSelector; import io.druid.segment.Segment; import io.druid.segment.column.ColumnCapabilities; import io.druid.segment.column.ValueType; @@ -132,15 +131,25 @@ public void updateSearchResultSet( final Object2IntRBTreeMap set ) { - if (selector != null && !(selector instanceof NullDimensionSelector)) { - final IndexedInts vals = selector.getRow(); - for (int i = 0; i < vals.size(); ++i) { - final String dimVal = selector.lookupName(vals.get(i)); - if (searchQuerySpec.accept(dimVal)) { - set.addTo(new SearchHit(outputName, Strings.nullToEmpty(dimVal)), 1); - if (set.size() >= limit) { - return; - } + if (selector == null) { + // Column doesn't exist + return; + } + + if (selector.nameLookupPossibleInAdvance() + && selector.getValueCardinality() == 1 + && selector.lookupName(0) == null) { + // Column exists, all values are null + return; + } + + final IndexedInts vals = selector.getRow(); + for (int i = 0; i < vals.size(); ++i) { + final String dimVal = selector.lookupName(vals.get(i)); + if (searchQuerySpec.accept(dimVal)) { + set.addTo(new SearchHit(outputName, Strings.nullToEmpty(dimVal)), 1); + if (set.size() >= limit) { + return; } } } diff --git a/processing/src/main/java/io/druid/segment/NullDimensionSelector.java b/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java similarity index 64% rename from processing/src/main/java/io/druid/segment/NullDimensionSelector.java rename to processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java index ef7fd5838ab2..750006155fa4 100644 --- a/processing/src/main/java/io/druid/segment/NullDimensionSelector.java +++ b/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java @@ -21,6 +21,7 @@ import com.google.common.base.Predicate; import com.google.common.base.Strings; +import io.druid.query.extraction.ExtractionFn; import io.druid.query.filter.ValueMatcher; import io.druid.query.monomorphicprocessing.RuntimeShapeInspector; import io.druid.segment.data.IndexedInts; @@ -29,19 +30,35 @@ import io.druid.segment.historical.SingleValueHistoricalDimensionSelector; import javax.annotation.Nullable; +import java.util.Objects; -public class NullDimensionSelector implements SingleValueHistoricalDimensionSelector, IdLookup +public class ConstantDimensionSelector implements SingleValueHistoricalDimensionSelector, IdLookup { - private static final NullDimensionSelector INSTANCE = new NullDimensionSelector(); + private static final ConstantDimensionSelector NULL_INSTANCE = new ConstantDimensionSelector(null); - private NullDimensionSelector() + private final String value; + + protected ConstantDimensionSelector(final String value) + { + this.value = value; + } + + public static ConstantDimensionSelector of(final String value) { - // Singleton. + if (Strings.isNullOrEmpty(value)) { + return NULL_INSTANCE; + } else { + return new ConstantDimensionSelector(value); + } } - public static NullDimensionSelector instance() + public static ConstantDimensionSelector of(final String value, final ExtractionFn extractionFn) { - return INSTANCE; + if (extractionFn == null) { + return of(value); + } else { + return of(extractionFn.apply(value)); + } } @Override @@ -69,15 +86,15 @@ public IndexedInts getRow(int offset) } @Override - public ValueMatcher makeValueMatcher(String value) + public ValueMatcher makeValueMatcher(String matchValue) { - return BooleanValueMatcher.of(value == null); + return BooleanValueMatcher.of(Objects.equals(value, matchValue)); } @Override public ValueMatcher makeValueMatcher(Predicate predicate) { - return BooleanValueMatcher.of(predicate.apply(null)); + return BooleanValueMatcher.of(predicate.apply(value)); } @Override @@ -90,7 +107,7 @@ public int getValueCardinality() public String lookupName(int id) { assert id == 0 : "id = " + id; - return null; + return value; } @Override @@ -109,12 +126,16 @@ public IdLookup idLookup() @Override public int lookupId(String name) { - return Strings.isNullOrEmpty(name) ? 0 : -1; + if (value == null) { + return Strings.isNullOrEmpty(name) ? 0 : -1; + } else { + return value.equals(name) ? 0 : -1; + } } @Override public void inspectRuntimeShape(RuntimeShapeInspector inspector) { - // nothing to inspect + inspector.visit("value", value); } } diff --git a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java index fb0f7eb6fe55..6914f89c5f08 100644 --- a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java @@ -404,7 +404,10 @@ public Sequence build() public Cursor apply(final Interval inputInterval) { final long timeStart = Math.max(interval.getStartMillis(), inputInterval.getStartMillis()); - final long timeEnd = Math.min(interval.getEndMillis(), gran.increment(inputInterval.getStart()).getMillis()); + final long timeEnd = Math.min( + interval.getEndMillis(), + gran.increment(inputInterval.getStart()).getMillis() + ); if (descending) { for (; baseOffset.withinBounds(); baseOffset.increment()) { @@ -503,7 +506,7 @@ private DimensionSelector makeDimensionSelectorUndecorated( final Column columnDesc = index.getColumn(dimension); if (columnDesc == null) { - return NullDimensionSelector.instance(); + return ConstantDimensionSelector.of(null, extractionFn); } if (dimension.equals(Column.TIME_COLUMN_NAME)) { @@ -534,7 +537,7 @@ private DimensionSelector makeDimensionSelectorUndecorated( final DictionaryEncodedColumn column = cachedColumn; if (column == null) { - return NullDimensionSelector.instance(); + return ConstantDimensionSelector.of(null, extractionFn); } else { return column.makeDimensionSelector(this, extractionFn); } @@ -883,7 +886,7 @@ public void reset() return new QueryableIndexBaseCursor() { private Offset baseOffset; - + { cursorOffset = new FilteredOffset(this, descending, postFilter, bitmapIndexSelector); reset(); diff --git a/processing/src/main/java/io/druid/segment/VirtualColumns.java b/processing/src/main/java/io/druid/segment/VirtualColumns.java index 964ce4544dff..3868bd9b12da 100644 --- a/processing/src/main/java/io/druid/segment/VirtualColumns.java +++ b/processing/src/main/java/io/druid/segment/VirtualColumns.java @@ -142,6 +142,7 @@ public VirtualColumn getVirtualColumn(String columnName) return withDotSupport.get(baseColumnName); } + @Nullable public ObjectColumnSelector makeObjectColumnSelector(String columnName, ColumnSelectorFactory factory) { final VirtualColumn virtualColumn = getVirtualColumn(columnName); @@ -161,10 +162,14 @@ public DimensionSelector makeDimensionSelector(DimensionSpec dimensionSpec, Colu { final VirtualColumn virtualColumn = getVirtualColumn(dimensionSpec.getDimension()); if (virtualColumn == null) { - return dimensionSpec.decorate(NullDimensionSelector.instance()); + return dimensionSpec.decorate(ConstantDimensionSelector.of(null, dimensionSpec.getExtractionFn())); } else { final DimensionSelector selector = virtualColumn.makeDimensionSelector(dimensionSpec, factory); - return selector == null ? dimensionSpec.decorate(NullDimensionSelector.instance()) : selector; + if (selector == null) { + return dimensionSpec.decorate(ConstantDimensionSelector.of(null, dimensionSpec.getExtractionFn())); + } else { + return selector; + } } } 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 e23b0e4d0fb6..417255b9c2ca 100644 --- a/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java @@ -34,6 +34,7 @@ import io.druid.query.filter.ValueMatcher; import io.druid.query.monomorphicprocessing.RuntimeShapeInspector; import io.druid.segment.Capabilities; +import io.druid.segment.ConstantDimensionSelector; import io.druid.segment.Cursor; import io.druid.segment.DimensionHandler; import io.druid.segment.DimensionIndexer; @@ -45,7 +46,6 @@ import io.druid.segment.LongColumnSelector; import io.druid.segment.LongWrappingDimensionSelector; import io.druid.segment.Metadata; -import io.druid.segment.NullDimensionSelector; import io.druid.segment.ObjectColumnSelector; import io.druid.segment.SingleScanTimeDimSelector; import io.druid.segment.StorageAdapter; @@ -418,7 +418,7 @@ private DimensionSelector makeDimensionSelectorUndecorated( // not a dimension, column may be a metric ColumnCapabilities capabilities = getColumnCapabilities(dimension); if (capabilities == null) { - return NullDimensionSelector.instance(); + return ConstantDimensionSelector.of(null, extractionFn); } if (capabilities.getType() == ValueType.LONG) { return new LongWrappingDimensionSelector(makeLongColumnSelector(dimension), extractionFn); @@ -431,7 +431,7 @@ private DimensionSelector makeDimensionSelectorUndecorated( } // if we can't wrap the base column, just return a column of all nulls - return NullDimensionSelector.instance(); + return ConstantDimensionSelector.of(null, extractionFn); } else { final DimensionIndexer indexer = dimensionDesc.getIndexer(); return indexer.makeDimensionSelector(dimensionSpec, currEntry, dimensionDesc); diff --git a/processing/src/test/java/io/druid/query/topn/TopNQueryRunnerTest.java b/processing/src/test/java/io/druid/query/topn/TopNQueryRunnerTest.java index 13c83489629e..8581bfb1feb5 100644 --- a/processing/src/test/java/io/druid/query/topn/TopNQueryRunnerTest.java +++ b/processing/src/test/java/io/druid/query/topn/TopNQueryRunnerTest.java @@ -80,6 +80,7 @@ import io.druid.query.extraction.JavaScriptExtractionFn; import io.druid.query.extraction.MapLookupExtractor; import io.druid.query.extraction.RegexDimExtractionFn; +import io.druid.query.extraction.StringFormatExtractionFn; import io.druid.query.extraction.StrlenExtractionFn; import io.druid.query.extraction.TimeFormatExtractionFn; import io.druid.query.filter.AndDimFilter; @@ -377,6 +378,61 @@ public void testFullOnTopN() ); } + @Test + public void testTopNOnMissingColumn() + { + TopNQuery query = new TopNQueryBuilder() + .dataSource(QueryRunnerTestHelper.dataSource) + .granularity(QueryRunnerTestHelper.allGran) + .dimension(new DefaultDimensionSpec("nonexistentColumn", "alias")) + .metric("rows") + .threshold(4) + .intervals(QueryRunnerTestHelper.fullOnInterval) + .aggregators(Collections.singletonList(new CountAggregatorFactory("rows"))) + .build(); + + final HashMap resultMap = new HashMap<>(); + resultMap.put("alias", null); + resultMap.put("rows", 1209L); + + List> expectedResults = Collections.singletonList( + new Result<>( + DateTimes.of("2011-01-12T00:00:00.000Z"), + new TopNResultValue(Collections.>singletonList(resultMap)) + ) + ); + assertExpectedResults(expectedResults, query); + } + + @Test + public void testTopNOnMissingColumnWithExtractionFn() + { + TopNQuery query = new TopNQueryBuilder() + .dataSource(QueryRunnerTestHelper.dataSource) + .granularity(QueryRunnerTestHelper.allGran) + .dimension(new ExtractionDimensionSpec("nonexistentColumn", "alias", new StringFormatExtractionFn("theValue"))) + .metric("rows") + .threshold(4) + .intervals(QueryRunnerTestHelper.fullOnInterval) + .aggregators(Collections.singletonList(new CountAggregatorFactory("rows"))) + .build(); + + List> expectedResults = Collections.singletonList( + new Result<>( + DateTimes.of("2011-01-12T00:00:00.000Z"), + new TopNResultValue( + Collections.>singletonList( + ImmutableMap.builder() + .put("alias", "theValue") + .put("rows", 1209L) + .build() + ) + ) + ) + ); + assertExpectedResults(expectedResults, query); + } + @Test public void testFullOnTopNOverPostAggs() { diff --git a/processing/src/test/java/io/druid/segment/ConstantDimensionSelectorTest.java b/processing/src/test/java/io/druid/segment/ConstantDimensionSelectorTest.java new file mode 100644 index 000000000000..998fe6144844 --- /dev/null +++ b/processing/src/test/java/io/druid/segment/ConstantDimensionSelectorTest.java @@ -0,0 +1,97 @@ +/* + * 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 io.druid.query.extraction.StringFormatExtractionFn; +import io.druid.query.extraction.SubstringDimExtractionFn; +import io.druid.segment.data.IndexedInts; +import org.junit.Assert; +import org.junit.Test; + +import java.util.Iterator; + +public class ConstantDimensionSelectorTest +{ + private final ConstantDimensionSelector NULL_SELECTOR = ConstantDimensionSelector.of(null); + private final ConstantDimensionSelector CONST_SELECTOR = ConstantDimensionSelector.of("billy"); + private final ConstantDimensionSelector NULL_EXTRACTION_SELECTOR = ConstantDimensionSelector.of( + null, + new StringFormatExtractionFn("billy") + ); + private final ConstantDimensionSelector CONST_EXTRACTION_SELECTOR = ConstantDimensionSelector.of( + "billybilly", + new SubstringDimExtractionFn(0, 5) + ); + + @Test + public void testGetRow() throws Exception + { + IndexedInts row = NULL_SELECTOR.getRow(); + Assert.assertEquals(1, row.size()); + Assert.assertEquals(0, row.get(0)); + + Iterator iter = row.iterator(); + Assert.assertEquals(true, iter.hasNext()); + Assert.assertEquals(0, iter.next().intValue()); + Assert.assertEquals(false, iter.hasNext()); + } + + @Test + public void testGetValueCardinality() throws Exception + { + Assert.assertEquals(1, NULL_SELECTOR.getValueCardinality()); + Assert.assertEquals(1, CONST_SELECTOR.getValueCardinality()); + Assert.assertEquals(1, NULL_EXTRACTION_SELECTOR.getValueCardinality()); + Assert.assertEquals(1, CONST_EXTRACTION_SELECTOR.getValueCardinality()); + } + + @Test + public void testLookupName() throws Exception + { + Assert.assertEquals(null, NULL_SELECTOR.lookupName(0)); + Assert.assertEquals("billy", CONST_SELECTOR.lookupName(0)); + Assert.assertEquals("billy", NULL_EXTRACTION_SELECTOR.lookupName(0)); + Assert.assertEquals("billy", CONST_EXTRACTION_SELECTOR.lookupName(0)); + } + + @Test + public void testLookupId() throws Exception + { + Assert.assertEquals(0, NULL_SELECTOR.idLookup().lookupId(null)); + Assert.assertEquals(0, NULL_SELECTOR.idLookup().lookupId("")); + Assert.assertEquals(-1, NULL_SELECTOR.idLookup().lookupId("billy")); + Assert.assertEquals(-1, NULL_SELECTOR.idLookup().lookupId("bob")); + + Assert.assertEquals(-1, CONST_SELECTOR.idLookup().lookupId(null)); + Assert.assertEquals(-1, CONST_SELECTOR.idLookup().lookupId("")); + Assert.assertEquals(0, CONST_SELECTOR.idLookup().lookupId("billy")); + Assert.assertEquals(-1, CONST_SELECTOR.idLookup().lookupId("bob")); + + Assert.assertEquals(-1, NULL_EXTRACTION_SELECTOR.idLookup().lookupId(null)); + Assert.assertEquals(-1, NULL_EXTRACTION_SELECTOR.idLookup().lookupId("")); + Assert.assertEquals(0, NULL_EXTRACTION_SELECTOR.idLookup().lookupId("billy")); + Assert.assertEquals(-1, NULL_EXTRACTION_SELECTOR.idLookup().lookupId("bob")); + + Assert.assertEquals(-1, CONST_EXTRACTION_SELECTOR.idLookup().lookupId(null)); + Assert.assertEquals(-1, CONST_EXTRACTION_SELECTOR.idLookup().lookupId("")); + Assert.assertEquals(0, CONST_EXTRACTION_SELECTOR.idLookup().lookupId("billy")); + Assert.assertEquals(-1, CONST_EXTRACTION_SELECTOR.idLookup().lookupId("bob")); + } +} diff --git a/processing/src/test/java/io/druid/segment/NullDimensionSelectorTest.java b/processing/src/test/java/io/druid/segment/NullDimensionSelectorTest.java deleted file mode 100644 index dbbee1442086..000000000000 --- a/processing/src/test/java/io/druid/segment/NullDimensionSelectorTest.java +++ /dev/null @@ -1,65 +0,0 @@ -/* - * 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 io.druid.segment.data.IndexedInts; -import org.junit.Assert; -import org.junit.Test; - -import java.util.Iterator; - -public class NullDimensionSelectorTest -{ - - private final NullDimensionSelector selector = NullDimensionSelector.instance(); - - @Test - public void testGetRow() throws Exception - { - IndexedInts row = selector.getRow(); - Assert.assertEquals(1, row.size()); - Assert.assertEquals(0, row.get(0)); - - Iterator iter = row.iterator(); - Assert.assertEquals(true, iter.hasNext()); - Assert.assertEquals(0, iter.next().intValue()); - Assert.assertEquals(false, iter.hasNext()); - } - - @Test - public void testGetValueCardinality() throws Exception - { - Assert.assertEquals(1, selector.getValueCardinality()); - } - - @Test - public void testLookupName() throws Exception - { - Assert.assertEquals(null, selector.lookupName(0)); - } - - @Test - public void testLookupId() throws Exception - { - Assert.assertEquals(0, selector.idLookup().lookupId(null)); - Assert.assertEquals(0, selector.idLookup().lookupId("")); - Assert.assertEquals(-1, selector.idLookup().lookupId("billy")); - } -} From d74286e9243264eae1131a43073ec4196b2b8bca Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Fri, 25 Aug 2017 10:17:29 -0700 Subject: [PATCH 2/7] Extract helper method. --- .../druid/query/search/SearchQueryRunner.java | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java b/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java index e3e0e363958f..2182e28c9553 100644 --- a/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java +++ b/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java @@ -131,31 +131,38 @@ public void updateSearchResultSet( final Object2IntRBTreeMap set ) { - if (selector == null) { - // Column doesn't exist - return; - } - - if (selector.nameLookupPossibleInAdvance() - && selector.getValueCardinality() == 1 - && selector.lookupName(0) == null) { - // Column exists, all values are null - return; - } - - final IndexedInts vals = selector.getRow(); - for (int i = 0; i < vals.size(); ++i) { - final String dimVal = selector.lookupName(vals.get(i)); - if (searchQuerySpec.accept(dimVal)) { - set.addTo(new SearchHit(outputName, Strings.nullToEmpty(dimVal)), 1); - if (set.size() >= limit) { - return; + if (!isNullSelector(selector)) { + final IndexedInts vals = selector.getRow(); + for (int i = 0; i < vals.size(); ++i) { + final String dimVal = selector.lookupName(vals.get(i)); + if (searchQuerySpec.accept(dimVal)) { + set.addTo(new SearchHit(outputName, Strings.nullToEmpty(dimVal)), 1); + if (set.size() >= limit) { + return; + } } } } } } + private static boolean isNullSelector(final DimensionSelector selector) + { + if (selector == null) { + // Column doesn't exist + return true; + } + + if (selector.nameLookupPossibleInAdvance() + && selector.getValueCardinality() == 1 + && selector.lookupName(0) == null) { + // Column exists, all values are null + return true; + } + + return false; + } + public static class LongSearchColumnSelectorStrategy implements SearchColumnSelectorStrategy { @Override From 16699a251ada933938cd001711cb0627f0b4f2f0 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Fri, 25 Aug 2017 13:06:01 -0700 Subject: [PATCH 3/7] Change contracts of VirtualColumns and VirtualColumn methods based on review comments. --- .../io/druid/segment/MapVirtualColumn.java | 8 +- .../druid/query/search/SearchQueryRunner.java | 11 +- .../segment/ConstantDimensionSelector.java | 20 +-- .../druid/segment/DimensionSelectorUtils.java | 20 +++ .../druid/segment/NullDimensionSelector.java | 120 ++++++++++++++++++ .../segment/QueryableIndexStorageAdapter.java | 4 +- .../java/io/druid/segment/VirtualColumn.java | 13 +- .../java/io/druid/segment/VirtualColumns.java | 84 ++++++++++-- .../IncrementalIndexStorageAdapter.java | 6 +- .../query/groupby/GroupByQueryRunnerTest.java | 32 +++++ .../ConstantDimensionSelectorTest.java | 8 +- .../segment/virtual/VirtualColumnsTest.java | 25 +++- 12 files changed, 293 insertions(+), 58 deletions(-) create mode 100644 processing/src/main/java/io/druid/segment/NullDimensionSelector.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 ce019e2246fc..ccfa85154e57 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 @@ -165,25 +165,25 @@ public String get() public DimensionSelector makeDimensionSelector(DimensionSpec dimensionSpec, ColumnSelectorFactory factory) { // Could probably do something useful here if the column name is dot-style. But for now just return nothing. - return null; + return dimensionSpec.decorate(DimensionSelectorUtils.constantSelector(null, dimensionSpec.getExtractionFn())); } @Override public FloatColumnSelector makeFloatColumnSelector(String columnName, ColumnSelectorFactory factory) { - return null; + return ZeroFloatColumnSelector.instance(); } @Override public LongColumnSelector makeLongColumnSelector(String columnName, ColumnSelectorFactory factory) { - return null; + return ZeroLongColumnSelector.instance(); } @Override public DoubleColumnSelector makeDoubleColumnSelector(String columnName, ColumnSelectorFactory factory) { - return null; + return ZeroDoubleColumnSelector.instance(); } @Override diff --git a/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java b/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java index 2182e28c9553..6116007b990e 100644 --- a/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java +++ b/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java @@ -153,14 +153,9 @@ private static boolean isNullSelector(final DimensionSelector selector) return true; } - if (selector.nameLookupPossibleInAdvance() - && selector.getValueCardinality() == 1 - && selector.lookupName(0) == null) { - // Column exists, all values are null - return true; - } - - return false; + return selector.nameLookupPossibleInAdvance() + && selector.getValueCardinality() == 1 + && selector.lookupName(0) == null; } public static class LongSearchColumnSelectorStrategy implements SearchColumnSelectorStrategy diff --git a/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java b/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java index 750006155fa4..4a34199b4fe4 100644 --- a/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java +++ b/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java @@ -34,33 +34,27 @@ public class ConstantDimensionSelector implements SingleValueHistoricalDimensionSelector, IdLookup { - private static final ConstantDimensionSelector NULL_INSTANCE = new ConstantDimensionSelector(null); - private final String value; - protected ConstantDimensionSelector(final String value) + public ConstantDimensionSelector(final String value) { + if (Strings.isNullOrEmpty(value)) { + // There's an optimized implementation for nulls that callers should use instead. + throw new IllegalArgumentException("Use NullDimensionSelector or DimensionSelectorUtils.constantSelector"); + } + this.value = value; } public static ConstantDimensionSelector of(final String value) { if (Strings.isNullOrEmpty(value)) { - return NULL_INSTANCE; + throw new IllegalArgumentException("Use NullDimensionSelector"); } else { return new ConstantDimensionSelector(value); } } - public static ConstantDimensionSelector of(final String value, final ExtractionFn extractionFn) - { - if (extractionFn == null) { - return of(value); - } else { - return of(extractionFn.apply(value)); - } - } - @Override public IndexedInts getRow() { diff --git a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java index 3bf165becb4f..85ec858734e3 100644 --- a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java +++ b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java @@ -21,7 +21,9 @@ import com.google.common.base.Predicate; import com.google.common.base.Predicates; +import com.google.common.base.Strings; import io.druid.java.util.common.IAE; +import io.druid.query.extraction.ExtractionFn; import io.druid.query.filter.ValueMatcher; import io.druid.query.monomorphicprocessing.RuntimeShapeInspector; import io.druid.segment.data.IndexedInts; @@ -246,4 +248,22 @@ public static BitSet makePredicateMatchingSet(DimensionSelector selector, Predic } return valueIds; } + + public static DimensionSelector constantSelector(final String value) + { + if (Strings.isNullOrEmpty(value)) { + return NullDimensionSelector.instance(); + } else { + return new ConstantDimensionSelector(value); + } + } + + public static DimensionSelector constantSelector(final String value, final ExtractionFn extractionFn) + { + if (extractionFn == null) { + return constantSelector(value); + } else { + return constantSelector(extractionFn.apply(value)); + } + } } diff --git a/processing/src/main/java/io/druid/segment/NullDimensionSelector.java b/processing/src/main/java/io/druid/segment/NullDimensionSelector.java new file mode 100644 index 000000000000..ef7fd5838ab2 --- /dev/null +++ b/processing/src/main/java/io/druid/segment/NullDimensionSelector.java @@ -0,0 +1,120 @@ +/* + * 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 com.google.common.base.Strings; +import io.druid.query.filter.ValueMatcher; +import io.druid.query.monomorphicprocessing.RuntimeShapeInspector; +import io.druid.segment.data.IndexedInts; +import io.druid.segment.data.ZeroIndexedInts; +import io.druid.segment.filter.BooleanValueMatcher; +import io.druid.segment.historical.SingleValueHistoricalDimensionSelector; + +import javax.annotation.Nullable; + +public class NullDimensionSelector implements SingleValueHistoricalDimensionSelector, IdLookup +{ + private static final NullDimensionSelector INSTANCE = new NullDimensionSelector(); + + private NullDimensionSelector() + { + // Singleton. + } + + public static NullDimensionSelector instance() + { + return INSTANCE; + } + + @Override + public IndexedInts getRow() + { + return ZeroIndexedInts.instance(); + } + + @Override + public int getRowValue() + { + return 0; + } + + @Override + public int getRowValue(int offset) + { + return 0; + } + + @Override + public IndexedInts getRow(int offset) + { + return getRow(); + } + + @Override + public ValueMatcher makeValueMatcher(String value) + { + return BooleanValueMatcher.of(value == null); + } + + @Override + public ValueMatcher makeValueMatcher(Predicate predicate) + { + return BooleanValueMatcher.of(predicate.apply(null)); + } + + @Override + public int getValueCardinality() + { + return 1; + } + + @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) + { + return Strings.isNullOrEmpty(name) ? 0 : -1; + } + + @Override + public void inspectRuntimeShape(RuntimeShapeInspector inspector) + { + // nothing to inspect + } +} diff --git a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java index 6914f89c5f08..704294f88889 100644 --- a/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java @@ -506,7 +506,7 @@ private DimensionSelector makeDimensionSelectorUndecorated( final Column columnDesc = index.getColumn(dimension); if (columnDesc == null) { - return ConstantDimensionSelector.of(null, extractionFn); + return DimensionSelectorUtils.constantSelector(null, extractionFn); } if (dimension.equals(Column.TIME_COLUMN_NAME)) { @@ -537,7 +537,7 @@ private DimensionSelector makeDimensionSelectorUndecorated( final DictionaryEncodedColumn column = cachedColumn; if (column == null) { - return ConstantDimensionSelector.of(null, extractionFn); + return DimensionSelectorUtils.constantSelector(null, extractionFn); } else { return column.makeDimensionSelector(this, extractionFn); } diff --git a/processing/src/main/java/io/druid/segment/VirtualColumn.java b/processing/src/main/java/io/druid/segment/VirtualColumn.java index 4c49d0a555c0..cbdc7c3fde5a 100644 --- a/processing/src/main/java/io/druid/segment/VirtualColumn.java +++ b/processing/src/main/java/io/druid/segment/VirtualColumn.java @@ -26,7 +26,6 @@ import io.druid.segment.column.ColumnCapabilities; import io.druid.segment.virtual.ExpressionVirtualColumn; -import javax.annotation.Nullable; import java.util.List; /** @@ -69,9 +68,8 @@ public interface VirtualColumn extends Cacheable * @param dimensionSpec the dimensionSpec this column was referenced with * @param factory column selector factory * - * @return the selector, or null if we can't make a selector + * @return the selector, must not be null */ - @Nullable DimensionSelector makeDimensionSelector(DimensionSpec dimensionSpec, ColumnSelectorFactory factory); /** @@ -81,9 +79,8 @@ public interface VirtualColumn extends Cacheable * @param columnName the name this virtual column was referenced with * @param factory column selector factory * - * @return the selector, or null if we can't make a selector + * @return the selector, must not be null */ - @Nullable FloatColumnSelector makeFloatColumnSelector(String columnName, ColumnSelectorFactory factory); /** @@ -93,9 +90,8 @@ public interface VirtualColumn extends Cacheable * @param columnName the name this virtual column was referenced with * @param factory column selector factory * - * @return the selector, or null if we can't make a selector + * @return the selector, must not be null */ - @Nullable LongColumnSelector makeLongColumnSelector(String columnName, ColumnSelectorFactory factory); /** @@ -105,9 +101,8 @@ public interface VirtualColumn extends Cacheable * @param columnName the name this virtual column was referenced with * @param factory column selector factory * - * @return the selector, or null if we can't make a selector + * @return the selector, must not be null */ - @Nullable DoubleColumnSelector makeDoubleColumnSelector(String columnName, ColumnSelectorFactory factory); /** diff --git a/processing/src/main/java/io/druid/segment/VirtualColumns.java b/processing/src/main/java/io/druid/segment/VirtualColumns.java index 3868bd9b12da..ef7003455a7e 100644 --- a/processing/src/main/java/io/druid/segment/VirtualColumns.java +++ b/processing/src/main/java/io/druid/segment/VirtualColumns.java @@ -127,6 +127,13 @@ private VirtualColumns( private final Map withDotSupport; private final Map withoutDotSupport; + /** + * Returns true if a virtual column exists with a particular columnName. + * + * @param columnName the column name + * + * @return true or false + */ public boolean exists(String columnName) { return getVirtualColumn(columnName) != null; @@ -142,12 +149,21 @@ public VirtualColumn getVirtualColumn(String columnName) return withDotSupport.get(baseColumnName); } - @Nullable + /** + * Create an object selector. + * + * @param columnName column mame + * @param factory base column selector factory + * + * @return selector + * + * @throws IllegalArgumentException if the virtual column does not exist (see {@link #exists(String)} + */ public ObjectColumnSelector makeObjectColumnSelector(String columnName, ColumnSelectorFactory factory) { final VirtualColumn virtualColumn = getVirtualColumn(columnName); if (virtualColumn == null) { - return null; + throw new IAE("No such virtual column[%s]", columnName); } else { return Preconditions.checkNotNull( virtualColumn.makeObjectColumnSelector(columnName, factory), @@ -158,43 +174,82 @@ public ObjectColumnSelector makeObjectColumnSelector(String columnName, ColumnSe } } + /** + * Create a dimension (string) selector. + * + * @param dimensionSpec the dimensionSpec for this selector + * @param factory base column selector factory + * + * @return selector + * + * @throws IllegalArgumentException if the virtual column does not exist (see {@link #exists(String)} + */ public DimensionSelector makeDimensionSelector(DimensionSpec dimensionSpec, ColumnSelectorFactory factory) { final VirtualColumn virtualColumn = getVirtualColumn(dimensionSpec.getDimension()); if (virtualColumn == null) { - return dimensionSpec.decorate(ConstantDimensionSelector.of(null, dimensionSpec.getExtractionFn())); + throw new IAE("No such virtual column[%s]", dimensionSpec.getDimension()); } else { final DimensionSelector selector = virtualColumn.makeDimensionSelector(dimensionSpec, factory); - if (selector == null) { - return dimensionSpec.decorate(ConstantDimensionSelector.of(null, dimensionSpec.getExtractionFn())); - } else { - return selector; - } + Preconditions.checkNotNull(selector, "selector"); + return selector; } } + /** + * Create a float selector. + * + * @param columnName column mame + * @param factory base column selector factory + * + * @return selector + * + * @throws IllegalArgumentException if the virtual column does not exist (see {@link #exists(String)} + */ public FloatColumnSelector makeFloatColumnSelector(String columnName, ColumnSelectorFactory factory) { final VirtualColumn virtualColumn = getVirtualColumn(columnName); if (virtualColumn == null) { - return ZeroFloatColumnSelector.instance(); + throw new IAE("No such virtual column[%s]", columnName); } else { final FloatColumnSelector selector = virtualColumn.makeFloatColumnSelector(columnName, factory); - return selector == null ? ZeroFloatColumnSelector.instance() : selector; + Preconditions.checkNotNull(selector, "selector"); + return selector; } } + /** + * Create a long selector. + * + * @param columnName column mame + * @param factory base column selector factory + * + * @return selector + * + * @throws IllegalArgumentException if the virtual column does not exist (see {@link #exists(String)} + */ public LongColumnSelector makeLongColumnSelector(String columnName, ColumnSelectorFactory factory) { final VirtualColumn virtualColumn = getVirtualColumn(columnName); if (virtualColumn == null) { - return ZeroLongColumnSelector.instance(); + throw new IAE("No such virtual column[%s]", columnName); } else { final LongColumnSelector selector = virtualColumn.makeLongColumnSelector(columnName, factory); - return selector == null ? ZeroLongColumnSelector.instance() : selector; + Preconditions.checkNotNull(selector, "selector"); + return selector; } } + /** + * Create a double selector. + * + * @param columnName column mame + * @param factory base column selector factory + * + * @return selector + * + * @throws IllegalArgumentException if the virtual column does not exist (see {@link #exists(String)} + */ public DoubleColumnSelector makeDoubleColumnSelector( String columnName, ColumnSelectorFactory factory @@ -202,10 +257,11 @@ public DoubleColumnSelector makeDoubleColumnSelector( { final VirtualColumn virtualColumn = getVirtualColumn(columnName); if (virtualColumn == null) { - return ZeroDoubleColumnSelector.instance(); + throw new IAE("No such virtual column[%s]", columnName); } else { final DoubleColumnSelector selector = virtualColumn.makeDoubleColumnSelector(columnName, factory); - return selector == null ? ZeroDoubleColumnSelector.instance() : selector; + Preconditions.checkNotNull(selector, "selector"); + return selector; } } 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 417255b9c2ca..e0c01ef10d01 100644 --- a/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java +++ b/processing/src/main/java/io/druid/segment/incremental/IncrementalIndexStorageAdapter.java @@ -34,11 +34,11 @@ import io.druid.query.filter.ValueMatcher; import io.druid.query.monomorphicprocessing.RuntimeShapeInspector; import io.druid.segment.Capabilities; -import io.druid.segment.ConstantDimensionSelector; import io.druid.segment.Cursor; import io.druid.segment.DimensionHandler; import io.druid.segment.DimensionIndexer; import io.druid.segment.DimensionSelector; +import io.druid.segment.DimensionSelectorUtils; import io.druid.segment.DoubleColumnSelector; import io.druid.segment.DoubleWrappingDimensionSelector; import io.druid.segment.FloatColumnSelector; @@ -418,7 +418,7 @@ private DimensionSelector makeDimensionSelectorUndecorated( // not a dimension, column may be a metric ColumnCapabilities capabilities = getColumnCapabilities(dimension); if (capabilities == null) { - return ConstantDimensionSelector.of(null, extractionFn); + return DimensionSelectorUtils.constantSelector(null, extractionFn); } if (capabilities.getType() == ValueType.LONG) { return new LongWrappingDimensionSelector(makeLongColumnSelector(dimension), extractionFn); @@ -431,7 +431,7 @@ private DimensionSelector makeDimensionSelectorUndecorated( } // if we can't wrap the base column, just return a column of all nulls - return ConstantDimensionSelector.of(null, extractionFn); + return DimensionSelectorUtils.constantSelector(null, extractionFn); } else { final DimensionIndexer indexer = dimensionDesc.getIndexer(); return indexer.makeDimensionSelector(dimensionSpec, currEntry, dimensionDesc); diff --git a/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java b/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java index 93268c1fb541..439dc6fc34ca 100644 --- a/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java +++ b/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java @@ -94,6 +94,7 @@ import io.druid.query.extraction.JavaScriptExtractionFn; import io.druid.query.extraction.MapLookupExtractor; import io.druid.query.extraction.RegexDimExtractionFn; +import io.druid.query.extraction.StringFormatExtractionFn; import io.druid.query.extraction.StrlenExtractionFn; import io.druid.query.extraction.TimeFormatExtractionFn; import io.druid.query.filter.AndDimFilter; @@ -143,6 +144,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.Comparator; import java.util.HashMap; import java.util.Iterator; @@ -453,6 +455,36 @@ public void testGroupBy() TestHelper.assertExpectedObjects(expectedResults, results, ""); } + @Test + public void testGroupByOnMissingColumn() + { + GroupByQuery query = GroupByQuery + .builder() + .setDataSource(QueryRunnerTestHelper.dataSource) + .setQuerySegmentSpec(QueryRunnerTestHelper.firstToThird) + .setDimensions( + Lists.newArrayList( + new DefaultDimensionSpec("nonexistent0", "alias0"), + new ExtractionDimensionSpec("nonexistent1", "alias1", new StringFormatExtractionFn("foo")) + ) + ) + .setAggregatorSpecs(Collections.singletonList(QueryRunnerTestHelper.rowsCount)) + .setGranularity(QueryRunnerTestHelper.allGran) + .build(); + + List expectedResults = Collections.singletonList( + GroupByQueryRunnerTestHelper.createExpectedRow( + "2011-04-01", + "alias0", null, + "alias1", "foo", + "rows", 26L + ) + ); + + Iterable results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, query); + TestHelper.assertExpectedObjects(expectedResults, results, ""); + } + @Test public void testGroupByWithStringPostAggregator() { diff --git a/processing/src/test/java/io/druid/segment/ConstantDimensionSelectorTest.java b/processing/src/test/java/io/druid/segment/ConstantDimensionSelectorTest.java index 998fe6144844..344c4938c8f4 100644 --- a/processing/src/test/java/io/druid/segment/ConstantDimensionSelectorTest.java +++ b/processing/src/test/java/io/druid/segment/ConstantDimensionSelectorTest.java @@ -29,13 +29,13 @@ public class ConstantDimensionSelectorTest { - private final ConstantDimensionSelector NULL_SELECTOR = ConstantDimensionSelector.of(null); - private final ConstantDimensionSelector CONST_SELECTOR = ConstantDimensionSelector.of("billy"); - private final ConstantDimensionSelector NULL_EXTRACTION_SELECTOR = ConstantDimensionSelector.of( + private final DimensionSelector NULL_SELECTOR = DimensionSelectorUtils.constantSelector(null); + private final DimensionSelector CONST_SELECTOR = DimensionSelectorUtils.constantSelector("billy"); + private final DimensionSelector NULL_EXTRACTION_SELECTOR = DimensionSelectorUtils.constantSelector( null, new StringFormatExtractionFn("billy") ); - private final ConstantDimensionSelector CONST_EXTRACTION_SELECTOR = ConstantDimensionSelector.of( + private final DimensionSelector CONST_EXTRACTION_SELECTOR = DimensionSelectorUtils.constantSelector( "billybilly", new SubstringDimExtractionFn(0, 5) ); diff --git a/processing/src/test/java/io/druid/segment/virtual/VirtualColumnsTest.java b/processing/src/test/java/io/druid/segment/virtual/VirtualColumnsTest.java index ef860cc3a4de..d2a27b3c8331 100644 --- a/processing/src/test/java/io/druid/segment/virtual/VirtualColumnsTest.java +++ b/processing/src/test/java/io/druid/segment/virtual/VirtualColumnsTest.java @@ -65,6 +65,28 @@ public class VirtualColumnsTest @Rule public ExpectedException expectedException = ExpectedException.none(); + @Test + public void testExists() + { + final VirtualColumns virtualColumns = makeVirtualColumns(); + + Assert.assertTrue(virtualColumns.exists("expr")); + Assert.assertTrue(virtualColumns.exists("foo")); + Assert.assertTrue(virtualColumns.exists("foo.5")); + Assert.assertFalse(virtualColumns.exists("bar")); + } + + @Test + public void testNonExistentSelector() + { + final VirtualColumns virtualColumns = makeVirtualColumns(); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("No such virtual column[bar]"); + + virtualColumns.makeObjectColumnSelector("bar", null); + } + @Test public void testMakeSelectors() { @@ -406,7 +428,8 @@ public long getLong() public DoubleColumnSelector makeDoubleColumnSelector(String columnName, ColumnSelectorFactory factory) { final ColumnValueSelector selector = makeLongColumnSelector(columnName, factory); - return new TestDoubleColumnSelector() { + return new TestDoubleColumnSelector() + { @Override public double getDouble() From f2cdcd943897fc8975e39cdbe3b31591eb308612 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Fri, 25 Aug 2017 13:16:21 -0700 Subject: [PATCH 4/7] Remove unused import. --- .../main/java/io/druid/segment/ConstantDimensionSelector.java | 1 - 1 file changed, 1 deletion(-) diff --git a/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java b/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java index 4a34199b4fe4..e76430bc7a5e 100644 --- a/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java +++ b/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java @@ -21,7 +21,6 @@ import com.google.common.base.Predicate; import com.google.common.base.Strings; -import io.druid.query.extraction.ExtractionFn; import io.druid.query.filter.ValueMatcher; import io.druid.query.monomorphicprocessing.RuntimeShapeInspector; import io.druid.segment.data.IndexedInts; From a5844b0d252518200738cb5d30b5390712c7a3dd Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Fri, 25 Aug 2017 13:16:48 -0700 Subject: [PATCH 5/7] Remove unused method. --- .../java/io/druid/segment/ConstantDimensionSelector.java | 9 --------- 1 file changed, 9 deletions(-) diff --git a/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java b/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java index e76430bc7a5e..062160a996a1 100644 --- a/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java +++ b/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java @@ -45,15 +45,6 @@ public ConstantDimensionSelector(final String value) this.value = value; } - public static ConstantDimensionSelector of(final String value) - { - if (Strings.isNullOrEmpty(value)) { - throw new IllegalArgumentException("Use NullDimensionSelector"); - } else { - return new ConstantDimensionSelector(value); - } - } - @Override public IndexedInts getRow() { From 2519f81d5a975aa2f50ba6af70a4f744204966bb Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Fri, 25 Aug 2017 13:26:21 -0700 Subject: [PATCH 6/7] Adjust helper function. --- .../java/io/druid/query/search/SearchQueryRunner.java | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java b/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java index 6116007b990e..fc849bacf25d 100644 --- a/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java +++ b/processing/src/main/java/io/druid/query/search/SearchQueryRunner.java @@ -131,7 +131,7 @@ public void updateSearchResultSet( final Object2IntRBTreeMap set ) { - if (!isNullSelector(selector)) { + if (selector != null && !isNilSelector(selector)) { final IndexedInts vals = selector.getRow(); for (int i = 0; i < vals.size(); ++i) { final String dimVal = selector.lookupName(vals.get(i)); @@ -146,13 +146,8 @@ public void updateSearchResultSet( } } - private static boolean isNullSelector(final DimensionSelector selector) + private static boolean isNilSelector(final DimensionSelector selector) { - if (selector == null) { - // Column doesn't exist - return true; - } - return selector.nameLookupPossibleInAdvance() && selector.getValueCardinality() == 1 && selector.lookupName(0) == null; From 5a90ae721531d434c14274a73b6e6ca77dbbce2f Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Fri, 25 Aug 2017 14:58:06 -0700 Subject: [PATCH 7/7] Adjustments --- .../java/io/druid/segment/ConstantDimensionSelector.java | 6 +----- .../java/io/druid/segment/DimensionSelectorUtils.java | 8 ++++++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java b/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java index 062160a996a1..f61e8b6e1ebf 100644 --- a/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java +++ b/processing/src/main/java/io/druid/segment/ConstantDimensionSelector.java @@ -110,11 +110,7 @@ public IdLookup idLookup() @Override public int lookupId(String name) { - if (value == null) { - return Strings.isNullOrEmpty(name) ? 0 : -1; - } else { - return value.equals(name) ? 0 : -1; - } + return value.equals(name) ? 0 : -1; } @Override diff --git a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java index 85ec858734e3..f83be70241b7 100644 --- a/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java +++ b/processing/src/main/java/io/druid/segment/DimensionSelectorUtils.java @@ -29,6 +29,7 @@ import io.druid.segment.data.IndexedInts; import io.druid.segment.filter.BooleanValueMatcher; +import javax.annotation.Nullable; import java.util.BitSet; import java.util.Objects; @@ -249,7 +250,7 @@ public static BitSet makePredicateMatchingSet(DimensionSelector selector, Predic return valueIds; } - public static DimensionSelector constantSelector(final String value) + public static DimensionSelector constantSelector(@Nullable final String value) { if (Strings.isNullOrEmpty(value)) { return NullDimensionSelector.instance(); @@ -258,7 +259,10 @@ public static DimensionSelector constantSelector(final String value) } } - public static DimensionSelector constantSelector(final String value, final ExtractionFn extractionFn) + public static DimensionSelector constantSelector( + @Nullable final String value, + @Nullable final ExtractionFn extractionFn + ) { if (extractionFn == null) { return constantSelector(value);