From 1c9ca55247f7d90cf15d65e43aba0c1853d85629 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Tue, 9 Jun 2020 19:32:16 -0700 Subject: [PATCH] remove incorrect and unnecessary overrides from BooleanVectorValueMatcher (#9994) * remove incorrect and unnecessary overrides from BooleanVectorValueMatcher * add test case * add unit tests for ... part of VectorValueMatcherColumnProcessorFactory * Update VectorValueMatcherColumnProcessorFactoryTest.java --- .../vector/BooleanVectorValueMatcher.java | 14 - ...alueMatcherColumnProcessorFactoryTest.java | 277 ++++++++++++++++++ .../query/groupby/GroupByQueryRunnerTest.java | 30 ++ 3 files changed, 307 insertions(+), 14 deletions(-) create mode 100644 processing/src/test/java/org/apache/druid/query/filter/vector/VectorValueMatcherColumnProcessorFactoryTest.java diff --git a/processing/src/main/java/org/apache/druid/query/filter/vector/BooleanVectorValueMatcher.java b/processing/src/main/java/org/apache/druid/query/filter/vector/BooleanVectorValueMatcher.java index 65af27b83fc..efa0236acfd 100644 --- a/processing/src/main/java/org/apache/druid/query/filter/vector/BooleanVectorValueMatcher.java +++ b/processing/src/main/java/org/apache/druid/query/filter/vector/BooleanVectorValueMatcher.java @@ -23,13 +23,11 @@ import org.apache.druid.segment.vector.VectorSizeInspector; public class BooleanVectorValueMatcher extends BaseVectorValueMatcher { - private final VectorSizeInspector selector; private final boolean matches; private BooleanVectorValueMatcher(final VectorSizeInspector selector, final boolean matches) { super(selector); - this.selector = selector; this.matches = matches; } @@ -38,18 +36,6 @@ public class BooleanVectorValueMatcher extends BaseVectorValueMatcher return new BooleanVectorValueMatcher(selector, matches); } - @Override - public int getCurrentVectorSize() - { - return selector.getCurrentVectorSize(); - } - - @Override - public int getMaxVectorSize() - { - return selector.getCurrentVectorSize(); - } - @Override public ReadableVectorMatch match(final ReadableVectorMatch mask) { diff --git a/processing/src/test/java/org/apache/druid/query/filter/vector/VectorValueMatcherColumnProcessorFactoryTest.java b/processing/src/test/java/org/apache/druid/query/filter/vector/VectorValueMatcherColumnProcessorFactoryTest.java new file mode 100644 index 00000000000..ffa0253b2cf --- /dev/null +++ b/processing/src/test/java/org/apache/druid/query/filter/vector/VectorValueMatcherColumnProcessorFactoryTest.java @@ -0,0 +1,277 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.query.filter.vector; + +import org.apache.druid.common.config.NullHandling; +import org.apache.druid.segment.IdLookup; +import org.apache.druid.segment.vector.MultiValueDimensionVectorSelector; +import org.apache.druid.segment.vector.SingleValueDimensionVectorSelector; +import org.apache.druid.segment.vector.VectorValueSelector; +import org.apache.druid.testing.InitializedNullHandlingTest; +import org.easymock.EasyMock; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +public class VectorValueMatcherColumnProcessorFactoryTest extends InitializedNullHandlingTest +{ + private static final int VECTOR_SIZE = 128; + private static final int CURRENT_SIZE = 24; + private VectorValueSelector vectorValueSelector; + + @Before + public void setup() + { + vectorValueSelector = EasyMock.createMock(VectorValueSelector.class); + EasyMock.expect(vectorValueSelector.getCurrentVectorSize()).andReturn(CURRENT_SIZE).anyTimes(); + EasyMock.expect(vectorValueSelector.getMaxVectorSize()).andReturn(VECTOR_SIZE).anyTimes(); + EasyMock.replay(vectorValueSelector); + } + + @Test + public void testFloat() + { + VectorValueMatcherFactory matcherFactory = + VectorValueMatcherColumnProcessorFactory.instance().makeFloatProcessor(vectorValueSelector); + + Assert.assertTrue(matcherFactory instanceof FloatVectorValueMatcher); + + VectorValueMatcher matcher = matcherFactory.makeMatcher("2.0"); + Assert.assertFalse(matcher instanceof BooleanVectorValueMatcher); + Assert.assertEquals(VECTOR_SIZE, matcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, matcher.getCurrentVectorSize()); + + // in default mode, matching null produces a boolean matcher + VectorValueMatcher booleanMatcher = matcherFactory.makeMatcher((String) null); + if (NullHandling.replaceWithDefault()) { + Assert.assertTrue(booleanMatcher instanceof BooleanVectorValueMatcher); + } else { + Assert.assertFalse(booleanMatcher instanceof BooleanVectorValueMatcher); + } + Assert.assertEquals(VECTOR_SIZE, booleanMatcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, booleanMatcher.getCurrentVectorSize()); + EasyMock.verify(vectorValueSelector); + } + + @Test + public void testDouble() + { + VectorValueMatcherFactory matcherFactory = + VectorValueMatcherColumnProcessorFactory.instance().makeDoubleProcessor(vectorValueSelector); + + Assert.assertTrue(matcherFactory instanceof DoubleVectorValueMatcher); + + + VectorValueMatcher matcher = matcherFactory.makeMatcher("1.0"); + Assert.assertFalse(matcher instanceof BooleanVectorValueMatcher); + Assert.assertEquals(VECTOR_SIZE, matcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, matcher.getCurrentVectorSize()); + + // in default mode, matching null produces a boolean matcher + VectorValueMatcher booleanMatcher = matcherFactory.makeMatcher((String) null); + if (NullHandling.replaceWithDefault()) { + Assert.assertTrue(booleanMatcher instanceof BooleanVectorValueMatcher); + } else { + Assert.assertFalse(booleanMatcher instanceof BooleanVectorValueMatcher); + } + Assert.assertEquals(VECTOR_SIZE, booleanMatcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, booleanMatcher.getCurrentVectorSize()); + EasyMock.verify(vectorValueSelector); + } + + @Test + public void testLong() + { + VectorValueMatcherFactory matcherFactory = + VectorValueMatcherColumnProcessorFactory.instance().makeLongProcessor(vectorValueSelector); + + Assert.assertTrue(matcherFactory instanceof LongVectorValueMatcher); + + VectorValueMatcher matcher = matcherFactory.makeMatcher("1"); + Assert.assertFalse(matcher instanceof BooleanVectorValueMatcher); + Assert.assertEquals(VECTOR_SIZE, matcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, matcher.getCurrentVectorSize()); + + // in default mode, matching null produces a boolean matcher + VectorValueMatcher booleanMatcher = matcherFactory.makeMatcher((String) null); + if (NullHandling.replaceWithDefault()) { + Assert.assertTrue(booleanMatcher instanceof BooleanVectorValueMatcher); + } else { + Assert.assertFalse(booleanMatcher instanceof BooleanVectorValueMatcher); + } + Assert.assertEquals(VECTOR_SIZE, booleanMatcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, booleanMatcher.getCurrentVectorSize()); + EasyMock.verify(vectorValueSelector); + } + + @Test + public void testSingleValueString() + { + IdLookup lookup = EasyMock.createMock(IdLookup.class); + SingleValueDimensionVectorSelector selector = + EasyMock.createMock(SingleValueDimensionVectorSelector.class); + EasyMock.expect(selector.getCurrentVectorSize()).andReturn(CURRENT_SIZE).anyTimes(); + EasyMock.expect(selector.getMaxVectorSize()).andReturn(VECTOR_SIZE).anyTimes(); + EasyMock.expect(selector.getValueCardinality()).andReturn(1024).anyTimes(); + EasyMock.expect(selector.nameLookupPossibleInAdvance()).andReturn(false).anyTimes(); + EasyMock.expect(selector.idLookup()).andReturn(lookup).anyTimes(); + EasyMock.expect(lookup.lookupId("any value")).andReturn(1).anyTimes(); + EasyMock.expect(lookup.lookupId("another value")).andReturn(-1).anyTimes(); + EasyMock.replay(selector, lookup); + + VectorValueMatcherFactory matcherFactory = + VectorValueMatcherColumnProcessorFactory.instance().makeSingleValueDimensionProcessor(selector); + + Assert.assertTrue(matcherFactory instanceof SingleValueStringVectorValueMatcher); + + // value exists in column nonboolean matcher + VectorValueMatcher matcher = matcherFactory.makeMatcher("any value"); + Assert.assertFalse(matcher instanceof BooleanVectorValueMatcher); + Assert.assertEquals(VECTOR_SIZE, matcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, matcher.getCurrentVectorSize()); + + // value not exist in dictionary uses boolean matcher + VectorValueMatcher booleanMatcher = matcherFactory.makeMatcher("another value"); + Assert.assertTrue(booleanMatcher instanceof BooleanVectorValueMatcher); + Assert.assertEquals(VECTOR_SIZE, booleanMatcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, booleanMatcher.getCurrentVectorSize()); + EasyMock.verify(selector, lookup); + } + + @Test + public void testSingleValueStringZeroCardinalityAlwaysBooleanMatcher() + { + // cardinality 0 has special path to always use boolean matcher + SingleValueDimensionVectorSelector selector = + EasyMock.createMock(SingleValueDimensionVectorSelector.class); + EasyMock.expect(selector.getCurrentVectorSize()).andReturn(CURRENT_SIZE).anyTimes(); + EasyMock.expect(selector.getMaxVectorSize()).andReturn(VECTOR_SIZE).anyTimes(); + EasyMock.expect(selector.getValueCardinality()).andReturn(0).anyTimes(); + EasyMock.replay(selector); + + VectorValueMatcherFactory matcherFactory = + VectorValueMatcherColumnProcessorFactory.instance().makeSingleValueDimensionProcessor(selector); + + Assert.assertTrue(matcherFactory instanceof SingleValueStringVectorValueMatcher); + + VectorValueMatcher matcher = matcherFactory.makeMatcher("any value"); + Assert.assertTrue(matcher instanceof BooleanVectorValueMatcher); + Assert.assertEquals(VECTOR_SIZE, matcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, matcher.getCurrentVectorSize()); + + // all are boolean with no valued column i guess + VectorValueMatcher anotherMatcher = matcherFactory.makeMatcher((String) null); + Assert.assertTrue(anotherMatcher instanceof BooleanVectorValueMatcher); + Assert.assertEquals(VECTOR_SIZE, anotherMatcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, anotherMatcher.getCurrentVectorSize()); + EasyMock.verify(selector); + } + + @Test + public void testSingleValueStringOneCardinalityBooleanMatcherIfNullAndNameLookupPossible() + { + // single value string column with cardinality 1 and name lookup possible in advance uses boolean matcher for + // matches + SingleValueDimensionVectorSelector selector = + EasyMock.createMock(SingleValueDimensionVectorSelector.class); + EasyMock.expect(selector.getCurrentVectorSize()).andReturn(CURRENT_SIZE).anyTimes(); + EasyMock.expect(selector.getMaxVectorSize()).andReturn(VECTOR_SIZE).anyTimes(); + EasyMock.expect(selector.getValueCardinality()).andReturn(1).anyTimes(); + EasyMock.expect(selector.nameLookupPossibleInAdvance()).andReturn(true).anyTimes(); + EasyMock.expect(selector.lookupName(0)).andReturn(null).anyTimes(); + EasyMock.replay(selector); + + VectorValueMatcherFactory matcherFactory = + VectorValueMatcherColumnProcessorFactory.instance().makeSingleValueDimensionProcessor(selector); + + Assert.assertTrue(matcherFactory instanceof SingleValueStringVectorValueMatcher); + + // false matcher + VectorValueMatcher booleanMatcher = matcherFactory.makeMatcher("any value"); + Assert.assertTrue(booleanMatcher instanceof BooleanVectorValueMatcher); + Assert.assertEquals(VECTOR_SIZE, booleanMatcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, booleanMatcher.getCurrentVectorSize()); + + // true matcher + VectorValueMatcher anotherBooleanMatcher = matcherFactory.makeMatcher((String) null); + Assert.assertTrue(anotherBooleanMatcher instanceof BooleanVectorValueMatcher); + Assert.assertEquals(VECTOR_SIZE, anotherBooleanMatcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, anotherBooleanMatcher.getCurrentVectorSize()); + EasyMock.verify(selector); + } + + @Test + public void testSingleValueStringOneCardinalityBooleanMatcherIfNullAndNameLookupNotPossible() + { + // if name lookup not possible in advance, use normal path, even if cardinality 1 + IdLookup lookup = EasyMock.createMock(IdLookup.class); + SingleValueDimensionVectorSelector selector = + EasyMock.createMock(SingleValueDimensionVectorSelector.class); + EasyMock.expect(selector.getCurrentVectorSize()).andReturn(CURRENT_SIZE).anyTimes(); + EasyMock.expect(selector.getMaxVectorSize()).andReturn(VECTOR_SIZE).anyTimes(); + EasyMock.expect(selector.getValueCardinality()).andReturn(1).anyTimes(); + EasyMock.expect(selector.nameLookupPossibleInAdvance()).andReturn(false).anyTimes(); + EasyMock.expect(selector.idLookup()).andReturn(lookup).anyTimes(); + EasyMock.expect(lookup.lookupId("any value")).andReturn(1).anyTimes(); + EasyMock.expect(lookup.lookupId(null)).andReturn(0).anyTimes(); + EasyMock.replay(selector, lookup); + + VectorValueMatcherFactory matcherFactory = + VectorValueMatcherColumnProcessorFactory.instance().makeSingleValueDimensionProcessor(selector); + + Assert.assertTrue(matcherFactory instanceof SingleValueStringVectorValueMatcher); + + VectorValueMatcher matcher = matcherFactory.makeMatcher("any value"); + Assert.assertFalse(matcher instanceof BooleanVectorValueMatcher); + Assert.assertEquals(VECTOR_SIZE, matcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, matcher.getCurrentVectorSize()); + EasyMock.verify(selector, lookup); + } + + @Test + public void testMultiValueString() + { + IdLookup lookup = EasyMock.createMock(IdLookup.class); + MultiValueDimensionVectorSelector selector = EasyMock.createMock(MultiValueDimensionVectorSelector.class); + EasyMock.expect(selector.getCurrentVectorSize()).andReturn(CURRENT_SIZE).anyTimes(); + EasyMock.expect(selector.getMaxVectorSize()).andReturn(VECTOR_SIZE).anyTimes(); + EasyMock.expect(selector.getValueCardinality()).andReturn(11).anyTimes(); + EasyMock.expect(selector.nameLookupPossibleInAdvance()).andReturn(false).anyTimes(); + EasyMock.expect(selector.idLookup()).andReturn(lookup).anyTimes(); + EasyMock.expect(lookup.lookupId("any value")).andReturn(-1).anyTimes(); + EasyMock.expect(lookup.lookupId(null)).andReturn(0).anyTimes(); + EasyMock.replay(selector, lookup); + VectorValueMatcherFactory matcherFactory = + VectorValueMatcherColumnProcessorFactory.instance().makeMultiValueDimensionProcessor(selector); + + Assert.assertTrue(matcherFactory instanceof MultiValueStringVectorValueMatcher); + + VectorValueMatcher valueNotExistMatcher = matcherFactory.makeMatcher("any value"); + Assert.assertTrue(valueNotExistMatcher instanceof BooleanVectorValueMatcher); + Assert.assertEquals(VECTOR_SIZE, valueNotExistMatcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, valueNotExistMatcher.getCurrentVectorSize()); + + VectorValueMatcher valueExistMatcher = matcherFactory.makeMatcher((String) null); + Assert.assertFalse(valueExistMatcher instanceof BooleanVectorValueMatcher); + Assert.assertEquals(VECTOR_SIZE, valueExistMatcher.getMaxVectorSize()); + Assert.assertEquals(CURRENT_SIZE, valueExistMatcher.getCurrentVectorSize()); + EasyMock.verify(selector, lookup); + } +} diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java index 0417d4a430b..6719ac62232 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java @@ -102,6 +102,7 @@ import org.apache.druid.query.filter.DimFilter; import org.apache.druid.query.filter.ExtractionDimFilter; import org.apache.druid.query.filter.InDimFilter; import org.apache.druid.query.filter.JavaScriptDimFilter; +import org.apache.druid.query.filter.NotDimFilter; import org.apache.druid.query.filter.OrDimFilter; import org.apache.druid.query.filter.RegexDimFilter; import org.apache.druid.query.filter.SearchQueryDimFilter; @@ -10439,6 +10440,35 @@ public class GroupByQueryRunnerTest extends InitializedNullHandlingTest TestHelper.assertExpectedObjects(expectedResults, results, "type-conversion"); } + @Test + public void testGroupByNoMatchingPrefilter() + { + GroupByQuery query = makeQueryBuilder() + .setDataSource(QueryRunnerTestHelper.DATA_SOURCE) + .setQuerySegmentSpec(QueryRunnerTestHelper.FIRST_TO_THIRD) + .setDimensions( + new DefaultDimensionSpec("quality", "quality") + ) + .setDimFilter(new SelectorDimFilter("market", "spot", null, null)) + .setAggregatorSpecs( + QueryRunnerTestHelper.ROWS_COUNT, + new FilteredAggregatorFactory( + new LongSumAggregatorFactory("index", "index"), + new NotDimFilter(new SelectorDimFilter("longNumericNull", null, null)) + ) + ) + .setGranularity(QueryRunnerTestHelper.DAY_GRAN) + .setLimit(1) + .build(); + + List expectedResults = ImmutableList.of( + makeRow(query, "2011-04-01", "quality", "automotive", "rows", 1L, "index", 135L) + ); + + Iterable results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, query); + TestHelper.assertExpectedObjects(expectedResults, results, "groupBy"); + } + private static ResultRow makeRow(final GroupByQuery query, final String timestamp, final Object... vals) { return GroupByQueryRunnerTestHelper.createExpectedRow(query, timestamp, vals);