From 2429dde91299214e99f38ac96b203de2f643d7ee Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sat, 19 Jul 2025 13:40:55 +0100 Subject: [PATCH 1/8] [PERF] PromQL: Don't wrap constant expressions as time-invariant This should mean we can stop unwrapping them later. Fix up tests that check very specific details. Signed-off-by: Bryan Boreham --- promql/engine.go | 38 ++++++++++++++++++++------------------ promql/engine_test.go | 16 ++++++---------- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index f5ee591d3b..da1fce8d1a 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -3738,8 +3738,8 @@ func PreprocessExpr(expr parser.Expr, start, end time.Time, step time.Duration) return nil, err } - isStepInvariant := preprocessExprHelper(expr, start, end) - if isStepInvariant { + _, shouldWrap := preprocessExprHelper(expr, start, end) + if shouldWrap { return newStepInvariantExpr(expr), nil } return expr, nil @@ -3749,7 +3749,7 @@ func PreprocessExpr(expr parser.Expr, start, end time.Time, step time.Duration) // with a StepInvariantExpr wherever it's step invariant. The returned boolean is true if the // passed expression qualifies to be wrapped by StepInvariantExpr. // It also resolves the preprocessors. -func preprocessExprHelper(expr parser.Expr, start, end time.Time) bool { +func preprocessExprHelper(expr parser.Expr, start, end time.Time) (isStepInvariant, shouldWrap bool) { switch n := expr.(type) { case *parser.VectorSelector: switch n.StartOrEnd { @@ -3758,46 +3758,48 @@ func preprocessExprHelper(expr parser.Expr, start, end time.Time) bool { case parser.END: n.Timestamp = makeInt64Pointer(timestamp.FromTime(end)) } - return n.Timestamp != nil + return n.Timestamp != nil, n.Timestamp != nil case *parser.AggregateExpr: return preprocessExprHelper(n.Expr, start, end) case *parser.BinaryExpr: - isInvariant1, isInvariant2 := preprocessExprHelper(n.LHS, start, end), preprocessExprHelper(n.RHS, start, end) + isInvariant1, shouldWrap1 := preprocessExprHelper(n.LHS, start, end) + isInvariant2, shouldWrap2 := preprocessExprHelper(n.RHS, start, end) if isInvariant1 && isInvariant2 { - return true + return true, true } - if isInvariant1 { + if shouldWrap1 { n.LHS = newStepInvariantExpr(n.LHS) } - if isInvariant2 { + if shouldWrap2 { n.RHS = newStepInvariantExpr(n.RHS) } - return false + return false, false case *parser.Call: _, ok := AtModifierUnsafeFunctions[n.Func.Name] isStepInvariant := !ok - isStepInvariantSlice := make([]bool, len(n.Args)) + shouldWrap := make([]bool, len(n.Args)) for i := range n.Args { - isStepInvariantSlice[i] = preprocessExprHelper(n.Args[i], start, end) - isStepInvariant = isStepInvariant && isStepInvariantSlice[i] + var argIsStepInvariant bool + argIsStepInvariant, shouldWrap[i] = preprocessExprHelper(n.Args[i], start, end) + isStepInvariant = isStepInvariant && argIsStepInvariant } if isStepInvariant { // The function and all arguments are step invariant. - return true + return true, true } - for i, isi := range isStepInvariantSlice { + for i, isi := range shouldWrap { if isi { n.Args[i] = newStepInvariantExpr(n.Args[i]) } } - return false + return false, false case *parser.MatrixSelector: return preprocessExprHelper(n.VectorSelector, start, end) @@ -3808,7 +3810,7 @@ func preprocessExprHelper(expr parser.Expr, start, end time.Time) bool { // Hence we wrap the inside of subquery irrespective of // @ on subquery (given it is also step invariant) so that // it is evaluated only once w.r.t. the start time of subquery. - isInvariant := preprocessExprHelper(n.Expr, start, end) + isInvariant, _ := preprocessExprHelper(n.Expr, start, end) if isInvariant { n.Expr = newStepInvariantExpr(n.Expr) } @@ -3818,7 +3820,7 @@ func preprocessExprHelper(expr parser.Expr, start, end time.Time) bool { case parser.END: n.Timestamp = makeInt64Pointer(timestamp.FromTime(end)) } - return n.Timestamp != nil + return n.Timestamp != nil, n.Timestamp != nil case *parser.ParenExpr: return preprocessExprHelper(n.Expr, start, end) @@ -3827,7 +3829,7 @@ func preprocessExprHelper(expr parser.Expr, start, end time.Time) bool { return preprocessExprHelper(n.Expr, start, end) case *parser.StringLiteral, *parser.NumberLiteral: - return true + return true, false } panic(fmt.Sprintf("found unexpected node %#v", expr)) diff --git a/promql/engine_test.go b/promql/engine_test.go index ce5ef6efd7..e98fde64e7 100644 --- a/promql/engine_test.go +++ b/promql/engine_test.go @@ -2304,20 +2304,16 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) { }{ { input: "123.4567", - expected: &parser.StepInvariantExpr{ - Expr: &parser.NumberLiteral{ - Val: 123.4567, - PosRange: posrange.PositionRange{Start: 0, End: 8}, - }, + expected: &parser.NumberLiteral{ + Val: 123.4567, + PosRange: posrange.PositionRange{Start: 0, End: 8}, }, }, { input: `"foo"`, - expected: &parser.StepInvariantExpr{ - Expr: &parser.StringLiteral{ - Val: "foo", - PosRange: posrange.PositionRange{Start: 0, End: 5}, - }, + expected: &parser.StringLiteral{ + Val: "foo", + PosRange: posrange.PositionRange{Start: 0, End: 5}, }, }, { From 70c4f9c0c25f07eeb030d831cf9518b52e9b86fe Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sat, 19 Jul 2025 16:11:00 +0100 Subject: [PATCH 2/8] [PERF] PromQL: Don't wrap matrix selectors as time-invariant Matrix selectors have a Timestamp which indicates they are time-invariant, so we don't need to wrap and then unwrap them when we come to use them. Fix up tests that check this level of detail. Signed-off-by: Bryan Boreham --- promql/engine.go | 8 ++-- promql/engine_test.go | 94 +++++++++++++++++++++++-------------------- 2 files changed, 55 insertions(+), 47 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index da1fce8d1a..0c1d021dcf 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -1787,8 +1787,7 @@ func (ev *evaluator) eval(ctx context.Context, expr parser.Expr) (parser.Value, } unwrapParenExpr(&e.Args[matrixArgIndex]) - arg := unwrapStepInvariantExpr(e.Args[matrixArgIndex]) - unwrapParenExpr(&arg) + arg := e.Args[matrixArgIndex] sel := arg.(*parser.MatrixSelector) selVS := sel.VectorSelector.(*parser.VectorSelector) @@ -3802,7 +3801,10 @@ func preprocessExprHelper(expr parser.Expr, start, end time.Time) (isStepInvaria return false, false case *parser.MatrixSelector: - return preprocessExprHelper(n.VectorSelector, start, end) + // We don't need to wrap a MatrixSelector because functions over range vectors evaluate those directly, + // and they can't appear at top level in a range query. + isStepInvariant, _ := preprocessExprHelper(n.VectorSelector, start, end) + return isStepInvariant, false case *parser.SubqueryExpr: // Since we adjust offset for the @ modifier evaluation, diff --git a/promql/engine_test.go b/promql/engine_test.go index e98fde64e7..137bf75cdc 100644 --- a/promql/engine_test.go +++ b/promql/engine_test.go @@ -2423,23 +2423,21 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) { }, { input: `test{a="b"}[5y] @ 1603774699`, - expected: &parser.StepInvariantExpr{ - Expr: &parser.MatrixSelector{ - VectorSelector: &parser.VectorSelector{ - Name: "test", - Timestamp: makeInt64Pointer(1603774699000), - LabelMatchers: []*labels.Matcher{ - parser.MustLabelMatcher(labels.MatchEqual, "a", "b"), - parser.MustLabelMatcher(labels.MatchEqual, "__name__", "test"), - }, - PosRange: posrange.PositionRange{ - Start: 0, - End: 11, - }, + expected: &parser.MatrixSelector{ + VectorSelector: &parser.VectorSelector{ + Name: "test", + Timestamp: makeInt64Pointer(1603774699000), + LabelMatchers: []*labels.Matcher{ + parser.MustLabelMatcher(labels.MatchEqual, "a", "b"), + parser.MustLabelMatcher(labels.MatchEqual, "__name__", "test"), + }, + PosRange: posrange.PositionRange{ + Start: 0, + End: 11, }, - Range: 5 * 365 * 24 * time.Hour, - EndPos: 28, }, + Range: 5 * 365 * 24 * time.Hour, + EndPos: 28, }, }, { @@ -2938,45 +2936,53 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) { }, }, { - input: `test[5y] @ start()`, + input: `sum_over_time(test[5y] @ start())`, expected: &parser.StepInvariantExpr{ - Expr: &parser.MatrixSelector{ - VectorSelector: &parser.VectorSelector{ - Name: "test", - Timestamp: makeInt64Pointer(timestamp.FromTime(startTime)), - StartOrEnd: parser.START, - LabelMatchers: []*labels.Matcher{ - parser.MustLabelMatcher(labels.MatchEqual, "__name__", "test"), - }, - PosRange: posrange.PositionRange{ - Start: 0, - End: 4, + Expr: &parser.Call{ + Func: &parser.Function{ + Name: "sum_over_time", + ArgTypes: []parser.ValueType{parser.ValueTypeMatrix}, + ReturnType: parser.ValueTypeVector, + }, + Args: parser.Expressions{ + &parser.MatrixSelector{ + VectorSelector: &parser.VectorSelector{ + Name: "test", + Timestamp: makeInt64Pointer(timestamp.FromTime(startTime)), + StartOrEnd: parser.START, + LabelMatchers: []*labels.Matcher{ + parser.MustLabelMatcher(labels.MatchEqual, "__name__", "test"), + }, + PosRange: posrange.PositionRange{ + Start: 14, + End: 18, + }, + }, + Range: 5 * 365 * 24 * time.Hour, + EndPos: 32, }, }, - Range: 5 * 365 * 24 * time.Hour, - EndPos: 18, + PosRange: posrange.PositionRange{Start: 0, End: 32}, }, }, }, { input: `test[5y] @ end()`, - expected: &parser.StepInvariantExpr{ - Expr: &parser.MatrixSelector{ - VectorSelector: &parser.VectorSelector{ - Name: "test", - Timestamp: makeInt64Pointer(timestamp.FromTime(endTime)), - StartOrEnd: parser.END, - LabelMatchers: []*labels.Matcher{ - parser.MustLabelMatcher(labels.MatchEqual, "__name__", "test"), - }, - PosRange: posrange.PositionRange{ - Start: 0, - End: 4, - }, + expected: &parser.MatrixSelector{ + VectorSelector: &parser.VectorSelector{ + Name: "test", + Timestamp: makeInt64Pointer(timestamp.FromTime(endTime)), + StartOrEnd: parser.END, + LabelMatchers: []*labels.Matcher{ + parser.MustLabelMatcher(labels.MatchEqual, "__name__", "test"), + }, + PosRange: posrange.PositionRange{ + Start: 0, + End: 4, }, - Range: 5 * 365 * 24 * time.Hour, - EndPos: 16, }, + Range: 5 * 365 * 24 * time.Hour, + EndPos: 16, }, }, { From 92c3c8bd9c807e2ec2ab04d35bf1f73ee16d2902 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sat, 19 Jul 2025 16:13:55 +0100 Subject: [PATCH 3/8] [PERF] PromQL: Stop checking step-invariant arguments In aggregations and function calls. We no longer wrap the literal values or matrix selectors that would appear here. Signed-off-by: Bryan Boreham --- promql/engine.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index 0c1d021dcf..0e0956a941 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -1672,8 +1672,7 @@ func (ev *evaluator) eval(ctx context.Context, expr parser.Expr) (parser.Value, sortedGrouping := e.Grouping slices.Sort(sortedGrouping) - unwrapParenExpr(&e.Param) - param := unwrapStepInvariantExpr(e.Param) + param := e.Param unwrapParenExpr(¶m) if e.Op == parser.COUNT_VALUES { @@ -1730,8 +1729,7 @@ func (ev *evaluator) eval(ctx context.Context, expr parser.Expr) (parser.Value, ) for i := range e.Args { unwrapParenExpr(&e.Args[i]) - a := unwrapStepInvariantExpr(e.Args[i]) - unwrapParenExpr(&a) + a := e.Args[i] if _, ok := a.(*parser.MatrixSelector); ok { matrixArgIndex = i matrixArg = true From 5476c17f0666718714863ab9f826ea473a560b57 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sat, 19 Jul 2025 16:26:59 +0100 Subject: [PATCH 4/8] [PERF] PromQL: Unwrap superfluous parens during preprocessing This means we only do it once, rather than on every step of a range query. Also the code gets a bit shorter. Signed-off-by: Bryan Boreham --- promql/engine.go | 21 +++++++++------------ promql/functions.go | 4 +--- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index 0e0956a941..e224d5a024 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -1672,11 +1672,8 @@ func (ev *evaluator) eval(ctx context.Context, expr parser.Expr) (parser.Value, sortedGrouping := e.Grouping slices.Sort(sortedGrouping) - param := e.Param - unwrapParenExpr(¶m) - if e.Op == parser.COUNT_VALUES { - valueLabel := param.(*parser.StringLiteral) + valueLabel := e.Param.(*parser.StringLiteral) if !model.LabelName(valueLabel.Val).IsValid() { ev.errorf("invalid label name %s", valueLabel) } @@ -1691,8 +1688,8 @@ func (ev *evaluator) eval(ctx context.Context, expr parser.Expr) (parser.Value, var warnings annotations.Annotations originalNumSamples := ev.currentSamples - // param is the number k for topk/bottomk, or q for quantile. - fp, ws := newFParams(ctx, ev, param) + // e.Param is the number k for topk/bottomk, or q for quantile. + fp, ws := newFParams(ctx, ev, e.Param) warnings.Merge(ws) // Now fetch the data to be aggregated. val, ws := ev.eval(ctx, e.Expr) @@ -1712,9 +1709,7 @@ func (ev *evaluator) eval(ctx context.Context, expr parser.Expr) (parser.Value, // Matrix evaluation always returns the evaluation time, // so this function needs special handling when given // a vector selector. - unwrapParenExpr(&e.Args[0]) arg := unwrapStepInvariantExpr(e.Args[0]) - unwrapParenExpr(&arg) vs, ok := arg.(*parser.VectorSelector) if ok { return ev.rangeEvalTimestampFunctionOverVectorSelector(ctx, vs, call, e) @@ -1728,7 +1723,6 @@ func (ev *evaluator) eval(ctx context.Context, expr parser.Expr) (parser.Value, warnings annotations.Annotations ) for i := range e.Args { - unwrapParenExpr(&e.Args[i]) a := e.Args[i] if _, ok := a.(*parser.MatrixSelector); ok { matrixArgIndex = i @@ -1784,7 +1778,6 @@ func (ev *evaluator) eval(ctx context.Context, expr parser.Expr) (parser.Value, } } - unwrapParenExpr(&e.Args[matrixArgIndex]) arg := e.Args[matrixArgIndex] sel := arg.(*parser.MatrixSelector) selVS := sel.VectorSelector.(*parser.VectorSelector) @@ -3726,8 +3719,8 @@ func unwrapStepInvariantExpr(e parser.Expr) parser.Expr { } // PreprocessExpr wraps all possible step invariant parts of the given expression with -// StepInvariantExpr. It also resolves the preprocessors and evaluates duration expressions -// into their numeric values. +// StepInvariantExpr. It also resolves the preprocessors, evaluates duration expressions +// into their numeric values and removes superfluous parenthesis on parameters to functions and aggregations. func PreprocessExpr(expr parser.Expr, start, end time.Time, step time.Duration) (parser.Expr, error) { detectHistogramStatsDecoding(expr) @@ -3745,6 +3738,7 @@ func PreprocessExpr(expr parser.Expr, start, end time.Time, step time.Duration) // preprocessExprHelper wraps the child nodes of the expression // with a StepInvariantExpr wherever it's step invariant. The returned boolean is true if the // passed expression qualifies to be wrapped by StepInvariantExpr. +// Also remove superfluous parenthesis on parameters to functions and aggregations. // It also resolves the preprocessors. func preprocessExprHelper(expr parser.Expr, start, end time.Time) (isStepInvariant, shouldWrap bool) { switch n := expr.(type) { @@ -3758,6 +3752,8 @@ func preprocessExprHelper(expr parser.Expr, start, end time.Time) (isStepInvaria return n.Timestamp != nil, n.Timestamp != nil case *parser.AggregateExpr: + unwrapParenExpr(&n.Expr) + unwrapParenExpr(&n.Param) return preprocessExprHelper(n.Expr, start, end) case *parser.BinaryExpr: @@ -3781,6 +3777,7 @@ func preprocessExprHelper(expr parser.Expr, start, end time.Time) (isStepInvaria isStepInvariant := !ok shouldWrap := make([]bool, len(n.Args)) for i := range n.Args { + unwrapParenExpr(&n.Args[i]) var argIsStepInvariant bool argIsStepInvariant, shouldWrap[i] = preprocessExprHelper(n.Args[i], start, end) isStepInvariant = isStepInvariant && argIsStepInvariant diff --git a/promql/functions.go b/promql/functions.go index 2577e7f27b..ccb60b6803 100644 --- a/promql/functions.go +++ b/promql/functions.go @@ -1943,9 +1943,7 @@ func createLabelsForAbsentFunction(expr parser.Expr) labels.Labels { } func stringFromArg(e parser.Expr) string { - tmp := unwrapStepInvariantExpr(e) // Unwrap StepInvariant - unwrapParenExpr(&tmp) // Optionally unwrap ParenExpr - return tmp.(*parser.StringLiteral).Val + return e.(*parser.StringLiteral).Val } func stringSliceFromArgs(args parser.Expressions) []string { From 1a4c334d355e183984131d6b2510ee8e56225311 Mon Sep 17 00:00:00 2001 From: Andrew Hall Date: Mon, 21 Jul 2025 13:12:37 +0800 Subject: [PATCH 5/8] Proposed implementation of https://github.com/prometheus/prometheus/issues/16898. Adds supports for instant queries which return a string literal and support for a range vector result in an instant query Signed-off-by: Andrew Hall --- promql/promqltest/README.md | 32 +++++ promql/promqltest/test.go | 128 +++++++++++++++++- promql/promqltest/testdata/literals.test | 12 ++ promql/promqltest/testdata/range_queries.test | 12 ++ 4 files changed, 177 insertions(+), 7 deletions(-) diff --git a/promql/promqltest/README.md b/promql/promqltest/README.md index 84a0e69f3a..5fe2f3bf66 100644 --- a/promql/promqltest/README.md +++ b/promql/promqltest/README.md @@ -106,8 +106,40 @@ eval range from to step * `` and `` specify the time range of the range query, and use the same syntax as `