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.

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

query-frontend: correctly replace @ modifier for split queries #8162

Merged

Conversation

dimitarvdimitrov
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov commented May 21, 2024

What this PR does

The existing code would only evaluate the at modifier for vector selectors, but subquery expressions also can have it. There are no other expression types in promQL which can take the @ modifiers.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@dimitarvdimitrov dimitarvdimitrov requested a review from a team as a code owner May 21, 2024 11:51
The existing code would only evaluate the at modifier for vector selectors, but subquery expressions also can have it.

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/query-frontend/at-modifier-with-subqueries branch from d791d8a to c2ed965 Compare May 21, 2024 11:52
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Comment on lines +670 to 687
switch exprAt := n.(type) {
case *parser.VectorSelector:
switch exprAt.StartOrEnd {
case parser.START:
selector.Timestamp = &start
exprAt.Timestamp = &start
case parser.END:
selector.Timestamp = &end
exprAt.Timestamp = &end
}
selector.StartOrEnd = 0
exprAt.StartOrEnd = 0
case *parser.SubqueryExpr:
switch exprAt.StartOrEnd {
case parser.START:
exprAt.Timestamp = &start
case parser.END:
exprAt.Timestamp = &end
}
exprAt.StartOrEnd = 0
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a brief generic way to write this, so the 7 lines are completely duplicated

Copy link
Contributor

@colega colega left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@dimitarvdimitrov dimitarvdimitrov merged commit b698ab3 into main May 23, 2024
29 checks passed
@dimitarvdimitrov dimitarvdimitrov deleted the dimitar/query-frontend/at-modifier-with-subqueries branch May 23, 2024 13:44
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Nice job!

narqo pushed a commit to narqo/grafana-mimir that referenced this pull request Jun 6, 2024
…fana#8162)

* query-frontend: correctly replace `@` modifier for split queries

The existing code would only evaluate the at modifier for vector selectors, but subquery expressions also can have it.

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* Add CHANGELOG.md entry

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* Rename variable

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

---------

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component/query-frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants