diff --git a/promql/engine.go b/promql/engine.go index 3cdf299dff..6e3deb2cb8 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -1670,12 +1670,8 @@ 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) - 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) } @@ -1690,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) @@ -1711,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) @@ -1727,9 +1721,7 @@ 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 := unwrapStepInvariantExpr(e.Args[i]) - unwrapParenExpr(&a) + a := e.Args[i] if _, ok := a.(*parser.MatrixSelector); ok { matrixArgIndex = i matrixArg = true @@ -1780,9 +1772,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) @@ -3725,8 +3715,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) @@ -3734,8 +3724,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 @@ -3744,8 +3734,9 @@ 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) bool { +func preprocessExprHelper(expr parser.Expr, start, end time.Time) (isStepInvariant, shouldWrap bool) { switch n := expr.(type) { case *parser.VectorSelector: switch n.StartOrEnd { @@ -3754,49 +3745,57 @@ 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: + unwrapParenExpr(&n.Expr) + unwrapParenExpr(&n.Param) 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] + unwrapParenExpr(&n.Args[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) + // 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, @@ -3804,7 +3803,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 +3813,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 +3822,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 111ffc3a4b..bfe51236b8 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}, }, }, { @@ -2427,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, }, }, { @@ -2942,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, }, }, { diff --git a/promql/functions.go b/promql/functions.go index 6b038fe336..42adcdbeae 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 { 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 `