From 5fdae7481275916c8c40c033c844896251b4e876 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Tue, 29 Dec 2020 16:53:31 -0600 Subject: [PATCH 1/5] draft of sorting groupby results --- executor.go | 145 +++++++++++++++++++++++++++++++++++++++++++++++ executor_test.go | 28 ++++++++- pql/ast.go | 1 + 3 files changed, 172 insertions(+), 2 deletions(-) diff --git a/executor.go b/executor.go index f4e5bdd45..32a1b2f77 100644 --- a/executor.go +++ b/executor.go @@ -2613,6 +2613,135 @@ func (r RowIDs) merge(other RowIDs, limit int) RowIDs { return result } +type order bool + +const ( + asc order = true + desc order = false +) + +type groupCountSorter struct { + fields []int + order []order + data []GroupCount +} + +func (g *groupCountSorter) Len() int { return len(g.data) } +func (g *groupCountSorter) Swap(i, j int) { g.data[i], g.data[j] = g.data[j], g.data[i] } +func (g *groupCountSorter) Less(i, j int) bool { + gci, gcj := g.data[i], g.data[j] + for idx, fieldIndex := range g.fields { + fieldOrder := g.order[idx] + switch fieldIndex { + case -1: // Count + if gci.Count == gcj.Count { + continue + } + if fieldOrder == asc { + return gci.Count < gcj.Count + } + return gcj.Count < gci.Count + case -2: // aggregate/Sum + if gci.Sum == gcj.Sum { + continue + } + if fieldOrder == asc { + return gci.Sum < gcj.Sum + } + return gcj.Sum < gci.Sum + default: + switch compareFieldRows(gci.Group[fieldIndex], gcj.Group[fieldIndex]) { + case 0: + continue + case -1: + return fieldOrder == asc + case 1: + return fieldOrder == desc + } + + } + } + return true +} + +// compareFieldRows returns -1 if a < b, +1 if a > b, and 0 if they +// are equal. It checks Value, RowKey and RowID, but assumes that +// Field is equal. +func compareFieldRows(a, b FieldRow) int { + if a.Value != nil { + if a.Value == b.Value { + return 0 + } + if *a.Value < *b.Value { + return -1 + } + return +1 + } + if a.RowKey != "" { + if a.RowKey == b.RowKey { + return 0 + } + if a.RowKey < b.RowKey { + return -1 + } + return +1 + } + if a.RowID != 0 { + if a.RowID == b.RowID { + return 0 + } + if a.RowID < b.RowID { + return -1 + } + return +1 + } + // OK, everything in a is zero... so b must be greater unless it's + // also zero (because if a.Value is nil, b.Value must also be + // nil... or something fishy is happening) + if b.RowKey == "" && b.RowID == 0 { + return 0 + } + return -1 +} + +// getSorter hackily parses the sortSpec and figures out how to sort +// the GroupBy results. TODO Probably has about a billion edge case +// bugs. Also needs to take in the Call so it can figure out if fields +// are valid and what order they're in. +func getSorter(sortSpec string) (*groupCountSorter, error) { + gcs := &groupCountSorter{ + fields: []int{}, + order: []order{}, + } + sortOn := strings.Split(sortSpec, ",") + for _, sortField := range sortOn { + sortField = strings.TrimSpace(sortField) + fieldDir := strings.Split(sortField, " ") + defaultOrder := asc + if fieldDir[0] == "count" { + gcs.fields = append(gcs.fields, -1) + defaultOrder = desc + } else if fieldDir[0] == "aggregate" { + gcs.fields = append(gcs.fields, -2) + defaultOrder = desc + } else { + gcs.fields = append(gcs.fields, 0) // TODO actually figure out which field. probably need the call + } + + if len(fieldDir) == 0 { + gcs.order = append(gcs.order, defaultOrder) + } + if fieldDir[1] == "asc" { + gcs.order = append(gcs.order, asc) + } else if fieldDir[1] == "desc" { + gcs.order = append(gcs.order, desc) + } else { + return nil, errors.Errorf("unknown sort direction '%s'", fieldDir[1]) + } + } + return gcs, nil +} + func (e *executor) executeGroupBy(ctx context.Context, qcx *Qcx, index string, c *pql.Call, shards []uint64, opt *execOptions) ([]GroupCount, error) { span, ctx := tracing.StartSpanFromContext(ctx, "Executor.executeGroupBy") defer span.Finish() @@ -2631,6 +2760,16 @@ func (e *executor) executeGroupBy(ctx context.Context, qcx *Qcx, index string, c return nil, err } + var sorter *groupCountSorter + if sortSpec, found, err := c.StringArg("sort"); err != nil { + return nil, errors.Wrap(err, "getting sort arg") + } else if found { + sorter, err = getSorter(sortSpec) + if err != nil { + return nil, errors.Wrap(err, "parsing sort spec") + } + } + idx := e.Holder.Index(index) if idx == nil { return nil, newNotFoundError(ErrIndexNotFound, index) @@ -2781,6 +2920,12 @@ func (e *executor) executeGroupBy(ctx context.Context, qcx *Qcx, index string, c results[n].Sum = int64(aggregateCount[0].(uint64)) } } + + if sorter != nil { + sorter.data = results + sort.Sort(sorter) + } + return results, nil } diff --git a/executor_test.go b/executor_test.go index 5d18bdaac..f6d711971 100644 --- a/executor_test.go +++ b/executor_test.go @@ -6789,12 +6789,12 @@ func TestMissingKeyRegression(t *testing.T) { func TestVariousQueries(t *testing.T) { for _, clusterSize := range []int{1, 3, 4, 7} { t.Run(fmt.Sprintf("%d-node", clusterSize), func(t *testing.T) { - testVariousQueries(t, clusterSize) + variousQueries(t, clusterSize) }) } } -func testVariousQueries(t *testing.T, clusterSize int) { +func variousQueries(t *testing.T, clusterSize int) { c := test.MustRunCluster(t, clusterSize) defer c.Close() @@ -7046,6 +7046,30 @@ pangolin,1,1 zebra,1,1 toucan,1,1 icecream,5,3 +`, + }, + { + query: "GroupBy(Rows(field=likes), sort=\"count desc, likes asc\")", + // note, sort is in order of rowID rather than rowKey + csvVerifier: `icecream,6,0 +molecula,1,0 +pilosa,1,0 +pangolin,1,0 +zebra,1,0 +toucan,1,0 +dog,1,0 +`, + }, + { + query: "GroupBy(Rows(field=likes), sort=\"count desc, likes desc\")", + // note, sort is in order of rowID rather than rowKey + csvVerifier: `icecream,6,0 +dog,1,0 +toucan,1,0 +zebra,1,0 +pangolin,1,0 +pilosa,1,0 +molecula,1,0 `, }, } diff --git a/pql/ast.go b/pql/ast.go index 50b38e5c5..056a8fc85 100644 --- a/pql/ast.go +++ b/pql/ast.go @@ -444,6 +444,7 @@ var callInfoByFunc = map[string]callInfo{ "previous": nil, "aggregate": nil, "having": nil, + "sort": "", }, }, "Options": { From ea539d8241f71988fd3121a5aaf227144dd39a36 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Thu, 31 Dec 2020 14:35:47 -0600 Subject: [PATCH 2/5] simplify groupby sorting and fix bugs Back out support for sorting on fields (only count and aggregate supported for now). Fix bug where default return of "true" caused sort to be unstable. (If they are equal, Less should return false) Fix bug where limit was being applied before sorting. Fix bug where offset was not actually allowed to be an argument to GroupBy (weird! guess we weren't testing that very well) Apply "having" after calculating Count(Distinct) aggregate so that having can apply to that. Switch to stable sort to make testing easier. --- executor.go | 125 ++++++++++++++++++++++++++++------------------- executor_test.go | 56 ++++++++++++++++----- pql/ast.go | 1 + 3 files changed, 121 insertions(+), 61 deletions(-) diff --git a/executor.go b/executor.go index 32a1b2f77..7482dff74 100644 --- a/executor.go +++ b/executor.go @@ -2661,7 +2661,7 @@ func (g *groupCountSorter) Less(i, j int) bool { } } - return true + return false } // compareFieldRows returns -1 if a < b, +1 if a > b, and 0 if they @@ -2706,8 +2706,7 @@ func compareFieldRows(a, b FieldRow) int { // getSorter hackily parses the sortSpec and figures out how to sort // the GroupBy results. TODO Probably has about a billion edge case -// bugs. Also needs to take in the Call so it can figure out if fields -// are valid and what order they're in. +// bugs. func getSorter(sortSpec string) (*groupCountSorter, error) { gcs := &groupCountSorter{ fields: []int{}, @@ -2717,19 +2716,16 @@ func getSorter(sortSpec string) (*groupCountSorter, error) { for _, sortField := range sortOn { sortField = strings.TrimSpace(sortField) fieldDir := strings.Split(sortField, " ") - defaultOrder := asc if fieldDir[0] == "count" { gcs.fields = append(gcs.fields, -1) - defaultOrder = desc - } else if fieldDir[0] == "aggregate" { + } else if fieldDir[0] == "aggregate" || fieldDir[0] == "sum" { gcs.fields = append(gcs.fields, -2) - defaultOrder = desc } else { - gcs.fields = append(gcs.fields, 0) // TODO actually figure out which field. probably need the call + return nil, errors.Errorf("sorting is only supported on count, aggregate, or sum, not '%s'", fieldDir[0]) } - if len(fieldDir) == 0 { - gcs.order = append(gcs.order, defaultOrder) + if len(fieldDir) == 1 { + gcs.order = append(gcs.order, desc) } if fieldDir[1] == "asc" { gcs.order = append(gcs.order, asc) @@ -2768,6 +2764,14 @@ func (e *executor) executeGroupBy(ctx context.Context, qcx *Qcx, index string, c if err != nil { return nil, errors.Wrap(err, "parsing sort spec") } + // 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 { + 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 + limit = int(^uint(0) >> 1) } idx := e.Holder.Index(index) @@ -2842,43 +2846,21 @@ func (e *executor) executeGroupBy(ctx context.Context, qcx *Qcx, index string, c } results, _ := other.([]GroupCount) - // Apply having. - if having, hasHaving, err := c.CallArg("having"); err != nil { - return nil, err - } else if hasHaving { - // parse the condition as PQL - if having.Name != "Condition" { - return nil, errors.New("the only supported having call is Condition()") - } - if len(having.Args) != 1 { - return nil, errors.New("Condition() must contain a single condition") - } - for subj, cond := range having.Args { - switch subj { - case "count", "sum": - results = applyConditionToGroupCounts(results, subj, cond.(*pql.Condition)) - default: - return nil, errors.New("Condition() only supports count or sum") - } + // 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 { + results, err = applyLimitAndOffsetToGroupByResult(c, results) + if err != nil { + return nil, errors.Wrap(err, "applying limit/offset") } } - // Apply offset. - if offset, hasOffset, err := c.UintArg("offset"); err != nil { - return nil, err - } else if hasOffset { - if int(offset) < len(results) { - results = results[offset:] - } - } - // Apply limit. - if limit, hasLimit, err := c.UintArg("limit"); err != nil { - return nil, err - } else if hasLimit { - if int(limit) < len(results) { - results = results[:limit] - } - } + // TODO as an optimization, we could apply some "having" + // conditions here long as they aren't on the Count(Distinct) + // aggregate + + // Calculate Count(Distinct) aggregate if requested. aggregate, _, err := c.CallArg("aggregate") if err == nil && aggregate != nil && aggregate.Name == "Count" && len(aggregate.Children) > 0 && aggregate.Children[0].Name == "Distinct" && !opt.Remote { for n, gc := range results { @@ -2898,10 +2880,10 @@ func (e *executor) executeGroupBy(ctx context.Context, qcx *Qcx, index string, c countDistinctIntersect := &pql.Call{ Name: "Count", Children: []*pql.Call{ - &pql.Call{ + { Name: "Distinct", Children: []*pql.Call{ - &pql.Call{ + { Name: "Intersect", Children: intersectRows, }, @@ -2921,11 +2903,56 @@ func (e *executor) executeGroupBy(ctx context.Context, qcx *Qcx, index string, c } } - if sorter != nil { - sorter.data = results - sort.Sort(sorter) + // Apply having. + if having, hasHaving, err := c.CallArg("having"); err != nil { + return nil, err + } else if hasHaving && !opt.Remote { + // parse the condition as PQL + if having.Name != "Condition" { + return nil, errors.New("the only supported having call is Condition()") + } + if len(having.Args) != 1 { + return nil, errors.New("Condition() must contain a single condition") + } + for subj, cond := range having.Args { + switch subj { + case "count", "sum": + results = applyConditionToGroupCounts(results, subj, cond.(*pql.Condition)) + default: + return nil, errors.New("Condition() only supports count or sum") + } + } } + if sorter != nil && !opt.Remote { + sorter.data = results + sort.Stable(sorter) + results, err = applyLimitAndOffsetToGroupByResult(c, results) + if err != nil { + return nil, errors.Wrap(err, "applying limit/offset") + } + } + + return results, nil +} + +func applyLimitAndOffsetToGroupByResult(c *pql.Call, results []GroupCount) ([]GroupCount, error) { + // Apply offset. + if offset, hasOffset, err := c.UintArg("offset"); err != nil { + return nil, err + } else if hasOffset { + if int(offset) < len(results) { + results = results[offset:] + } + } + // Apply limit. + if limit, hasLimit, err := c.UintArg("limit"); err != nil { + return nil, err + } else if hasLimit { + if int(limit) < len(results) { + results = results[:limit] + } + } return results, nil } diff --git a/executor_test.go b/executor_test.go index f6d711971..920977cb8 100644 --- a/executor_test.go +++ b/executor_test.go @@ -6844,6 +6844,18 @@ func variousQueries(t *testing.T, clusterSize int) { {Val: 0, Key: "userE"}, }) + // Create and populate "affinity" int field with negative, positive, zero and null values. + + c.CreateField(t, "users", pilosa.IndexOptions{Keys: true, TrackExistence: true}, "net_worth", pilosa.OptFieldTypeInt(-100000000, 100000000)) + c.ImportIntKey(t, "users", "net_worth", []test.IntKey{ + {Val: 1, Key: "userA"}, + {Val: 10, Key: "userB"}, + {Val: 100, Key: "userC"}, + {Val: 1000, Key: "userD"}, + {Val: 10000, Key: "userE"}, + {Val: 100000, Key: "userF"}, + }) + c.CreateField(t, "users", pilosa.IndexOptions{Keys: true, TrackExistence: true}, "zip_code", pilosa.OptFieldTypeInt(0, 100000)) c.ImportIntKey(t, "users", "zip_code", []test.IntKey{ {Val: 78739, Key: "userA"}, @@ -7008,6 +7020,10 @@ dog,1,0 icecream,6,0 `, }, + { + query: "GroupBy(Rows(field=likes), having=Condition(count>5))", + csvVerifier: "icecream,6,0\n", + }, { query: "GroupBy(Rows(field=likes), filter=Row(affinity>-7))", csvVerifier: `molecula,1,0 @@ -7028,6 +7044,10 @@ dog,1,0 icecream,6,3 `, }, + { + query: "GroupBy(Rows(field=likes), aggregate=Count(Distinct(field=zip_code)), having=Condition(sum>2))", + csvVerifier: "icecream,6,3\n", + }, { query: "GroupBy(Rows(field=likes), filter=Row(affinity>-11), aggregate=Count(Distinct(field=zip_code)))", csvVerifier: `molecula,1,1 @@ -7049,8 +7069,7 @@ icecream,5,3 `, }, { - query: "GroupBy(Rows(field=likes), sort=\"count desc, likes asc\")", - // note, sort is in order of rowID rather than rowKey + query: "GroupBy(Rows(field=likes), sort=\"count desc\")", csvVerifier: `icecream,6,0 molecula,1,0 pilosa,1,0 @@ -7061,15 +7080,28 @@ dog,1,0 `, }, { - query: "GroupBy(Rows(field=likes), sort=\"count desc, likes desc\")", - // note, sort is in order of rowID rather than rowKey - csvVerifier: `icecream,6,0 -dog,1,0 -toucan,1,0 -zebra,1,0 -pangolin,1,0 -pilosa,1,0 -molecula,1,0 + query: "GroupBy(Rows(field=likes), aggregate=Sum(field=net_worth), sort=\"aggregate desc, count asc\")", + csvVerifier: `icecream,6,111111 +dog,1,100000 +toucan,1,10000 +zebra,1,1000 +pangolin,1,100 +pilosa,1,10 +molecula,1,1 +`, + }, + { + query: "GroupBy(Rows(field=likes), aggregate=Sum(field=net_worth), sort=\"aggregate desc, count asc\", limit=3)", + csvVerifier: `icecream,6,111111 +dog,1,100000 +toucan,1,10000 +`, + }, + { + query: "GroupBy(Rows(field=likes), aggregate=Sum(field=net_worth),sort=\"aggregate desc, count asc\",limit=3,offset=2)", + csvVerifier: `toucan,1,10000 +zebra,1,1000 +pangolin,1,100 `, }, } @@ -7088,7 +7120,7 @@ molecula,1,0 // verify everything after header got := csvString[strings.Index(csvString, "\n")+1:] if got != tst.csvVerifier { - t.Errorf("expected '%s', got '%s'", tst.csvVerifier, got) + t.Errorf("expected:\n%s\ngot:\n%s", tst.csvVerifier, got) } // TODO: add HTTP and Postgres and ability to convert diff --git a/pql/ast.go b/pql/ast.go index 056a8fc85..40e2acaba 100644 --- a/pql/ast.go +++ b/pql/ast.go @@ -441,6 +441,7 @@ var callInfoByFunc = map[string]callInfo{ prototypes: map[string]interface{}{ "filter": nil, "limit": int64(0), + "offset": int64(0), "previous": nil, "aggregate": nil, "having": nil, From 6094663e7ab190eaf49e3658c3abb1af0bbd5f43 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Fri, 1 Jan 2021 21:56:59 -0600 Subject: [PATCH 3/5] 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 `, }, { From 6dccb3d6be8af1e0e0a6e12c13bb86edb14bd077 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Sun, 3 Jan 2021 08:34:53 -0600 Subject: [PATCH 4/5] remove (unused) sorting code related to fields, add comments --- executor.go | 62 +++++++++++------------------------------------------ 1 file changed, 13 insertions(+), 49 deletions(-) diff --git a/executor.go b/executor.go index 1863b7b02..586e4468e 100644 --- a/executor.go +++ b/executor.go @@ -2613,6 +2613,7 @@ func (r RowIDs) merge(other RowIDs, limit int) RowIDs { return result } +// order denotes sort order—can be asc or desc (see constants below). type order bool const ( @@ -2620,6 +2621,17 @@ const ( desc order = false ) +// groupCountSorter sorts the output of a GroupBy request (a +// []GroupCount) according to sorting instructions encoded in "fields" +// and "order". +// +// Each field in "fields" is an integer which can be -1 to denote +// sorting on the Count and -2 to denote sorting on the +// sum/aggregate. Currently nothing else is supported, but the idea +// was that if there were positive integers they would be indexes into +// GroupCount.FieldRow and allowing sorting on the values of different +// fields in the group. Each item in "order" corresponds to the same +// index in "fields" and denotes the order of the sort. type groupCountSorter struct { fields []int order []order @@ -2650,60 +2662,12 @@ func (g *groupCountSorter) Less(i, j int) bool { } return gcj.Sum < gci.Sum default: - switch compareFieldRows(gci.Group[fieldIndex], gcj.Group[fieldIndex]) { - case 0: - continue - case -1: - return fieldOrder == asc - case 1: - return fieldOrder == desc - } - + panic("impossible") } } return false } -// compareFieldRows returns -1 if a < b, +1 if a > b, and 0 if they -// are equal. It checks Value, RowKey and RowID, but assumes that -// Field is equal. -func compareFieldRows(a, b FieldRow) int { - if a.Value != nil { - if a.Value == b.Value { - return 0 - } - if *a.Value < *b.Value { - return -1 - } - return +1 - } - if a.RowKey != "" { - if a.RowKey == b.RowKey { - return 0 - } - if a.RowKey < b.RowKey { - return -1 - } - return +1 - } - if a.RowID != 0 { - if a.RowID == b.RowID { - return 0 - } - if a.RowID < b.RowID { - return -1 - } - return +1 - } - // OK, everything in a is zero... so b must be greater unless it's - // also zero (because if a.Value is nil, b.Value must also be - // nil... or something fishy is happening) - if b.RowKey == "" && b.RowID == 0 { - return 0 - } - return -1 -} - // getSorter hackily parses the sortSpec and figures out how to sort // the GroupBy results. TODO Probably has about a billion edge case // bugs. From 216e28a77e208d645f1af47c496b1e7ba7201dcd Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Mon, 4 Jan 2021 10:02:05 -0600 Subject: [PATCH 5/5] add getSorter tests, fix bugs --- executor.go | 34 +++++++-------- executor_internal_test.go | 91 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 18 deletions(-) diff --git a/executor.go b/executor.go index 586e4468e..6536c17f6 100644 --- a/executor.go +++ b/executor.go @@ -2646,21 +2646,17 @@ func (g *groupCountSorter) Less(i, j int) bool { fieldOrder := g.order[idx] switch fieldIndex { case -1: // Count - if gci.Count == gcj.Count { - continue + if gci.Count < gcj.Count { + return fieldOrder == asc + } else if gci.Count > gcj.Count { + return fieldOrder == desc } - if fieldOrder == asc { - return gci.Count < gcj.Count - } - return gcj.Count < gci.Count case -2: // aggregate/Sum - if gci.Sum == gcj.Sum { - continue + if gci.Sum < gcj.Sum { + return fieldOrder == asc + } else if gci.Sum > gcj.Sum { + return fieldOrder == desc } - if fieldOrder == asc { - return gci.Sum < gcj.Sum - } - return gcj.Sum < gci.Sum default: panic("impossible") } @@ -2669,8 +2665,7 @@ func (g *groupCountSorter) Less(i, j int) bool { } // getSorter hackily parses the sortSpec and figures out how to sort -// the GroupBy results. TODO Probably has about a billion edge case -// bugs. +// the GroupBy results. func getSorter(sortSpec string) (*groupCountSorter, error) { gcs := &groupCountSorter{ fields: []int{}, @@ -2679,8 +2674,10 @@ func getSorter(sortSpec string) (*groupCountSorter, error) { sortOn := strings.Split(sortSpec, ",") for _, sortField := range sortOn { sortField = strings.TrimSpace(sortField) - fieldDir := strings.Split(sortField, " ") - if fieldDir[0] == "count" { + fieldDir := strings.Fields(sortField) + if len(fieldDir) == 0 { + return nil, errors.Errorf("invalid sorting directive: '%s'", sortField) + } else if fieldDir[0] == "count" { gcs.fields = append(gcs.fields, -1) } else if fieldDir[0] == "aggregate" || fieldDir[0] == "sum" { gcs.fields = append(gcs.fields, -2) @@ -2690,8 +2687,9 @@ func getSorter(sortSpec string) (*groupCountSorter, error) { if len(fieldDir) == 1 { gcs.order = append(gcs.order, desc) - } - if fieldDir[1] == "asc" { + } else if len(fieldDir) > 2 { + return nil, errors.Errorf("parsing sort directive: '%s': too many elements", sortField) + } else if fieldDir[1] == "asc" { gcs.order = append(gcs.order, asc) } else if fieldDir[1] == "desc" { gcs.order = append(gcs.order, desc) diff --git a/executor_internal_test.go b/executor_internal_test.go index 06e271fde..6de260293 100644 --- a/executor_internal_test.go +++ b/executor_internal_test.go @@ -18,6 +18,7 @@ import ( "context" "encoding/json" "fmt" + "reflect" "strconv" "testing" @@ -384,3 +385,93 @@ func TestToInt64(t *testing.T) { } } } + +func TestGetSorter(t *testing.T) { + tests := []struct { + sortSpec string + expGCS *groupCountSorter + expErr string + }{ + { + sortSpec: "count asc", + expGCS: &groupCountSorter{fields: []int{-1}, order: []order{asc}}, + }, + { + sortSpec: "count asc", + expGCS: &groupCountSorter{fields: []int{-1}, order: []order{asc}}, + }, + { + sortSpec: " count asc", + expGCS: &groupCountSorter{fields: []int{-1}, order: []order{asc}}, + }, + { + sortSpec: " count asc ", + expGCS: &groupCountSorter{fields: []int{-1}, order: []order{asc}}, + }, + { + sortSpec: "count", + expGCS: &groupCountSorter{fields: []int{-1}, order: []order{desc}}, + }, + { + sortSpec: "sum asc", + expGCS: &groupCountSorter{fields: []int{-2}, order: []order{asc}}, + }, + { + sortSpec: "aggregate asc", + expGCS: &groupCountSorter{fields: []int{-2}, order: []order{asc}}, + }, + { + sortSpec: "boondoggle asc", + expErr: "sorting is only supported on count, aggregate, or sum, not 'boondoggle'", + }, + { + sortSpec: "sum asc, count desc", + expGCS: &groupCountSorter{fields: []int{-2, -1}, order: []order{asc, desc}}, + }, + { + sortSpec: "count asc, sum desc", + expGCS: &groupCountSorter{fields: []int{-1, -2}, order: []order{asc, desc}}, + }, + { + sortSpec: " count asc , sum desc ", + expGCS: &groupCountSorter{fields: []int{-1, -2}, order: []order{asc, desc}}, + }, + { + sortSpec: " count asc , sum desc blah", + expErr: "parsing sort directive: 'sum desc blah': too many elements", + }, + { + sortSpec: "count asc, sum fesc", + expErr: "unknown sort direction 'fesc'", + }, + { + sortSpec: " , sum fesc", + expErr: "invalid sorting directive: ''", + }, + { + // weird and useless, but I guess fine? + sortSpec: "count asc,count asc ", + expGCS: &groupCountSorter{fields: []int{-1, -1}, order: []order{asc, asc}}, + }, + } + + for i, tst := range tests { + t.Run(fmt.Sprintf("%s_%d", tst.sortSpec, i), func(t *testing.T) { + gcs, err := getSorter(tst.sortSpec) + if err != nil { + if tst.expErr == "" { + t.Errorf("unexpected error: %v", err) + return + } + if tst.expErr != err.Error() { + t.Errorf("mismatched errors got: '%v', exp: '%s'", err, tst.expErr) + } + return + } + if !reflect.DeepEqual(gcs, tst.expGCS) { + t.Errorf("exp:\n%+v\ngot:\n%v\n", tst.expGCS, gcs) + } + + }) + } +}