Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jan 16, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f77ad2e
Replace primary/secondary sort fields with an array of sort directives.
ericpromislow Jan 10, 2025
70a8472
Return an error if # sort-fields != # sort-orders
ericpromislow Jan 15, 2025
33cea4c
Rebase against main to pull in separated sql generator and matcher.
ericpromislow Jan 16, 2025
660b347
Add unit tests checking that sort-fields match up with sort-orders.
ericpromislow Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 18 additions & 28 deletions pkg/cache/sql/informer/listoption_indexer.go
View file
Open in desktop
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 {
ericpromislow marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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
Loading