MQE: fix issue where queries containing a duplicate expression and timestamp() produce incorrect results
#11830
Conversation
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
9aa5f2b to
0a75c75
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for propagating timestamp() through vector selectors to avoid incorrect deduplication, ensuring sample timestamps are returned when requested.
- Introduce a
ReturnSampleTimestampsflag onVectorSelectornodes and wire it through planning, execution, and serialization. - Update
Describe,EquivalentTo, andOperatorFactorymethods to account for the new flag. - Extend tests for vector selectors and common subexpression elimination to cover
timestamp()scenarios and regenerate protobuf definitions.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/streamingpromql/planning/core/vector_selector.go | Support ReturnSampleTimestamps in Describe, EquivalentTo, and factory |
| pkg/streamingpromql/planning/core/vector_selector_test.go | Add test cases for Describe and Equivalence with sample timestamps flag |
| pkg/streamingpromql/planning/core/core.proto | Add returnSampleTimestamps field to VectorSelectorDetails |
| pkg/streamingpromql/planning/core/core.pb.go | Regenerate protobuf code to handle the new field |
| pkg/streamingpromql/planning.go | Mark ReturnSampleTimestamps on VectorSelector when planning timestamp() |
| pkg/streamingpromql/optimize/plan/commonsubexpressionelimination/optimization_pass_test.go | Add CSE test scenarios involving timestamp() |
| pkg/streamingpromql/operators/functions/factories.go | Remove redundant setter in TimestampFunctionOperatorFactory |
Comments suppressed due to low confidence (1)
pkg/streamingpromql/planning.go:324
- Add a unit test for
nodeFromExprto verify that parsingtimestamp(foo)correctly setsReturnSampleTimestampson the resulting VectorSelector.
case "timestamp":
|
|
||
| if isSelector { | ||
| selector.ReturnSampleTimestamps = true | ||
| // We'll have already set ReturnSampleTimestamps on the InstantVectorSelector during the planning process, so we don't need to do that here. |
There was a problem hiding this comment.
[nitpick] This comment references the now-removed selector variable and may confuse readers; consider updating or removing the outdated comment.
Suggested change
| // We'll have already set ReturnSampleTimestamps on the InstantVectorSelector during the planning process, so we don't need to do that here. | |
| // The ReturnSampleTimestamps property of InstantVectorSelector is set during the planning process, so no additional action is needed here. |
Copilot uses AI. Check for mistakes.
aknuds1
approved these changes
Jun 24, 2025
mimir-github-bot bot
pushed a commit
that referenced
this pull request
Jun 24, 2025
…imestamp()` produce incorrect results (#11830) #### What this PR does This PR fixes an issue in common subexpression elimination where an expression like `timestamp(foo_time) - foo_time` would produce incorrect results. This happens because the instant vector selector is incorrectly deduplicated in this case: at present, the instant vector selector operator behaves differently if it is wrapped in `timestamp()`, but the query planner did not take this into consideration when identifying duplicate expressions. A future improvement would be to allow the instant vector selector operator to return both the sample value and sample timestamp to different consumers, but given queries like this are rare, I've opted for the simpler approach. #### Which issue(s) this PR fixes or relates to #11189 #### Checklist - [x] Tests updated. - [n/a] Documentation added. - [covered by #10067] `CHANGELOG.md` updated - the order of entries should be `[CHANGE]`, `[FEATURE]`, `[ENHANCEMENT]`, `[BUGFIX]`. If changelog entry is not needed, please add the `changelog-not-needed` label to the PR. - [n/a] [`about-versioning.md`](https://github.com/grafana/mimir/blob/main/docs/sources/mimir/configure/about-versioning.md) updated with experimental features. (cherry picked from commit dd17406)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What this PR does
This PR fixes an issue in common subexpression elimination where an expression like
timestamp(foo_time) - foo_timewould produce incorrect results.This happens because the instant vector selector is incorrectly deduplicated in this case: at present, the instant vector selector operator behaves differently if it is wrapped in
timestamp(), but the query planner did not take this into consideration when identifying duplicate expressions.A future improvement would be to allow the instant vector selector operator to return both the sample value and sample timestamp to different consumers, but given queries like this are rare, I've opted for the simpler approach.
Which issue(s) this PR fixes or relates to
#11189
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]. If changelog entry is not needed, please add thechangelog-not-neededlabel to the PR.about-versioning.mdupdated with experimental features.