From 97eb94397f09064093cd458cec0cadbcbffaa3fe Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Fri, 21 Dec 2018 11:55:25 -0600 Subject: [PATCH 1/4] fix Rows bug where Pilosa would crash without 'field' argument. --- executor.go | 5 ++++- executor_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/executor.go b/executor.go index bbb8c2297..57aadf3a5 100644 --- a/executor.go +++ b/executor.go @@ -2761,7 +2761,10 @@ 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", call.Name) + } gbi.fields[i].Field = fieldName // Fetch fragment. frag := holder.fragment(index, fieldName, viewStandard, shard) diff --git a/executor_test.go b/executor_test.go index 141533d7f..3924657da 100644 --- a/executor_test.go +++ b/executor_test.go @@ -2672,6 +2672,39 @@ 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) { + c := test.MustRunCluster(t, 3) + 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", + }, + } + + 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) { From 19696e3085ccf6311b7538c20d6d69a5914d842f Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Fri, 21 Dec 2018 11:56:05 -0600 Subject: [PATCH 2/4] add GroupBy and Rows docs --- docs/query-language.md | 108 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/docs/query-language.md b/docs/query-language.md index babcc390c..4b93b6311 100644 --- a/docs/query-language.md +++ b/docs/query-language.md @@ -781,3 +781,111 @@ Options(Row(f1=10), shards=[0, 2]) ```response {"attrs":{},"columns":[100, 2097152]} ``` + +**Spec:** + +``` +Rows(field=, previous=, limit=, column=) +``` + +**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...], limit=, filter=) +``` + +**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}] +``` From 81d08be0440b24fbfd30e794615a70d1b33c1d62 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Fri, 21 Dec 2018 14:16:55 -0600 Subject: [PATCH 3/4] 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 From ed637e69219342937617ba31d10c9e5b2cb3d3c4 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Fri, 21 Dec 2018 17:24:44 -0600 Subject: [PATCH 4/4] add test for group by with invalid filter --- executor_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/executor_test.go b/executor_test.go index 73872abff..8ac7008ab 100644 --- a/executor_test.go +++ b/executor_test.go @@ -2675,7 +2675,7 @@ func TestExecutor_Execute_Rows(t *testing.T) { } func TestExecutor_Execute_Query_Error(t *testing.T) { - c := test.MustRunCluster(t, 3) + c := test.MustRunCluster(t, 1) defer c.Close() c.CreateField(t, "i", pilosa.IndexOptions{}, "general") @@ -2711,6 +2711,10 @@ func TestExecutor_Execute_Query_Error(t *testing.T) { 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 {