From 1f481986075e490ae1ddc195369e465b7c17790e Mon Sep 17 00:00:00 2001
From: Gian Merlino <gianmerlino@gmail.com>
Date: Wed, 22 Mar 2017 11:08:48 -0700
Subject: [PATCH] Fix some query cache key collisions. (#4094)

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 9701370ce60..9394c7d00a9 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 class GroupByQueryQueryToolChest extends QueryToolChest<Row, GroupByQuery
             .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 f4b60b1071b..07bb0964a70 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 class TimeseriesQueryQueryToolChest extends QueryToolChest<Result<Timeser
             .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 8b3d3e12e23..8d3d06ab348 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 class TopNQueryQueryToolChest extends QueryToolChest<Result<TopNResultVal
             .appendInt(query.getThreshold())
             .appendCacheable(query.getGranularity())
             .appendCacheable(query.getDimensionsFilter())
-            .appendCacheablesIgnoringOrder(query.getAggregatorSpecs())
+            .appendCacheables(query.getAggregatorSpecs())
             .appendCacheable(query.getVirtualColumns());
 
         final List<PostAggregator> 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 5331a0b030e..3ba99b208c2 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 com.google.common.collect.ImmutableMap;
 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 @@ import java.util.Arrays;
 @RunWith(Parameterized.class)
 public class TimeseriesQueryQueryToolChestTest
 {
+  private static final TimeseriesQueryQueryToolChest TOOL_CHEST = new TimeseriesQueryQueryToolChest(null);
+
   @Parameterized.Parameters(name = "descending={0}")
   public static Iterable<Object[]> constructorFeeder() throws IOException
   {
@@ -61,9 +64,8 @@ public class TimeseriesQueryQueryToolChestTest
   @Test
   public void testCacheStrategy() throws Exception
   {
-
     CacheStrategy<Result<TimeseriesResultValue>, Object, TimeseriesQuery> strategy =
-        new TimeseriesQueryQueryToolChest(null).getCacheStrategy(
+        TOOL_CHEST.getCacheStrategy(
             new TimeseriesQuery(
                 new TableDataSource("dummy"),
                 new MultipleIntervalSegmentSpec(
@@ -77,7 +79,10 @@ public class TimeseriesQueryQueryToolChestTest
                 VirtualColumns.EMPTY,
                 null,
                 Granularities.ALL,
-                ImmutableList.<AggregatorFactory>of(new CountAggregatorFactory("metric1")),
+                ImmutableList.of(
+                    new CountAggregatorFactory("metric1"),
+                    new LongSumAggregatorFactory("metric0", "metric0")
+                ),
                 null,
                 null
             )
@@ -87,7 +92,7 @@ public class TimeseriesQueryQueryToolChestTest
         // test timestamps that result in integer size millis
         new DateTime(123L),
         new TimeseriesResultValue(
-            ImmutableMap.<String, Object>of("metric1", 2)
+            ImmutableMap.of("metric1", 2, "metric0", 3)
         )
     );
 
@@ -103,4 +108,42 @@ public class TimeseriesQueryQueryToolChestTest
 
     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)
+        )
+    );
+  }
 }