From b290e0ec17e15d1fddae8fa2a90baa6e479836cb Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sat, 19 Jul 2025 13:40:55 +0100 Subject: [PATCH 1/4] [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 | 54 +++++++++++++++++++++---------------------- promql/engine_test.go | 16 +++++-------- 2 files changed, 32 insertions(+), 38 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index 866ed279db..3f4c24c2d3 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -2108,11 +2108,6 @@ func (ev *evaluator) eval(ctx context.Context, expr parser.Expr) (parser.Value, ev.samplesStats.IncrementSamplesAtTimestamp(ev.endTimestamp, newEv.samplesStats.TotalSamples) return res, ws case *parser.StepInvariantExpr: - switch ce := e.Expr.(type) { - case *parser.StringLiteral, *parser.NumberLiteral: - return ev.eval(ctx, ce) - } - newEv := &evaluator{ startTimestamp: ev.startTimestamp, endTimestamp: ev.startTimestamp, // Always a single evaluation. @@ -3734,18 +3729,19 @@ 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 } -// 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. -// It also resolves the preprocessors. -func preprocessExprHelper(expr parser.Expr, start, end time.Time) bool { +// preprocessExprHelper wraps child nodes of expr with a StepInvariantExpr, +// at the highest level within the tree that is step-invariant. +// Also resolves start() and end() on selector and subquery nodes. +// Return isStepInvariant is true when the whole subexpression is step invariant. +// Return shoudlWrap is false for cases like MatrixSelector and StringLiteral that never need to be wrapped. +func preprocessExprHelper(expr parser.Expr, start, end time.Time) (isStepInvariant, shouldWrap bool) { switch n := expr.(type) { case *parser.VectorSelector: switch n.StartOrEnd { @@ -3754,46 +3750,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) - if isInvariant1 && isInvariant2 { - return true + isInvariantLHS, shouldWrapLHS := preprocessExprHelper(n.LHS, start, end) + isInvariantRHS, shouldWrapRHS := preprocessExprHelper(n.RHS, start, end) + if isInvariantLHS && isInvariantRHS { + return true, true } - if isInvariant1 { + if shouldWrapLHS { n.LHS = newStepInvariantExpr(n.LHS) } - if isInvariant2 { + if shouldWrapRHS { 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) @@ -3804,7 +3802,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) } @@ -3814,7 +3812,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) @@ -3823,7 +3821,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 2d724c07af..0ade9214f8 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 94d3cac4ea23a5aee5129a52ed5032c2ead29a45 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sat, 19 Jul 2025 16:11:00 +0100 Subject: [PATCH 2/4] [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 | 7 ++-- promql/engine_test.go | 94 +++++++++++++++++++++++-------------------- 2 files changed, 54 insertions(+), 47 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index 3f4c24c2d3..baa645fd9a 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -1781,8 +1781,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) @@ -3794,7 +3793,9 @@ 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. + 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 0ade9214f8..18671c76aa 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: 33}, }, }, }, { 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 384db72ede4c3269f8b73844a4455f0f15f724d7 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sat, 19 Jul 2025 16:13:55 +0100 Subject: [PATCH 3/4] [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 baa645fd9a..252d2a1fad 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -1670,8 +1670,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 { @@ -1728,8 +1727,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 7ab68550dc5991825c5242340341e98129f03291 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sat, 19 Jul 2025 16:26:59 +0100 Subject: [PATCH 4/4] [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 252d2a1fad..deb778a1c3 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -1670,11 +1670,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) } @@ -1689,8 +1686,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) @@ -1710,9 +1707,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) @@ -1726,7 +1721,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 @@ -1778,7 +1772,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) @@ -3717,8 +3710,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) @@ -3736,6 +3729,7 @@ func PreprocessExpr(expr parser.Expr, start, end time.Time, step time.Duration) // preprocessExprHelper wraps child nodes of expr with a StepInvariantExpr, // at the highest level within the tree that is step-invariant. // Also resolves start() and end() on selector and subquery nodes. +// Also remove superfluous parenthesis on parameters to functions and aggregations. // Return isStepInvariant is true when the whole subexpression is step invariant. // Return shoudlWrap is false for cases like MatrixSelector and StringLiteral that never need to be wrapped. func preprocessExprHelper(expr parser.Expr, start, end time.Time) (isStepInvariant, shouldWrap bool) { @@ -3750,6 +3744,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: @@ -3773,6 +3769,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 fe5227312f..82cfdd65b4 100644 --- a/promql/functions.go +++ b/promql/functions.go @@ -1946,9 +1946,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 {