Skip to content

Conversation

charleskorn
Copy link
Contributor

What this PR does

This PR refactors the Query type to avoid using the EvalStmt where possible.

This will make it easier to introduce queries based on query plans, as query plans may not have an equivalent PromQL statement.

Which issue(s) this PR fixes or relates to

(none)

Checklist

  • Tests updated.
  • [n/a] Documentation added.
  • [covered by Mimir Query Engine #10067] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • [n/a] about-versioning.md updated with experimental features.

@charleskorn charleskorn marked this pull request as ready for review March 26, 2025 00:13
@charleskorn charleskorn requested a review from a team as a code owner March 26, 2025 00:13
Copy link
Contributor

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

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

lgtm, just a nit

@@ -37,6 +37,7 @@ import (
var errQueryCancelled = cancellation.NewErrorf("query execution cancelled")
var errQueryClosed = cancellation.NewErrorf("Query.Close() called")
var errQueryFinished = cancellation.NewErrorf("query execution finished")
var errStringForRangeQuery = errors.New("query expression produces a string, but expression for range queries must produce an instant vector or scalar")
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) could make this generic to use in newQuery too

Suggested change
var errStringForRangeQuery = errors.New("query expression produces a string, but expression for range queries must produce an instant vector or scalar")
var strStringForRangeQuery = "query expression produces a %s but expression for range queries must produce an instant vector or scalar"

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've tackled this in a slightly different way in f178f97: I've changed Exec to not check if it's an instant query if we get a StringOperator. This makes the behaviour consistent with how we handle RangeVectorOperators, which are also not supported as the root operator for instant queries.

@charleskorn charleskorn force-pushed the charleskorn/mqe-dont-use-statement branch from 6799a1a to f178f97 Compare March 26, 2025 02:02
@charleskorn charleskorn enabled auto-merge (squash) March 26, 2025 02:07
@charleskorn charleskorn merged commit 862de5b into main Mar 26, 2025
41 of 51 checks passed
@charleskorn charleskorn deleted the charleskorn/mqe-dont-use-statement branch March 26, 2025 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants