From 6094663e7ab190eaf49e3658c3abb1af0bbd5f43 Mon Sep 17 00:00:00 2001
From: Matt Jaffee <matthew.jaffee@gmail.com>
Date: Fri, 1 Jan 2021 21:56:59 -0600
Subject: [PATCH] fix bug with "having" and "limit" in GroupBy

the limit could get applied before "having" in some cases which could
result in results being discarded which met the having condition while
results were kept which did not, ultimately resulting in GroupBy
falsely reporting fewer results than actually existed.
---
 executor.go      | 15 ++++++++++-----
 executor_test.go |  6 ++++++
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/executor.go b/executor.go
index 7482dff74..1863b7b02 100644
--- a/executor.go
+++ b/executor.go
@@ -2767,7 +2767,8 @@ func (e *executor) executeGroupBy(ctx context.Context, qcx *Qcx, index string, c
 		// don't want to prematurely limit the results if we're sorting
 		limit = int(^uint(0) >> 1)
 	}
-	if _, hasHaving, err := c.CallArg("having"); err != nil {
+	having, hasHaving, err := c.CallArg("having")
+	if err != nil {
 		return nil, errors.Wrap(err, "getting 'having' argument")
 	} else if hasHaving {
 		// don't want to prematurely limit the results if we're filtering some out
@@ -2849,7 +2850,7 @@ func (e *executor) executeGroupBy(ctx context.Context, qcx *Qcx, index string, c
 	// If there's no sorting, we want to apply limits before
 	// calculating the Distinct aggregate which is expensive on a
 	// per-result basis.
-	if sorter == nil {
+	if sorter == nil && !hasHaving {
 		results, err = applyLimitAndOffsetToGroupByResult(c, results)
 		if err != nil {
 			return nil, errors.Wrap(err, "applying limit/offset")
@@ -2904,9 +2905,7 @@ func (e *executor) executeGroupBy(ctx context.Context, qcx *Qcx, index string, c
 	}
 
 	// Apply having.
-	if having, hasHaving, err := c.CallArg("having"); err != nil {
-		return nil, err
-	} else if hasHaving && !opt.Remote {
+	if hasHaving && !opt.Remote {
 		// parse the condition as PQL
 		if having.Name != "Condition" {
 			return nil, errors.New("the only supported having call is Condition()")
@@ -2931,6 +2930,12 @@ func (e *executor) executeGroupBy(ctx context.Context, qcx *Qcx, index string, c
 		if err != nil {
 			return nil, errors.Wrap(err, "applying limit/offset")
 		}
+	} else if hasHaving && !opt.Remote {
+		results, err = applyLimitAndOffsetToGroupByResult(c, results)
+		if err != nil {
+			return nil, errors.Wrap(err, "applying limit/offset")
+		}
+
 	}
 
 	return results, nil
diff --git a/executor_test.go b/executor_test.go
index 920977cb8..cfb2d555d 100644
--- a/executor_test.go
+++ b/executor_test.go
@@ -7018,6 +7018,12 @@ zebra,1,0
 toucan,1,0
 dog,1,0
 icecream,6,0
+`,
+		},
+		{
+			query: "GroupBy(Rows(field=likes), aggregate=Sum(field=net_worth), limit=2, having=Condition(sum>10))",
+			csvVerifier: `pangolin,1,100
+zebra,1,1000
 `,
 		},
 		{