From 82da4794641cc7eedcb16a5d14504ad94364f58b Mon Sep 17 00:00:00 2001
From: Maarten Rijke <maartenrijke@gmail.com>
Date: Wed, 13 May 2015 11:02:49 +0200
Subject: [PATCH] Fix for GroupBy with Having+Limit+Orderspec

  * Inverted function arguments to compose postProcFn for GroupBy queries
    with havingspec + limitspec.
  * Replaced query.getLimitSpec() with null in GroupByQueryToolChest's
    mergeGroupByResults
  * Added unittest to verify functionality
---
 .../io/druid/query/groupby/GroupByQuery.java  |  4 +-
 .../groupby/GroupByQueryQueryToolChest.java   |  2 +-
 .../query/groupby/GroupByQueryRunnerTest.java | 85 +++++++++++++++++++
 3 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/processing/src/main/java/io/druid/query/groupby/GroupByQuery.java b/processing/src/main/java/io/druid/query/groupby/GroupByQuery.java
index 44825749d0c..7d496f10bc3 100644
--- a/processing/src/main/java/io/druid/query/groupby/GroupByQuery.java
+++ b/processing/src/main/java/io/druid/query/groupby/GroupByQuery.java
@@ -104,6 +104,7 @@ public class GroupByQuery extends BaseQuery<Row>
 
     if (havingSpec != null) {
       postProcFn = Functions.compose(
+          postProcFn,
           new Function<Sequence<Row>, Sequence<Row>>()
           {
             @Override
@@ -121,8 +122,7 @@ public class GroupByQuery extends BaseQuery<Row>
                   }
               );
             }
-          },
-          postProcFn
+          }
       );
     }
 
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 fe2132afa3f..40fb157e208 100644
--- a/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java
+++ b/processing/src/main/java/io/druid/query/groupby/GroupByQueryQueryToolChest.java
@@ -189,7 +189,7 @@ public class GroupByQueryQueryToolChest extends QueryToolChest<Row, GroupByQuery
                   ImmutableList.<PostAggregator>of(),
                   // Don't do "having" clause until the end of this method.
                   null,
-                  query.getLimitSpec(),
+                  null,
                   query.getContext()
               ).withOverriddenContext(
                   ImmutableMap.<String, Object>of(
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 70dbaa273d0..ea33b40340d 100644
--- a/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java
+++ b/processing/src/test/java/io/druid/query/groupby/GroupByQueryRunnerTest.java
@@ -1406,6 +1406,91 @@ public class GroupByQueryRunnerTest
     TestHelper.assertExpectedObjects(expectedResults, mergedRunner.run(fullQuery, context), "merged");
   }
 
+  @Test
+  public void testGroupByWithOrderLimitHavingSpec()
+  {
+    GroupByQuery.Builder builder = GroupByQuery
+        .builder()
+        .setDataSource(QueryRunnerTestHelper.dataSource)
+        .setInterval("2011-01-25/2011-01-28")
+        .setDimensions(Lists.<DimensionSpec>newArrayList(new DefaultDimensionSpec("quality", "alias")))
+        .setAggregatorSpecs(
+            Arrays.asList(
+                QueryRunnerTestHelper.rowsCount,
+                new DoubleSumAggregatorFactory("index", "index")
+            )
+        )
+        .setGranularity(QueryGranularity.ALL)
+        .setHavingSpec(new GreaterThanHavingSpec("index", 310L))
+        .setLimitSpec(
+            new DefaultLimitSpec(
+                Lists.newArrayList(
+                    new OrderByColumnSpec(
+                        "index",
+                        OrderByColumnSpec.Direction.ASCENDING
+                    )
+                ),
+                5
+            )
+        );
+
+    List<Row> expectedResults = Arrays.asList(
+        GroupByQueryRunnerTestHelper.createExpectedRow(
+            "2011-01-25",
+            "alias",
+            "business",
+            "rows",
+            3L,
+            "index",
+            312.38165283203125
+        ),
+        GroupByQueryRunnerTestHelper.createExpectedRow(
+            "2011-01-25",
+            "alias",
+            "news",
+            "rows",
+            3L,
+            "index",
+            312.7834167480469
+        ),
+        GroupByQueryRunnerTestHelper.createExpectedRow(
+            "2011-01-25",
+            "alias",
+            "technology",
+            "rows",
+            3L,
+            "index",
+            324.6412353515625
+        ),
+        GroupByQueryRunnerTestHelper.createExpectedRow(
+            "2011-01-25",
+            "alias",
+            "travel",
+            "rows",
+            3L,
+            "index",
+            393.36322021484375
+        ),
+        GroupByQueryRunnerTestHelper.createExpectedRow(
+            "2011-01-25",
+            "alias",
+            "health",
+            "rows",
+            3L,
+            "index",
+            511.2996826171875
+        )
+    );
+
+    GroupByQuery fullQuery = builder.build();
+    Iterable<Row> results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, fullQuery);
+    TestHelper.assertExpectedObjects(
+        expectedResults,
+        results,
+        ""
+    );
+  }
+
   @Test
   public void testPostAggHavingSpec()
   {