storage: introduce search interface with scoring and filtering#18497
Open
roidelapluie wants to merge 1 commit intoprometheus:mainfrom
Open
storage: introduce search interface with scoring and filtering#18497roidelapluie wants to merge 1 commit intoprometheus:mainfrom
roidelapluie wants to merge 1 commit intoprometheus:mainfrom
Conversation
f3f783f to
48c2b3f
Compare
48c2b3f to
1324caa
Compare
Signed-off-by: Julien Pivotto <[email protected]>
1324caa to
d87f4a3
Compare
aknuds1
requested changes
Apr 17, 2026
Contributor
aknuds1
left a comment
There was a problem hiding this comment.
Directionally good, but my review uncovered some issues and refactoring opportunities.
Comment on lines
+139
to
+154
| switch v := gq.(type) { | ||
| case *mergeGenericQuerier: | ||
| var searchers []Searcher | ||
| for _, q := range v.queriers { | ||
| searchers = append(searchers, collectSearchers(q)...) | ||
| } | ||
| return searchers | ||
| case *genericQuerierAdapter: | ||
| if s, ok := v.q.(Searcher); ok { | ||
| return []Searcher{s} | ||
| } | ||
| default: | ||
| if s, ok := gq.(Searcher); ok { | ||
| return []Searcher{s} | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
I suggest a refactor to simplify:
Suggested change
| switch v := gq.(type) { | |
| case *mergeGenericQuerier: | |
| var searchers []Searcher | |
| for _, q := range v.queriers { | |
| searchers = append(searchers, collectSearchers(q)...) | |
| } | |
| return searchers | |
| case *genericQuerierAdapter: | |
| if s, ok := v.q.(Searcher); ok { | |
| return []Searcher{s} | |
| } | |
| default: | |
| if s, ok := gq.(Searcher); ok { | |
| return []Searcher{s} | |
| } | |
| } | |
| if m, ok := gq.(*mergeGenericQuerier); ok { | |
| var searchers []Searcher | |
| for _, q := range m.queriers { | |
| searchers = append(searchers, collectSearchers(q)...) | |
| } | |
| return searchers | |
| } | |
| if s, ok := searcherFromGenericQuerier(gq); ok { | |
| return []Searcher{s} | |
| } |
| // ApplySearchHints filters, sorts, and limits a slice of string values according to hints, | ||
| // returning scored SearchResult entries. The caller must pass a non-nil hints value. | ||
| func ApplySearchHints(values []string, hints *SearchHints) []SearchResult { | ||
| var results []SearchResult |
Contributor
There was a problem hiding this comment.
I suggest preallocating as the upper bound is known:
Suggested change
| var results []SearchResult | |
| results := make([]SearchResult, 0, len(values)) |
| } | ||
|
|
||
| // mergeSearchSets merges results from multiple Searcher calls, deduplicating by value | ||
| // and taking the maximum score for duplicates. Returns a SearchResultSet. |
Contributor
There was a problem hiding this comment.
Suggested change
| // and taking the maximum score for duplicates. Returns a SearchResultSet. | |
| // and taking the maximum score for duplicates. When hints.CompareFunc is nil, results | |
| // are sorted alphabetically by value. Returns a SearchResultSet. |
| warns.Merge(rs.Warnings()) | ||
| if err := rs.Err(); err != nil { | ||
| _ = rs.Close() | ||
| return ErrSearchResultSet(err) |
Contributor
There was a problem hiding this comment.
Retain warnings:
Suggested change
| return ErrSearchResultSet(err) | |
| return errSearchResultSetWithWarnings(err, warns) |
Comment on lines
+176
to
-183
| // p.lvs[name] is a shared, append-only slice; clone before returning so | ||
| // callers cannot observe future appends or corrupt the internal state. | ||
| if hints != nil && hints.Limit > 0 && len(values) > hints.Limit { | ||
| values = values[:hints.Limit] | ||
| return slices.Clone(values[:hints.Limit]) | ||
| } | ||
|
|
||
| // The slice from p.lvs[name] is shared between all readers, and it is append-only. | ||
| // Since it's shared, we need to make a copy of it before returning it to make | ||
| // sure that no caller modifies the original one by sorting it or filtering it. | ||
| // Since it's append-only, we can do this while not holding the mutex anymore. |
Contributor
There was a problem hiding this comment.
Please revert these changes, they are purely cosmetical and I don't see the revision making things better?
| labelHints.Limit = hints.Limit | ||
| } | ||
|
|
||
| values, err := q.index.SortedLabelValues(ctx, name, labelHints, matchers...) |
Contributor
There was a problem hiding this comment.
Let's optimize:
Suggested change
| values, err := q.index.SortedLabelValues(ctx, name, labelHints, matchers...) | |
| var values []string | |
| var err error | |
| if hints.CompareFunc != nil { | |
| // Custom ordering will re-sort, so skip the index-level sort. | |
| values, err = q.index.LabelValues(ctx, name, labelHints, matchers...) | |
| } else { | |
| values, err = q.index.SortedLabelValues(ctx, name, labelHints, matchers...) | |
| } |
| require.NoError(t, rs.Err()) | ||
| require.NoError(t, rs.Close()) | ||
| return got | ||
| } |
Contributor
There was a problem hiding this comment.
Test blockBaseQuerier.LabelNames with limit:
Suggested change
| } | |
| func TestBlockBaseQuerierLabelNamesLimit(t *testing.T) { | |
| ctx := t.Context() | |
| q := newBlockBaseQuerierForSearch(t, | |
| labels.FromStrings("env", "prod", "job", "api"), | |
| labels.FromStrings("region", "eu"), | |
| ) | |
| t.Run("no limit returns all names", func(t *testing.T) { | |
| names, _, err := q.LabelNames(ctx, nil) | |
| require.NoError(t, err) | |
| require.Len(t, names, 3) | |
| }) | |
| t.Run("limit truncates results", func(t *testing.T) { | |
| names, _, err := q.LabelNames(ctx, &storage.LabelHints{Limit: 1}) | |
| require.NoError(t, err) | |
| require.Len(t, names, 1) | |
| }) | |
| } |
| } | ||
|
|
||
| func TestBlockBaseQuerierSearchLabelNames(t *testing.T) { | ||
| ctx := context.Background() |
Contributor
There was a problem hiding this comment.
[Nit]
Suggested change
| ctx := context.Background() | |
| ctx := t.Context() |
| got := collectSearchResultSet(t, rs) | ||
| require.Empty(t, got) | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| } | |
| t.Run("CompareFunc sorts via unsorted LabelValues path", func(t *testing.T) { | |
| // When CompareFunc is set, SearchLabelValues uses the unsorted | |
| // LabelValues index method. Verify results are sorted by CompareFunc. | |
| rs := q.SearchLabelValues(ctx, "env", &storage.SearchHints{ | |
| CompareFunc: reverseAlphaComparator{}, | |
| }) | |
| got := collectSearchResultSet(t, rs) | |
| require.Len(t, got, 3) | |
| gotValues := make([]string, len(got)) | |
| for i, r := range got { | |
| gotValues[i] = r.Value | |
| } | |
| require.Equal(t, []string{"staging", "prod", "dev"}, gotValues) | |
| }) | |
| } | |
| // reverseAlphaComparator sorts SearchResults in reverse alphabetical order by value. | |
| type reverseAlphaComparator struct{} | |
| func (reverseAlphaComparator) Compare(a, b storage.SearchResult) int { | |
| return cmp.Compare(b.Value, a.Value) | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue(s) does the PR fix:
Release notes for end users (ALL commits must be considered).
Reviewers should verify clarity and quality.