From ebab0081a7235286dd55bfbfc58ee07632cee846 Mon Sep 17 00:00:00 2001
From: Gian Merlino <gian@imply.io>
Date: Wed, 8 Mar 2017 08:32:35 -0800
Subject: [PATCH] groupBy v2: Always merge queries.

This fixes #4020 because it means the timestamp will always be included for outermost
queries. Historicals receiving queries from older brokers will think they're
outermost (because CTX_KEY_OUTERMOST isn't set to "false"), so they'll include a
timestamp, so the older brokers will be OK.
---
 .../query/groupby/GroupByQueryQueryToolChest.java      |  7 ++++---
 .../druid/query/groupby/strategy/GroupByStrategy.java  |  5 +++++
 .../query/groupby/strategy/GroupByStrategyV1.java      | 10 ++++++++--
 .../query/groupby/strategy/GroupByStrategyV2.java      |  6 ++++++
 4 files changed, 23 insertions(+), 5 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 8b0a9f8e473c..9701370ce60e 100644
--- a/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java
+++ b/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java
@@ -108,9 +108,10 @@ public Sequence<Row> run(Query<Row> query, Map<String, Object> responseContext)
           return runner.run(query, responseContext);
         }
 
-        if (query.getContextBoolean(GROUP_BY_MERGE_KEY, true)) {
+        final GroupByQuery groupByQuery = (GroupByQuery) query;
+        if (strategySelector.strategize(groupByQuery).doMergeResults(groupByQuery)) {
           return initAndMergeGroupByResults(
-              (GroupByQuery) query,
+              groupByQuery,
               runner,
               responseContext
           );
@@ -181,7 +182,7 @@ private Sequence<Row> mergeGroupByResults(
           subquery.withOverriddenContext(
               ImmutableMap.<String, Object>of(
                   //setting sort to false avoids unnecessary sorting while merging results. we only need to sort
-                  //in the end when returning results to user.
+                  //in the end when returning results to user. (note this is only respected by groupBy v1)
                   GroupByQueryHelper.CTX_KEY_SORT_RESULTS,
                   false
               )
diff --git a/processing/src/main/java/io/druid/query/groupby/strategy/GroupByStrategy.java b/processing/src/main/java/io/druid/query/groupby/strategy/GroupByStrategy.java
index 5d140fae8267..c170db1520dc 100644
--- a/processing/src/main/java/io/druid/query/groupby/strategy/GroupByStrategy.java
+++ b/processing/src/main/java/io/druid/query/groupby/strategy/GroupByStrategy.java
@@ -53,6 +53,11 @@ public interface GroupByStrategy
    */
   boolean isCacheable(boolean willMergeRunners);
 
+  /**
+   * Indicates if this query should undergo "mergeResults" or not.
+   */
+  boolean doMergeResults(final GroupByQuery query);
+
   /**
    * Decorate a runner with an interval chunking decorator.
    */
diff --git a/processing/src/main/java/io/druid/query/groupby/strategy/GroupByStrategyV1.java b/processing/src/main/java/io/druid/query/groupby/strategy/GroupByStrategyV1.java
index 544e6c9ce1cf..9b2c9163818b 100644
--- a/processing/src/main/java/io/druid/query/groupby/strategy/GroupByStrategyV1.java
+++ b/processing/src/main/java/io/druid/query/groupby/strategy/GroupByStrategyV1.java
@@ -101,6 +101,12 @@ public QueryRunner<Row> createIntervalChunkingRunner(
     return decorator.decorate(runner, toolChest);
   }
 
+  @Override
+  public boolean doMergeResults(final GroupByQuery query)
+  {
+    return query.getContextBoolean(GroupByQueryQueryToolChest.GROUP_BY_MERGE_KEY, true);
+  }
+
   @Override
   public Sequence<Row> mergeResults(
       final QueryRunner<Row> baseRunner,
@@ -131,10 +137,10 @@ public Sequence<Row> mergeResults(
                 ImmutableMap.<String, Object>of(
                     "finalize", false,
                     //setting sort to false avoids unnecessary sorting while merging results. we only need to sort
-                    //in the end when returning results to user.
+                    //in the end when returning results to user. (note this is only respected by groupBy v1)
                     GroupByQueryHelper.CTX_KEY_SORT_RESULTS, false,
                     //no merging needed at historicals because GroupByQueryRunnerFactory.mergeRunners(..) would return
-                    //merged results
+                    //merged results. (note this is only respected by groupBy v1)
                     GroupByQueryQueryToolChest.GROUP_BY_MERGE_KEY, false,
                     GroupByQueryConfig.CTX_KEY_STRATEGY, GroupByStrategySelector.STRATEGY_V1
                 )
diff --git a/processing/src/main/java/io/druid/query/groupby/strategy/GroupByStrategyV2.java b/processing/src/main/java/io/druid/query/groupby/strategy/GroupByStrategyV2.java
index e4dc798d17c8..45dbd954d1c7 100644
--- a/processing/src/main/java/io/druid/query/groupby/strategy/GroupByStrategyV2.java
+++ b/processing/src/main/java/io/druid/query/groupby/strategy/GroupByStrategyV2.java
@@ -179,6 +179,12 @@ public boolean isCacheable(boolean willMergeRunners)
     return willMergeRunners;
   }
 
+  @Override
+  public boolean doMergeResults(final GroupByQuery query)
+  {
+    return true;
+  }
+
   @Override
   public QueryRunner<Row> createIntervalChunkingRunner(
       final IntervalChunkingQueryRunnerDecorator decorator,