Sort labels (#527)

* Support sorting on metadata.labels.NAME The key to doing this is if we want to sort on, say, `metadata.labels.foo`, we need to search for all rows with a label of the name `foo` in all the various join tables we create for each label the query references. We ignore nulls by giving them lowest priority using "NULLS LAST" ("NULLS FIRST" if sorting in descending order). * Ensure labels that are mentioned only in sort params are still selected. If we don't do this -- say we sort on metadata.labels.foo but never make a test on it, the sort resuilts are ignored. * Remove extraneous debugger statements.
2025-09-01 15:37:31 +00:00 · 2025-03-04 09:30:14 -08:00
parent a85993129e
commit 0edba0da3e
3 changed files with 294 additions and 26 deletions
--- a/pkg/sqlcache/informer/indexer_test.go
+++ b/pkg/sqlcache/informer/indexer_test.go
@@ -177,7 +177,7 @@ func TestAfterUpsert(t *testing.T) {
 		deleteIndicesStmt := NewMockStmt(gomock.NewController(t))
 		addIndexStmt := NewMockStmt(gomock.NewController(t))
 		indexer := &Indexer{
-			ctx:               context.Background(),
+			ctx:   context.Background(),
 			Store: store,
 			indexers: map[string]cache.IndexFunc{
 				"a": func(obj interface{}) ([]string, error) {
@@ -200,7 +200,7 @@ func TestAfterUpsert(t *testing.T) {
 		objKey := "key"
 		deleteIndicesStmt := NewMockStmt(gomock.NewController(t))
 		indexer := &Indexer{
-			ctx:               context.Background(),
+			ctx:   context.Background(),
 			Store: store,

 			indexers: map[string]cache.IndexFunc{
@@ -223,7 +223,7 @@ func TestAfterUpsert(t *testing.T) {
 		addIndexStmt := NewMockStmt(gomock.NewController(t))
 		objKey := "key"
 		indexer := &Indexer{
-			ctx:               context.Background(),
+			ctx:   context.Background(),
 			Store: store,
 			indexers: map[string]cache.IndexFunc{
 				"a": func(obj interface{}) ([]string, error) {
--- a/pkg/sqlcache/informer/listoption_indexer.go
+++ b/pkg/sqlcache/informer/listoption_indexer.go
@@ -267,8 +267,10 @@ type QueryInfo struct {
 }

 func (l *ListOptionIndexer) constructQuery(lo ListOptions, partitions []partition.Partition, namespace string, dbName string) (*QueryInfo, error) {
+	ensureSortLabelsAreSelected(&lo)
 	queryInfo := &QueryInfo{}
-	queryHasLabelFilter := hasLabelFilter(lo.Filters)
+	queryUsesLabels := hasLabelFilter(lo.Filters)
+	joinTableIndexByLabelName := make(map[string]int)

 	// First, what kind of filtering will we be doing?
 	// 1- Intro: SELECT and JOIN clauses
@@ -276,18 +278,26 @@ func (l *ListOptionIndexer) constructQuery(lo ListOptions, partitions []partitio
 	// but it's possible that a key has no associated labels, so if we're doing a
 	// non-existence test on labels we need to do a LEFT OUTER JOIN
 	distinctModifier := ""
-	if queryHasLabelFilter {
+	if queryUsesLabels {
 		distinctModifier = " DISTINCT"
 	}
 	query := fmt.Sprintf(`SELECT%s o.object, o.objectnonce, o.dekid FROM "%s" o`, distinctModifier, dbName)
 	query += "\n  "
 	query += fmt.Sprintf(`JOIN "%s_fields" f ON o.key = f.key`, dbName)
-	if queryHasLabelFilter {
-		for i, orFilters := range lo.Filters {
-			if hasLabelFilter([]OrFilter{orFilters}) {
-				query += "\n  "
-				// Make the lt index 1-based for readability
-				query += fmt.Sprintf(`LEFT OUTER JOIN "%s_labels" lt%d ON o.key = lt%d.key`, dbName, i+1, i+1)
+	if queryUsesLabels {
+		for i, orFilter := range lo.Filters {
+			for j, filter := range orFilter.Filters {
+				if isLabelFilter(&filter) {
+					labelName := filter.Field[2]
+					_, ok := joinTableIndexByLabelName[labelName]
+					if !ok {
+						// Make the lt index 1-based for readability
+						jtIndex := i + j + 1
+						joinTableIndexByLabelName[labelName] = jtIndex
+						query += "\n  "
+						query += fmt.Sprintf(`LEFT OUTER JOIN "%s_labels" lt%d ON o.key = lt%d.key`, dbName, jtIndex, jtIndex)
+					}
+				}
 			}
 		}
 	}
@@ -295,8 +305,8 @@ func (l *ListOptionIndexer) constructQuery(lo ListOptions, partitions []partitio

 	// 2- Filtering: WHERE clauses (from lo.Filters)
 	whereClauses := []string{}
-	for i, orFilters := range lo.Filters {
-		orClause, orParams, err := l.buildORClauseFromFilters(i+1, orFilters, dbName)
+	for _, orFilters := range lo.Filters {
+		orClause, orParams, err := l.buildORClauseFromFilters(orFilters, dbName, joinTableIndexByLabelName)
 		if err != nil {
 			return queryInfo, err
 		}
@@ -384,16 +394,24 @@ func (l *ListOptionIndexer) constructQuery(lo ListOptions, partitions []partitio
 	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
+			if isLabelsFieldList(field) {
+				clause, sortParam, err := buildSortLabelsClause(field[2], joinTableIndexByLabelName, lo.Sort.Orders[i] == ASC)
+				if err != nil {
+					return nil, err
+				}
+				orderByClauses = append(orderByClauses, clause)
+				params = append(params, sortParam)
+			} else {
+				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))
 			}
-
-			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, ", ")
@@ -521,7 +539,7 @@ func (l *ListOptionIndexer) validateColumn(column string) error {
 }

 // buildORClause creates an SQLite compatible query that ORs conditions built from passed filters
-func (l *ListOptionIndexer) buildORClauseFromFilters(index int, orFilters OrFilter, dbName string) (string, []any, error) {
+func (l *ListOptionIndexer) buildORClauseFromFilters(orFilters OrFilter, dbName string, joinTableIndexByLabelName map[string]int) (string, []any, error) {
 	var params []any
 	clauses := make([]string, 0, len(orFilters.Filters))
 	var newParams []any
@@ -530,6 +548,10 @@ func (l *ListOptionIndexer) buildORClauseFromFilters(index int, orFilters OrFilt

 	for _, filter := range orFilters.Filters {
 		if isLabelFilter(&filter) {
+			index, ok := joinTableIndexByLabelName[filter.Field[2]]
+			if !ok {
+				return "", nil, fmt.Errorf("internal error: no index for label name %s", filter.Field[2])
+			}
 			newClause, newParams, err = l.getLabelFilter(index, filter, dbName)
 		} else {
 			newClause, newParams, err = l.getFieldFilter(filter)
@@ -549,6 +571,80 @@ func (l *ListOptionIndexer) buildORClauseFromFilters(index int, orFilters OrFilt
 	return fmt.Sprintf("(%s)", strings.Join(clauses, ") OR (")), params, nil
 }

+func buildSortLabelsClause(labelName string, joinTableIndexByLabelName map[string]int, isAsc bool) (string, string, error) {
+	ltIndex, ok := joinTableIndexByLabelName[labelName]
+	if !ok {
+		return "", "", fmt.Errorf(`internal error: no join-table index given for labelName "%s"`, labelName)
+	}
+	stmt := fmt.Sprintf(`CASE lt%d.label WHEN ? THEN lt%d.value ELSE NULL END`, ltIndex, ltIndex)
+	dir := "ASC"
+	nullsPosition := "LAST"
+	if !isAsc {
+		dir = "DESC"
+		nullsPosition = "FIRST"
+	}
+	return fmt.Sprintf("(%s) %s NULLS %s", stmt, dir, nullsPosition), labelName, nil
+}
+
+// If the user tries to sort on a particular label without mentioning it in a query,
+// it turns out that the sort-directive is ignored. It could be that the sqlite engine
+// is doing some kind of optimization on the `select distinct`, but verifying an otherwise
+// unreferenced label exists solves this problem.
+// And it's better to do this by modifying the ListOptions object.
+// There are no thread-safety issues in doing this because the ListOptions object is
+// created in Store.ListByPartitions, and that ends up calling ListOptionIndexer.ConstructQuery.
+// No other goroutines access this object.
+func ensureSortLabelsAreSelected(lo *ListOptions) {
+	if len(lo.Sort.Fields) == 0 {
+		return
+	}
+	unboundSortLabels := make(map[string]bool)
+	for _, fieldList := range lo.Sort.Fields {
+		if isLabelsFieldList(fieldList) {
+			unboundSortLabels[fieldList[2]] = true
+		}
+	}
+	if len(unboundSortLabels) == 0 {
+		return
+	}
+	// If we have sort directives but no filters, add an exists-filter for each label.
+	if lo.Filters == nil || len(lo.Filters) == 0 {
+		lo.Filters = make([]OrFilter, 1)
+		lo.Filters[0].Filters = make([]Filter, len(unboundSortLabels))
+		i := 0
+		for labelName := range unboundSortLabels {
+			lo.Filters[0].Filters[i] = Filter{
+				Field: []string{"metadata", "labels", labelName},
+				Op:    Exists,
+			}
+			i++
+		}
+		return
+	}
+	// The gotcha is we have to bind the labels for each set of orFilters, so copy them each time
+	for i, orFilters := range lo.Filters {
+		copyUnboundSortLabels := make(map[string]bool, len(unboundSortLabels))
+		for k, v := range unboundSortLabels {
+			copyUnboundSortLabels[k] = v
+		}
+		for _, filter := range orFilters.Filters {
+			if isLabelFilter(&filter) {
+				copyUnboundSortLabels[filter.Field[2]] = false
+			}
+		}
+		// Now for any labels that are still true, add another where clause
+		for labelName, needsBinding := range copyUnboundSortLabels {
+			if needsBinding {
+				// `orFilters` is a copy of lo.Filters[i], so reference the original.
+				lo.Filters[i].Filters = append(lo.Filters[i].Filters, Filter{
+					Field: []string{"metadata", "labels", labelName},
+					Op:    Exists,
+				})
+			}
+		}
+	}
+}
+
 // Possible ops from the k8s parser:
 // KEY = and == (same) VALUE
 // KEY != VALUE
@@ -846,6 +942,10 @@ func hasLabelFilter(filters []OrFilter) bool {
 	return false
 }

+func isLabelsFieldList(fields []string) bool {
+	return len(fields) == 3 && fields[0] == "metadata" && fields[1] == "labels"
+}
+
 // toUnstructuredList turns a slice of unstructured objects into an unstructured.UnstructuredList
 func toUnstructuredList(items []any) *unstructured.UnstructuredList {
 	objectItems := make([]map[string]any, len(items))
--- a/pkg/sqlcache/informer/listoption_indexer_test.go
+++ b/pkg/sqlcache/informer/listoption_indexer_test.go
@@ -690,7 +690,32 @@ func TestListByOptions(t *testing.T) {
 	})

 	tests = append(tests, testCase{
-		description: "ListByOptions sorting on two complex 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",
+		description: "sort one unbound label descending",
+		listOptions: ListOptions{
+			Sort: Sort{
+				Fields: [][]string{{"metadata", "labels", "flip"}},
+				Orders: []SortOrder{DESC},
+			},
+		},
+		partitions: []partition.Partition{},
+		ns:         "test5a",
+		expectedStmt: `SELECT DISTINCT o.object, o.objectnonce, o.dekid FROM "something" o
+  JOIN "something_fields" f ON o.key = f.key
+  LEFT OUTER JOIN "something_labels" lt1 ON o.key = lt1.key
+  WHERE
+    (lt1.label = ?) AND
+    (f."metadata.namespace" = ?) AND
+    (FALSE)
+  ORDER BY (CASE lt1.label WHEN ? THEN lt1.value ELSE NULL END) DESC NULLS FIRST`,
+		expectedStmtArgs:  []any{"flip", "test5a", "flip"},
+		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 sorting on two complex fields should sort on the first field in ascending order first and then sort on the second labels field in ascending order in prepared sql.Stmt",
 		listOptions: ListOptions{
 			Sort: Sort{
 				Fields: [][]string{{"metadata", "fields", "3"}, {"metadata", "labels", "stub.io/candy"}},
@@ -700,11 +725,14 @@ func TestListByOptions(t *testing.T) {
 		extraIndexedFields: []string{"metadata.fields[3]", "metadata.labels[stub.io/candy]"},
 		partitions:         []partition.Partition{},
 		ns:                 "",
-		expectedStmt: `SELECT o.object, o.objectnonce, o.dekid FROM "something" o
+		expectedStmt: `SELECT DISTINCT o.object, o.objectnonce, o.dekid FROM "something" o
  JOIN "something_fields" f ON o.key = f.key
+  LEFT OUTER JOIN "something_labels" lt1 ON o.key = lt1.key
  WHERE
+    (lt1.label = ?) AND
    (FALSE)
-  ORDER BY f."metadata.fields[3]" ASC, f."metadata.labels[stub.io/candy]" ASC`,
+  ORDER BY f."metadata.fields[3]" ASC, (CASE lt1.label WHEN ? THEN lt1.value ELSE NULL END) ASC NULLS LAST`,
+		expectedStmtArgs:  []any{"stub.io/candy", "stub.io/candy"},
 		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: "",
@@ -1504,6 +1532,39 @@ func TestConstructQuery(t *testing.T) {
 		expectedErr:      nil,
 	})

+	tests = append(tests, testCase{
+		description: "TestConstructQuery: handles == statements for label statements, match partial, sort on metadata.queryField1",
+		listOptions: ListOptions{
+			Filters: []OrFilter{
+				{
+					[]Filter{
+						{
+							Field:   []string{"metadata", "labels", "labelEqualPartial"},
+							Matches: []string{"somevalue"},
+							Op:      Eq,
+							Partial: true,
+						},
+					},
+				},
+			},
+			Sort: Sort{
+				Fields: [][]string{{"metadata", "queryField1"}},
+				Orders: []SortOrder{ASC},
+			},
+		},
+		partitions: []partition.Partition{},
+		ns:         "",
+		expectedStmt: `SELECT DISTINCT o.object, o.objectnonce, o.dekid FROM "something" o
+  JOIN "something_fields" f ON o.key = f.key
+  LEFT OUTER JOIN "something_labels" lt1 ON o.key = lt1.key
+  WHERE
+    (lt1.label = ? AND lt1.value LIKE ? ESCAPE '\') AND
+    (FALSE)
+  ORDER BY f."metadata.queryField1" ASC`,
+		expectedStmtArgs: []any{"labelEqualPartial", "%somevalue%"},
+		expectedErr:      nil,
+	})
+
 	tests = append(tests, testCase{
 		description: "ConstructQuery: sorting when # fields < # sort orders should return an error",
 		listOptions: ListOptions{
@@ -1519,6 +1580,65 @@ func TestConstructQuery(t *testing.T) {
 		expectedErr:      fmt.Errorf("sort fields length 2 != sort orders length 3"),
 	})

+	tests = append(tests, testCase{
+		description: "TestConstructQuery: sort on label statements with no query",
+		listOptions: ListOptions{
+			Sort: Sort{
+				Fields: [][]string{{"metadata", "labels", "this"}},
+				Orders: []SortOrder{ASC},
+			},
+		},
+		partitions: []partition.Partition{},
+		ns:         "",
+		expectedStmt: `SELECT DISTINCT o.object, o.objectnonce, o.dekid FROM "something" o
+  JOIN "something_fields" f ON o.key = f.key
+  LEFT OUTER JOIN "something_labels" lt1 ON o.key = lt1.key
+  WHERE
+    (lt1.label = ?) AND
+    (FALSE)
+  ORDER BY (CASE lt1.label WHEN ? THEN lt1.value ELSE NULL END) ASC NULLS LAST`,
+		expectedStmtArgs: []any{"this", "this"},
+		expectedErr:      nil,
+	})
+
+	tests = append(tests, testCase{
+		description: "TestConstructQuery: sort and query on both labels and non-labels without overlap",
+		listOptions: ListOptions{
+			Filters: []OrFilter{
+				{
+					[]Filter{
+						{
+							Field:   []string{"metadata", "queryField1"},
+							Matches: []string{"toys"},
+							Op:      Eq,
+						},
+						{
+							Field:   []string{"metadata", "labels", "jamb"},
+							Matches: []string{"juice"},
+							Op:      Eq,
+						},
+					},
+				},
+			},
+			Sort: Sort{
+				Fields: [][]string{{"metadata", "labels", "this"}, {"status", "queryField2"}},
+				Orders: []SortOrder{ASC, DESC},
+			},
+		},
+		partitions: []partition.Partition{},
+		ns:         "",
+		expectedStmt: `SELECT DISTINCT o.object, o.objectnonce, o.dekid FROM "something" o
+  JOIN "something_fields" f ON o.key = f.key
+  LEFT OUTER JOIN "something_labels" lt2 ON o.key = lt2.key
+  LEFT OUTER JOIN "something_labels" lt3 ON o.key = lt3.key
+  WHERE
+    ((f."metadata.queryField1" = ?) OR (lt2.label = ? AND lt2.value = ?) OR (lt3.label = ?)) AND
+    (FALSE)
+  ORDER BY (CASE lt3.label WHEN ? THEN lt3.value ELSE NULL END) ASC NULLS LAST, f."status.queryField2" DESC`,
+		expectedStmtArgs: []any{"toys", "jamb", "juice", "this", "this"},
+		expectedErr:      nil,
+	})
+
 	tests = append(tests, testCase{
 		description: "ConstructQuery: sorting when # fields > # sort orders should return an error",
 		listOptions: ListOptions{
@@ -1620,3 +1740,51 @@ func TestSmartJoin(t *testing.T) {
 		})
 	}
 }
+
+func TestBuildSortLabelsClause(t *testing.T) {
+	type testCase struct {
+		description               string
+		labelName                 string
+		joinTableIndexByLabelName map[string]int
+		direction                 bool
+		expectedStmt              string
+        expectedParam             string
+		expectedErr               string
+	}
+
+	var tests []testCase
+	tests = append(tests, testCase{
+		description: "TestBuildSortClause: empty index list errors",
+		labelName:   "emptyListError",
+		expectedErr: `internal error: no join-table index given for labelName "emptyListError"`,
+	})
+	tests = append(tests, testCase{
+		description:               "TestBuildSortClause: hit ascending",
+		labelName:                 "testBSL1",
+		joinTableIndexByLabelName: map[string]int{"testBSL1": 3},
+		direction:                 true,
+		expectedStmt:              `(CASE lt3.label WHEN ? THEN lt3.value ELSE NULL END) ASC NULLS LAST`,
+		expectedParam:             "testBSL1",
+	})
+	tests = append(tests, testCase{
+		description:               "TestBuildSortClause: hit descending",
+		labelName:                 "testBSL2",
+		joinTableIndexByLabelName: map[string]int{"testBSL2": 4},
+		direction:                 false,
+		expectedStmt:              `(CASE lt4.label WHEN ? THEN lt4.value ELSE NULL END) DESC NULLS FIRST`,
+		expectedParam:             "testBSL2",
+	})
+	t.Parallel()
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			stmt, param, err := buildSortLabelsClause(test.labelName, test.joinTableIndexByLabelName, test.direction)
+			if test.expectedErr != "" {
+				assert.Equal(t, test.expectedErr, err.Error())
+			} else {
+				assert.Nil(t, err)
+				assert.Equal(t, test.expectedStmt, stmt)
+				assert.Equal(t, test.expectedParam, param)
+			}
+		})
+	}
+}