Skip to content

Commit

Permalink
Replace primary/secondary sort fields with an array of sort directive…
Browse files Browse the repository at this point in the history 
…s. (#122)

* Replace primary/secondary sort fields with an array of sort directives.

* Return an error if # sort-fields != # sort-orders

* Rebase against main to pull in separated sql generator and matcher.

* Add unit tests checking that sort-fields match up with sort-orders.
  • Loading branch information
@ericpromislow
ericpromislow authored Jan 16, 2025
1 parent 9e2b687 commit 91795f7
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 69 deletions.
46 changes: 18 additions & 28 deletions pkg/cache/sql/informer/listoption_indexer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,39 +381,29 @@ func (l *ListOptionIndexer) constructQuery(lo ListOptions, partitions []partitio
}
}

// 3- Sorting: ORDER BY clauses (from lo.Sort)
orderByClauses := []string{}
if len(lo.Sort.PrimaryField) > 0 {
columnName := toColumnName(lo.Sort.PrimaryField)
if err := l.validateColumn(columnName); err != nil {
return queryInfo, err
}

direction := "ASC"
if lo.Sort.PrimaryOrder == DESC {
direction = "DESC"
}
orderByClauses = append(orderByClauses, fmt.Sprintf(`f."%s" %s`, columnName, direction))
}
if len(lo.Sort.SecondaryField) > 0 {
columnName := toColumnName(lo.Sort.SecondaryField)
if err := l.validateColumn(columnName); err != nil {
return queryInfo, err
}

direction := "ASC"
if lo.Sort.SecondaryOrder == DESC {
direction = "DESC"
}
orderByClauses = append(orderByClauses, fmt.Sprintf(`f."%s" %s`, columnName, direction))
}

// before proceeding, save a copy of the query and params without LIMIT/OFFSET/ORDER info
// for COUNTing all results later
countQuery := fmt.Sprintf("SELECT COUNT(*) FROM (%s)", query)
countParams := params[:]

if len(orderByClauses) > 0 {
// 3- Sorting: ORDER BY clauses (from lo.Sort)
if len(lo.Sort.Fields) != len(lo.Sort.Orders) {
return nil, fmt.Errorf("sort fields length %d != sort orders length %d", len(lo.Sort.Fields), len(lo.Sort.Orders))
}
if len(lo.Sort.Fields) > 0 {
orderByClauses := []string{}
for i, field := range lo.Sort.Fields {
columnName := toColumnName(field)
if err := l.validateColumn(columnName); err != nil {
return queryInfo, err
}

direction := "ASC"
if lo.Sort.Orders[i] == DESC {
direction = "DESC"
}
orderByClauses = append(orderByClauses, fmt.Sprintf(`f."%s" %s`, columnName, direction))
}
query += "\n ORDER BY "
query += strings.Join(orderByClauses, ", ")
} else {
Expand Down
93 changes: 56 additions & 37 deletions pkg/cache/sql/informer/listoption_indexer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -590,10 +590,11 @@ func TestListByOptions(t *testing.T) {
})

tests = append(tests, testCase{
description: "ListByOptions with Sort.PrimaryField set only should sort on that field only, in ascending order in prepared sql.Stmt",
description: "ListByOptions with only one Sort.Field set should sort on that field only, in ascending order in prepared sql.Stmt",
listOptions: ListOptions{
Sort: Sort{
PrimaryField: []string{"metadata", "somefield"},
Fields: [][]string{{"metadata", "somefield"}},
Orders: []SortOrder{ASC},
},
},
partitions: []partition.Partition{},
Expand All @@ -610,31 +611,36 @@ func TestListByOptions(t *testing.T) {
expectedContToken: "",
expectedErr: nil,
})

tests = append(tests, testCase{
description: "ListByOptions with Sort.SecondaryField set only should sort on that field only, in ascending order in prepared sql.Stmt",
description: "sort one field descending",
listOptions: ListOptions{
Sort: Sort{
SecondaryField: []string{"metadata", "somefield"},
Fields: [][]string{{"metadata", "somefield"}},
Orders: []SortOrder{DESC},
},
},
partitions: []partition.Partition{},
ns: "",
ns: "test5a",
expectedStmt: `SELECT o.object, o.objectnonce, o.dekid FROM "something" o
JOIN "something_fields" f ON o.key = f.key
WHERE
(f."metadata.namespace" = ?) AND
(FALSE)
ORDER BY f."metadata.somefield" ASC`,
ORDER BY f."metadata.somefield" DESC`,
expectedStmtArgs: []any{"test5a"},
returnList: []any{&unstructured.Unstructured{Object: unstrTestObjectMap}, &unstructured.Unstructured{Object: unstrTestObjectMap}},
expectedList: &unstructured.UnstructuredList{Object: map[string]interface{}{"items": []map[string]interface{}{unstrTestObjectMap, unstrTestObjectMap}}, Items: []unstructured.Unstructured{{Object: unstrTestObjectMap}, {Object: unstrTestObjectMap}}},
expectedContToken: "",
expectedErr: nil,
})

tests = append(tests, testCase{
description: "ListByOptions with Sort.PrimaryField and Sort.SecondaryField set should sort on PrimaryField in ascending order first and then sort on SecondaryField in ascending order in prepared sql.Stmt",
description: "ListByOptions sorting on two fields should sort on the first field in ascending order first and then sort on the second field in ascending order in prepared sql.Stmt",
listOptions: ListOptions{
Sort: Sort{
PrimaryField: []string{"metadata", "somefield"},
SecondaryField: []string{"status", "someotherfield"},
Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}},
Orders: []SortOrder{ASC, ASC},
},
},
partitions: []partition.Partition{},
Expand All @@ -649,13 +655,13 @@ func TestListByOptions(t *testing.T) {
expectedContToken: "",
expectedErr: nil,
})

tests = append(tests, testCase{
description: "ListByOptions with Sort.PrimaryField and Sort.SecondaryField set and PrimaryOrder set to DESC should sort on PrimaryField in descending order first and then sort on SecondaryField in ascending order in prepared sql.Stmt",
description: "ListByOptions sorting on two fields should sort on the first field in descending order first and then sort on the second field in ascending order in prepared sql.Stmt",
listOptions: ListOptions{
Sort: Sort{
PrimaryField: []string{"metadata", "somefield"},
SecondaryField: []string{"status", "someotherfield"},
PrimaryOrder: DESC,
Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}},
Orders: []SortOrder{DESC, ASC},
},
},
partitions: []partition.Partition{},
Expand All @@ -670,31 +676,13 @@ func TestListByOptions(t *testing.T) {
expectedContToken: "",
expectedErr: nil,
})

tests = append(tests, testCase{
description: "ListByOptions with Sort.SecondaryField set and Sort.PrimaryOrder set to descending should sort on that SecondaryField in ascending order only and ignore PrimaryOrder in prepared sql.Stmt",
listOptions: ListOptions{
Sort: Sort{
SecondaryField: []string{"status", "someotherfield"},
PrimaryOrder: DESC,
},
},
partitions: []partition.Partition{},
ns: "",
expectedStmt: `SELECT o.object, o.objectnonce, o.dekid FROM "something" o
JOIN "something_fields" f ON o.key = f.key
WHERE
(FALSE)
ORDER BY f."status.someotherfield" ASC`,
returnList: []any{&unstructured.Unstructured{Object: unstrTestObjectMap}, &unstructured.Unstructured{Object: unstrTestObjectMap}},
expectedList: &unstructured.UnstructuredList{Object: map[string]interface{}{"items": []map[string]interface{}{unstrTestObjectMap, unstrTestObjectMap}}, Items: []unstructured.Unstructured{{Object: unstrTestObjectMap}, {Object: unstrTestObjectMap}}},
expectedContToken: "",
expectedErr: nil,
})
tests = append(tests, testCase{
description: "ListByOptions with Sort.PrimaryOrder set only should sort on default primary and secondary fields in ascending order in prepared sql.Stmt",
description: "ListByOptions sorting when # fields != # sort orders should return an error",
listOptions: ListOptions{
Sort: Sort{
PrimaryOrder: DESC,
Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}},
Orders: []SortOrder{DESC, ASC, ASC},
},
},
partitions: []partition.Partition{},
Expand All @@ -703,12 +691,13 @@ func TestListByOptions(t *testing.T) {
JOIN "something_fields" f ON o.key = f.key
WHERE
(FALSE)
ORDER BY f."metadata.name" ASC `,
ORDER BY f."metadata.somefield" DESC, f."status.someotherfield" ASC`,
returnList: []any{&unstructured.Unstructured{Object: unstrTestObjectMap}, &unstructured.Unstructured{Object: unstrTestObjectMap}},
expectedList: &unstructured.UnstructuredList{Object: map[string]interface{}{"items": []map[string]interface{}{unstrTestObjectMap, unstrTestObjectMap}}, Items: []unstructured.Unstructured{{Object: unstrTestObjectMap}, {Object: unstrTestObjectMap}}},
expectedContToken: "",
expectedErr: nil,
expectedErr: fmt.Errorf("sort fields length 2 != sort orders length 3"),
})

tests = append(tests, testCase{
description: "ListByOptions with Pagination.PageSize set should set limit to PageSize in prepared sql.Stmt",
listOptions: ListOptions{
Expand Down Expand Up @@ -1433,6 +1422,36 @@ func TestConstructQuery(t *testing.T) {
expectedErr: nil,
})

tests = append(tests, testCase{
description: "ConstructQuery: sorting when # fields < # sort orders should return an error",
listOptions: ListOptions{
Sort: Sort{
Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}},
Orders: []SortOrder{DESC, ASC, ASC},
},
},
partitions: []partition.Partition{},
ns: "",
expectedStmt: "",
expectedStmtArgs: []any{},
expectedErr: fmt.Errorf("sort fields length 2 != sort orders length 3"),
})

tests = append(tests, testCase{
description: "ConstructQuery: sorting when # fields > # sort orders should return an error",
listOptions: ListOptions{
Sort: Sort{
Fields: [][]string{{"metadata", "somefield"}, {"status", "someotherfield"}, {"metadata", "labels", "a1"}, {"metadata", "labels", "a2"}},
Orders: []SortOrder{DESC, ASC, ASC},
},
},
partitions: []partition.Partition{},
ns: "",
expectedStmt: "",
expectedStmtArgs: []any{},
expectedErr: fmt.Errorf("sort fields length 4 != sort orders length 3"),
})

t.Parallel()
for _, test := range tests {
t.Run(test.description, func(t *testing.T) {
Expand Down
7 changes: 3 additions & 4 deletions pkg/cache/sql/informer/listoptions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,10 @@ type OrFilter struct {
// The subfield to sort by is represented in a request query using . notation, e.g. 'metadata.name'.
// The subfield is internally represented as a slice, e.g. [metadata, name].
// The order is represented by prefixing the sort key by '-', e.g. sort=-metadata.name.
// e.g. To sort internal clusters first followed by clusters in alpha order: sort=-spec.internal,spec.displayName
type Sort struct {
PrimaryField []string
SecondaryField []string
PrimaryOrder SortOrder
SecondaryOrder SortOrder
Fields [][]string
Orders []SortOrder
}

// Pagination represents how to return paginated results.
Expand Down

0 comments on commit 91795f7

Please sign in to comment.