Merge pull request #1802 from jaffee/group-by-fixes

Group by fixes

docs/query-language.md

@@ -781,3 +781,111 @@ Options(Row(f1=10), shards=[0, 2])
 ```response
 {"attrs":{},"columns":[100, 2097152]}
 ```
+
+**Spec:**
+
+```
+Rows(field=<STRING>, previous=<UINT|STRING>, limit=<UINT>, column=<UINT|STRING>)
+```
+
+**Description:**
+
+Rows returns a list of row IDs in the given field which have at least one bit
+set. The field argument is mandatory, the others are  optional.
+
+If `previous` is given, rows prior to and including the specified row ID or
+key will not be returned. If `column` is given, only rows which have a set bit
+in the given column will be returned. `previous` or `column` must be strings if
+and only if the field or index respectively is using key translation. If `limit`
+is given, the number of rowIDs returned will be less than or equal to
+`limit`. The combination of `limit` and `previous` allows for paging over large
+result sets. Results are always ordered, so setting `previous` as the last
+result of the previous request will start from the next available row.
+
+
+**Result Type:** Object with `"rows" or "keys" and an array of integers or strings respectively.`
+
+**Examples:**
+
+Without keys:
+```request
+Rows(field=blah)
+```
+```response
+{"rows":[1,9,39]}
+```
+
+With keys:
+```request
+Rows(field=blahk)
+```
+```response
+{"rows":null,"keys":["haha","zaaa","traa"]}
+```
+
+
+**Spec:**
+
+```
+GroupBy(<RowsCall>, [RowsCall...], limit=<UINT>, filter=<CALL>)
+```
+
+**Description:**
+
+GroupBy returns the count of the intersection of every combination of rows
+taking one row each from the specified `Rows` calls. It returns only those
+combinations for which the count is greater than 0. 
+
+The optional `filter` argument takes any type of `Row` query (e.g. Row, Union,
+ Intersect, etc.) which will be intersected with each result prior to returning
+ the count. This is analagous to a WHERE clause applied to a relational GROUP BY
+ query.
+
+The optional `limit` argument limits the number of results returned. The results
+are ordered, so as long as the data isn't changing, the same query will return
+the same result set.
+
+Paging through results is supported by passing the `previous` argument to each
+of the `Rows` calls in the GroupBy. Take the last result from your previous
+`GroupBy` query, and pass each row ID in that result as the `previous` argument
+to each of the respective `Rows` queries in your next `GroupBy` query.
+
+**Result Type:** Array of "groups". Each group is an object with a group key and
+a count key. The count is an integer, and the group is an array of objects which
+specify the field and row for each row that was intersected to get that result.
+
+**Examples:**
+
+A single `Rows` query.
+```request
+GroupBy(Rows(field=blah))
+```
+```response
+[{"group":[{"field":"blah","rowID":1}],"count":1},
+{"group":[{"field":"blah","rowID":9}],"count":1},
+{"group":[{"field":"blah","rowID":39}],"count":1}]
+```
+
+With two `Rows` queries - one with IDs and one with keys.
+```request
+GroupBy(Rows(field=blah), Rows(field=blahk), limit=7)
+```
+```response
+[{"group":[{"field":"blah","rowID":1},{"field":"blahk","rowKey":"haha"}],"count":1},
+ {"group":[{"field":"blah","rowID":1},{"field":"blahk","rowKey":"zaaa"}],"count":1},
+ {"group":[{"field":"blah","rowID":1},{"field":"blahk","rowKey":"traa"}],"count":1},
+ {"group":[{"field":"blah","rowID":9},{"field":"blahk","rowKey":"haha"}],"count":1},
+ {"group":[{"field":"blah","rowID":9},{"field":"blahk","rowKey":"zaaa"}],"count":1},
+ {"group":[{"field":"blah","rowID":9},{"field":"blahk","rowKey":"traa"}],"count":1},
+ {"group":[{"field":"blah","rowID":39},{"field":"blahk","rowKey":"haha"}],"count":1}]
+```
+
+Getting the rest of the results from the previous example (paging).
+```request
+GroupBy(Rows(field=blah, previous=39), Rows(field=blahk, previous="haha"), limit=7)
+```
+
+```response
+[{"group":[{"field":"blah","rowID":39},{"field":"blahk","rowKey":"zaaa"}],"count":1},
+ {"group":[{"field":"blah","rowID":39},{"field":"blahk","rowKey":"traa"}],"count":1}]
+```

executor.go

@@ -2761,7 +2761,13 @@ func newGroupByIterator(rowIDs []RowIDs, children []*pql.Call, filter *Row, inde
 
 	ignorePrev := false
 	for i, call := range children {
-		fieldName := call.Args["field"].(string) // this has already been validated by this point
+		fieldName, ok := call.Args["field"].(string)
+		if !ok {
+			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.
 		frag := holder.fragment(index, fieldName, viewStandard, shard)

executor_test.go

@@ -2674,6 +2674,66 @@ func TestExecutor_Execute_Rows(t *testing.T) {
 	}
 }
 
+func TestExecutor_Execute_Query_Error(t *testing.T) {
+	c := test.MustRunCluster(t, 1)
+	defer c.Close()
+	c.CreateField(t, "i", pilosa.IndexOptions{}, "general")
+
+	tests := []struct {
+		query string
+		error string
+	}{
+		{
+			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",
+		},
+		{
+			query: "GroupBy(Rows(field=general), filter=Rows(field=general))",
+			error: "unknown call: Rows",
+		},
+	}
+
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			r, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{
+				Index: "i",
+				Query: test.query,
+			})
+			if err == nil {
+				t.Fatalf("should have gotten an error on invalid rows query, but got %#v", r)
+			}
+			if !strings.Contains(err.Error(), test.error) {
+				t.Fatalf("unexpected error message: %s", err.Error())
+			}
+		})
+	}
+
+}
+
 func TestExecutor_Execute_Rows_Keys(t *testing.T) {
 	c := test.MustRunCluster(t, 1)
 	defer c.Close()

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