From 3623f7468fec16c6fd24ae916951c4b4e227c2d1 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Wed, 22 Mar 2017 09:03:35 -0700 Subject: [PATCH] Fix some query cache key collisions. The query caches generally store dimensions and aggregators positionally, so appendCacheablesIgnoringOrder could lead to incorrect results being pulled from the cache. --- .../groupby/GroupByQueryQueryToolChest.java | 4 +- .../TimeseriesQueryQueryToolChest.java | 2 +- .../query/topn/TopNQueryQueryToolChest.java | 2 +- .../TimeseriesQueryQueryToolChestTest.java | 53 +++++++++++++++++-- 4 files changed, 52 insertions(+), 9 deletions(-) diff --git a/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java index 9701370ce60e..9394c7d00a92 100644 --- a/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -388,8 +388,8 @@ public byte[] computeCacheKey(GroupByQuery query) .appendByte(CACHE_STRATEGY_VERSION) .appendCacheable(query.getGranularity()) .appendCacheable(query.getDimFilter()) - .appendCacheablesIgnoringOrder(query.getAggregatorSpecs()) - .appendCacheablesIgnoringOrder(query.getDimensions()) + .appendCacheables(query.getAggregatorSpecs()) + .appendCacheables(query.getDimensions()) .appendCacheable(query.getVirtualColumns()) .build(); } diff --git a/processing/src/main/java/io/druid/query/timeseries/TimeseriesQueryQueryToolChest.java b/processing/src/main/java/io/druid/query/timeseries/TimeseriesQueryQueryToolChest.java index f4b60b1071b0..07bb0964a70d 100644 --- a/processing/src/main/java/io/druid/query/timeseries/TimeseriesQueryQueryToolChest.java +++ b/processing/src/main/java/io/druid/query/timeseries/TimeseriesQueryQueryToolChest.java @@ -141,7 +141,7 @@ public byte[] computeCacheKey(TimeseriesQuery query) .appendBoolean(query.isSkipEmptyBuckets()) .appendCacheable(query.getGranularity()) .appendCacheable(query.getDimensionsFilter()) - .appendCacheablesIgnoringOrder(query.getAggregatorSpecs()) + .appendCacheables(query.getAggregatorSpecs()) .appendCacheable(query.getVirtualColumns()) .build(); } diff --git a/processing/src/main/java/io/druid/query/topn/TopNQueryQueryToolChest.java b/processing/src/main/java/io/druid/query/topn/TopNQueryQueryToolChest.java index 8b3d3e12e23d..8d3d06ab3485 100644 --- a/processing/src/main/java/io/druid/query/topn/TopNQueryQueryToolChest.java +++ b/processing/src/main/java/io/druid/query/topn/TopNQueryQueryToolChest.java @@ -317,7 +317,7 @@ public byte[] computeCacheKey(TopNQuery query) .appendInt(query.getThreshold()) .appendCacheable(query.getGranularity()) .appendCacheable(query.getDimensionsFilter()) - .appendCacheablesIgnoringOrder(query.getAggregatorSpecs()) + .appendCacheables(query.getAggregatorSpecs()) .appendCacheable(query.getVirtualColumns()); final List postAggregators = prunePostAggregators(query); diff --git a/processing/src/test/java/io/druid/query/timeseries/TimeseriesQueryQueryToolChestTest.java b/processing/src/test/java/io/druid/query/timeseries/TimeseriesQueryQueryToolChestTest.java index 5331a0b030ef..3ba99b208c21 100644 --- a/processing/src/test/java/io/druid/query/timeseries/TimeseriesQueryQueryToolChestTest.java +++ b/processing/src/test/java/io/druid/query/timeseries/TimeseriesQueryQueryToolChestTest.java @@ -25,11 +25,12 @@ import io.druid.jackson.DefaultObjectMapper; import io.druid.java.util.common.granularity.Granularities; import io.druid.query.CacheStrategy; +import io.druid.query.Druids; import io.druid.query.QueryRunnerTestHelper; import io.druid.query.Result; import io.druid.query.TableDataSource; -import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.CountAggregatorFactory; +import io.druid.query.aggregation.LongSumAggregatorFactory; import io.druid.query.spec.MultipleIntervalSegmentSpec; import io.druid.segment.VirtualColumns; import org.joda.time.DateTime; @@ -45,6 +46,8 @@ @RunWith(Parameterized.class) public class TimeseriesQueryQueryToolChestTest { + private static final TimeseriesQueryQueryToolChest TOOL_CHEST = new TimeseriesQueryQueryToolChest(null); + @Parameterized.Parameters(name = "descending={0}") public static Iterable constructorFeeder() throws IOException { @@ -61,9 +64,8 @@ public TimeseriesQueryQueryToolChestTest(boolean descending) @Test public void testCacheStrategy() throws Exception { - CacheStrategy, Object, TimeseriesQuery> strategy = - new TimeseriesQueryQueryToolChest(null).getCacheStrategy( + TOOL_CHEST.getCacheStrategy( new TimeseriesQuery( new TableDataSource("dummy"), new MultipleIntervalSegmentSpec( @@ -77,7 +79,10 @@ public void testCacheStrategy() throws Exception VirtualColumns.EMPTY, null, Granularities.ALL, - ImmutableList.of(new CountAggregatorFactory("metric1")), + ImmutableList.of( + new CountAggregatorFactory("metric1"), + new LongSumAggregatorFactory("metric0", "metric0") + ), null, null ) @@ -87,7 +92,7 @@ public void testCacheStrategy() throws Exception // test timestamps that result in integer size millis new DateTime(123L), new TimeseriesResultValue( - ImmutableMap.of("metric1", 2) + ImmutableMap.of("metric1", 2, "metric0", 3) ) ); @@ -103,4 +108,42 @@ public void testCacheStrategy() throws Exception Assert.assertEquals(result, fromCacheResult); } + + @Test + public void testCacheKey() throws Exception + { + final TimeseriesQuery query1 = Druids.newTimeseriesQueryBuilder() + .dataSource("dummy") + .intervals("2015-01-01/2015-01-02") + .descending(descending) + .granularity(Granularities.ALL) + .aggregators( + ImmutableList.of( + new CountAggregatorFactory("metric1"), + new LongSumAggregatorFactory("metric0", "metric0") + ) + ) + .build(); + + final TimeseriesQuery query2 = Druids.newTimeseriesQueryBuilder() + .dataSource("dummy") + .intervals("2015-01-01/2015-01-02") + .descending(descending) + .granularity(Granularities.ALL) + .aggregators( + ImmutableList.of( + new LongSumAggregatorFactory("metric0", "metric0"), + new CountAggregatorFactory("metric1") + ) + ) + .build(); + + // Test for https://github.com/druid-io/druid/issues/4093. + Assert.assertFalse( + Arrays.equals( + TOOL_CHEST.getCacheStrategy(query1).computeCacheKey(query1), + TOOL_CHEST.getCacheStrategy(query2).computeCacheKey(query2) + ) + ); + } }