From 58ff92d3d64c30dfdd70c2beb171d501315fd43a Mon Sep 17 00:00:00 2001
From: Travis <travis@pilosa.com>
Date: Thu, 28 Jan 2021 21:12:02 -0600
Subject: [PATCH] Fix Groupby test which uses RowKey instead of RowID

This commit introduces a CheckGroupByOnKey function which acts like the
CheckGroupBy function, but it only ensures equality on RowKey, not
RowID.
---
 executor_test.go | 16 +++++++++-------
 test/pilosa.go   | 27 +++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/executor_test.go b/executor_test.go
index 9891843ec..5ca6c6f5e 100644
--- a/executor_test.go
+++ b/executor_test.go
@@ -5911,16 +5911,18 @@ func TestExecutor_Execute_GroupBy(t *testing.T) {
 		`)
 
 		t.Run("test foreign index with keys", func(t *testing.T) {
-			// the execututor returns row IDs when the field has keys, so they should be included in the target.
-			// because the order is determined by the partitioned index key, they seem out of order.
+			// The execututor returns row IDs when the field has keys, but we
+			// don't include them because they are not necessary in the result
+			// comparison. Because of this, we use the CheckGroupByOnKey
+			// function here to check equality only on the key field.
 			expected := []pilosa.GroupCount{
-				{Group: []pilosa.FieldRow{{Field: "child", RowID: 0, RowKey: "one"}}, Count: 3},
-				{Group: []pilosa.FieldRow{{Field: "child", RowID: 1, RowKey: "five"}}, Count: 1},
-				{Group: []pilosa.FieldRow{{Field: "child", RowID: 2, RowKey: "three"}}, Count: 2},
+				{Group: []pilosa.FieldRow{{Field: "child", RowKey: "one"}}, Count: 3},
+				{Group: []pilosa.FieldRow{{Field: "child", RowKey: "three"}}, Count: 2},
+				{Group: []pilosa.FieldRow{{Field: "child", RowKey: "five"}}, Count: 1},
 			}
 
-			results := c.Query(t, "fic", `GroupBy(Rows(child))`).Results[0].(*pilosa.GroupCounts).Groups()
-			test.CheckGroupBy(t, expected, results)
+			results := c.Query(t, "fic", `GroupBy(Rows(child), sort="count desc")`).Results[0].(*pilosa.GroupCounts).Groups()
+			test.CheckGroupByOnKey(t, expected, results)
 		})
 
 	}
diff --git a/test/pilosa.go b/test/pilosa.go
index e3a0918a0..7a3250ef7 100644
--- a/test/pilosa.go
+++ b/test/pilosa.go
@@ -337,6 +337,33 @@ func CheckGroupBy(t *testing.T, expected, results []pilosa.GroupCount) {
 	}
 }
 
+// CheckGroupByOnKey is like CheckGroupBy, but it doen't enforce a match on the GroupBy.Group.RowID value.
+// In cases where the Group has a RowKey, then the value of RowID is not consistently assigned. Instead,
+// it depends on the order of key translation IDs based on shard allocation to the
+func CheckGroupByOnKey(t *testing.T, expected, results []pilosa.GroupCount) {
+	t.Helper()
+	if len(results) != len(expected) {
+		t.Fatalf("number of groupings mismatch:\n got:%+v\nwant:%+v\n", results, expected)
+	}
+	for i, result := range results {
+		exp := expected[i]
+		if len(exp.Group) != len(result.Group) {
+			t.Fatalf("number of groups within GroupCount mismatch:\n got:%+v\nwant:%+v\n", result, exp)
+		}
+		if exp.Count != result.Count {
+			t.Fatalf("GroupCount count mismatch:\n got:%+v\nwant:%+v\n", result, exp)
+		}
+		if exp.Agg != result.Agg {
+			t.Fatalf("GroupCount aggregate mismatch:\n got:%+v\nwant:%+v\n", result, exp)
+		}
+		for j, grp := range result.Group {
+			if grp.Field != exp.Group[j].Field || grp.RowKey != exp.Group[j].RowKey {
+				t.Fatalf("GroupCount group value mismatch:\n got:%+v\nwant:%+v\n", result, exp)
+			}
+		}
+	}
+}
+
 // httpResponse is a wrapper for http.Response that holds the Body as a string.
 type httpResponse struct {
 	*gohttp.Response