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

MQE: fix incorrect query results or "found duplicate series for the match group" errors when binary operation has unsorted labels in on #9482

Merged
merged 3 commits into from
Oct 1, 2024
Merged

MQE: fix incorrect query results or "found duplicate series for the match group" errors when binary operation has unsorted labels in on #9482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a24775e
MQE: fix incorrect query results or "found duplicate series for the m…
charleskorn Oct 1, 2024
bc09209
Add changelog entry
charleskorn Oct 1, 2024
14d415e
Address PR feedback: explain purpose of tests
charleskorn Oct 1, 2024
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* `cortex_alertmanager_alerts`
* `cortex_alertmanager_silences`
* [CHANGE] Cache: Deprecate experimental support for Redis as a cache backend. #9453
* [FEATURE] Querier: add experimental streaming PromQL engine, enabled with `-querier.query-engine=mimir`. #9367 #9368 #9398 #9399 #9403 #9417 #9418 #9419 #9420
* [FEATURE] Querier: add experimental streaming PromQL engine, enabled with `-querier.query-engine=mimir`. #9367 #9368 #9398 #9399 #9403 #9417 #9418 #9419 #9420 #9482
* [FEATURE] Query-frontend: added experimental configuration options `query-frontend.cache-errors` and `query-frontend.results-cache-ttl-for-errors` to allow non-transient responses to be cached. When set to `true` error responses from hitting limits or bad data are cached for a short TTL. #9028
* [FEATURE] gRPC: Support S2 compression. #9322
* `-alertmanager.alertmanager-client.grpc-compression=s2`
Expand Down
2 changes: 2 additions & 0 deletions pkg/streamingpromql/operators/vector_vector_binary_operation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,8 @@ func (b *VectorVectorBinaryOperation) groupKeyFunc() func(labels.Labels) []byte
buf := make([]byte, 0, 1024)

if b.VectorMatching.On {
slices.Sort(b.VectorMatching.MatchingLabels)

return func(l labels.Labels) []byte {
return l.BytesWithLabels(buf, b.VectorMatching.MatchingLabels...)
}
Expand Down
16 changes: 16 additions & 0 deletions pkg/streamingpromql/testdata/ours/binary_operators.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,27 @@ eval range from 0 to 24m step 6m left_side - on(env, pod) right_side
{env="test", pod="a"} -9 -18 -27
{env="test", pod="b"} -36 -45 -54

# Test the same thing again with the grouping labels in a different order.
# (The implementation of binary operations relies on grouping labels being sorted in some places,
# so this test exists to ensure this is done correctly.)
eval range from 0 to 24m step 6m left_side - on(pod, env) right_side
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) should add a comment here on the purpose of these tests and that the load order needs to be preserved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that the load order needs to be preserved

Not sure I follow this part - what are you referring to by "load order" here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the order the series are loaded in (above on line 88) could affect this test right? It's possible they are loaded in a way where the series are already sorted after the on/etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter what order the series are in - the only thing that matters is the order of the label names passed to BytesWithLabels or BytesWithoutLabels.

(and regardless of the order of the series in the load, they'll be retrieved in sorted order due to how chunks streaming works)

{env="prod", pod="a"} -63 -72 -81
{env="test", pod="a"} -9 -18 -27
{env="test", pod="b"} -36 -45 -54

eval range from 0 to 24m step 6m left_side - ignoring(env, pod) right_side
{group="baz"} -33 -42 -51
{group="bar"} -6 -15 -24
{group="foo"} -69 -78 -87

# Test the same thing again with the grouping labels in a different order.
# (The implementation of binary operations relies on grouping labels being sorted in some places,
# so this test exists to ensure this is done correctly.)
eval range from 0 to 24m step 6m left_side - ignoring(pod, env) right_side
{group="baz"} -33 -42 -51
{group="bar"} -6 -15 -24
{group="foo"} -69 -78 -87

clear

# One-to-one matching, but different series match at different time steps, or not at all
Expand Down
Loading