Skip to content

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Sep 25, 2025

What this PR does

This PR adds a version number to query plans, and modifies the behaviour of EncodedQueryPlan.ToDecodedPlan to ensure that queriers only evaluate query plans they support.

Which issue(s) this PR fixes or relates to

#12302 and #12551

Checklist

  • Tests updated.
  • [n/a] Documentation added.
  • 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 updated with experimental features.

@charleskorn charleskorn marked this pull request as ready for review September 25, 2025 01:51
@charleskorn charleskorn requested a review from a team as a code owner September 25, 2025 01:51
Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

LGTM

// It returns references to the specified nodeIndices.
func (p *EncodedQueryPlan) ToDecodedPlan(nodeIndices ...int64) (*QueryPlan, []Node, error) {
if p.Version > MaximumSupportedQueryPlanVersion {
return nil, nil, apierror.Newf(apierror.TypeBadData, "query plan has version %v, but the maximum supported query plan version is %v", p.Version, MaximumSupportedQueryPlanVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the use of apierror here? It seems odd to use an error specific to the Prometheus HTTP API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I've been using apierror wrong this whole time - I view it as a way to hint what kind of error it is, so that the API endpoint logic doesn't need to know about every kind of error and how to translate that to an error code / HTTP status code.

Is there something else you had in mind instead?

Copy link
Contributor

@tacole02 tacole02 left a comment

Choose a reason for hiding this comment

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

Changelog looks good

@charleskorn
Copy link
Contributor Author

Given the approval, I'm going to merge this so we can start testing it. We can continue discussing the error type and make any changes in a follow-up PR.

@charleskorn charleskorn enabled auto-merge (squash) September 28, 2025 22:25
@charleskorn charleskorn merged commit 70b0950 into main Sep 28, 2025
37 checks passed
@charleskorn charleskorn deleted the charleskorn/query-plan-versioning branch September 28, 2025 22:40
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.

3 participants