From 81d08be0440b24fbfd30e794615a70d1b33c1d62 Mon Sep 17 00:00:00 2001
From: Matt Jaffee <matthew.jaffee@gmail.com>
Date: Fri, 21 Dec 2018 14:16:55 -0600
Subject: [PATCH] fix PQL, Rows and Group By problems

make sure that args which are Uints are positive and return an error if not.

improve group by error messages if field for Rows query is invalid
---
 executor.go      |  5 ++++-
 executor_test.go | 27 +++++++++++++++++++++++++--
 pql/ast.go       |  3 +++
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/executor.go b/executor.go
index 57aadf3a5..08eae1d49 100644
--- a/executor.go
+++ b/executor.go
@@ -2763,7 +2763,10 @@ func newGroupByIterator(rowIDs []RowIDs, children []*pql.Call, filter *Row, inde
 	for i, call := range children {
 		fieldName, ok := call.Args["field"].(string)
 		if !ok {
-			return nil, errors.Errorf("%s call must have 'field' argument", call.Name)
+			return nil, errors.Errorf("%s call must have 'field' argument with valid (string) field name. Got %v of type %[2]T", call.Name, call.Args["field"])
+		}
+		if holder.Field(index, fieldName) == nil {
+			return nil, ErrFieldNotFound
 		}
 		gbi.fields[i].Field = fieldName
 		// Fetch fragment.
diff --git a/executor_test.go b/executor_test.go
index 3924657da..73872abff 100644
--- a/executor_test.go
+++ b/executor_test.go
@@ -2672,10 +2672,9 @@ func TestExecutor_Execute_Rows(t *testing.T) {
 	if !reflect.DeepEqual(rows, pilosa.RowIdentifiers{Rows: []uint64{11, 12}}) {
 		t.Fatalf("unexpected rows: %+v", rows)
 	}
-
 }
 
-func TestExecutor_Execute_Rows_Error(t *testing.T) {
+func TestExecutor_Execute_Query_Error(t *testing.T) {
 	c := test.MustRunCluster(t, 3)
 	defer c.Close()
 	c.CreateField(t, "i", pilosa.IndexOptions{}, "general")
@@ -2688,6 +2687,30 @@ func TestExecutor_Execute_Rows_Error(t *testing.T) {
 			query: "GroupBy(Rows())",
 			error: "Rows call must have 'field' argument",
 		},
+		{
+			query: "GroupBy(Rows(field=true))",
+			error: "Rows call must have 'field' argument",
+		},
+		{
+			query: "GroupBy(Rows(field=\"true\"))",
+			error: "field not found",
+		},
+		{
+			query: "GroupBy(Rows(field=1))",
+			error: "Rows call must have 'field' argument",
+		},
+		{
+			query: "GroupBy(Rows(field))",
+			error: "parse error",
+		},
+		{
+			query: "GroupBy(Rows(field=general, limit=-1))",
+			error: "must be positive, but got",
+		},
+		{
+			query: "GroupBy(Rows(field=general), limit=-1)",
+			error: "must be positive, but got",
+		},
 	}
 
 	for i, test := range tests {
diff --git a/pql/ast.go b/pql/ast.go
index 32255b6a8..03b360099 100644
--- a/pql/ast.go
+++ b/pql/ast.go
@@ -291,6 +291,9 @@ func (c *Call) UintArg(key string) (uint64, bool, error) {
 	}
 	switch tval := val.(type) {
 	case int64:
+		if tval < 0 {
+			return 0, true, fmt.Errorf("value for '%s' must be positive, but got %v", key, tval)
+		}
 		return uint64(tval), true, nil
 	case uint64:
 		return tval, true, nil