From b290e0ec17e15d1fddae8fa2a90baa6e479836cb Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sat, 19 Jul 2025 13:40:55 +0100 Subject: [PATCH] [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}, }, }, {