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) + } + + }) + } +}