Don't drop ParenExpr when creating StepInvariantExpr (#9591)

* Add test case to showcase the problem in #9590

Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>

* Don't unwrap ParenExpr in newStepInvariantExpr

Fixes #9590

Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>

promql/engine.go

@@ -1180,7 +1180,9 @@ func (ev *evaluator) eval(expr parser.Expr) (parser.Value, storage.Warnings) {
 		}
 
 		unwrapParenExpr(&e.Param)
-		if s, ok := unwrapStepInvariantExpr(e.Param).(*parser.StringLiteral); ok {
+		param := unwrapStepInvariantExpr(e.Param)
+		unwrapParenExpr(&param)
+		if s, ok := param.(*parser.StringLiteral); ok {
 			return ev.rangeEval(initSeries, func(v []parser.Value, sh [][]EvalSeriesHelper, enh *EvalNodeHelper) (Vector, storage.Warnings) {
 				return ev.aggregation(e.Op, sortedGrouping, e.Without, s.Val, v[0].(Vector), sh[0], enh), nil
 			}, e.Expr)
@@ -1202,6 +1204,7 @@ func (ev *evaluator) eval(expr parser.Expr) (parser.Value, storage.Warnings) {
 			// a vector selector.
 			unwrapParenExpr(&e.Args[0])
 			arg := unwrapStepInvariantExpr(e.Args[0])
+			unwrapParenExpr(&arg)
 			vs, ok := arg.(*parser.VectorSelector)
 			if ok {
 				return ev.rangeEval(nil, func(v []parser.Value, _ [][]EvalSeriesHelper, enh *EvalNodeHelper) (Vector, storage.Warnings) {
@@ -1225,6 +1228,7 @@ func (ev *evaluator) eval(expr parser.Expr) (parser.Value, storage.Warnings) {
 		for i := range e.Args {
 			unwrapParenExpr(&e.Args[i])
 			a := unwrapStepInvariantExpr(e.Args[i])
+			unwrapParenExpr(&a)
 			if _, ok := a.(*parser.MatrixSelector); ok {
 				matrixArgIndex = i
 				matrixArg = true
@@ -1267,7 +1271,10 @@ func (ev *evaluator) eval(expr parser.Expr) (parser.Value, storage.Warnings) {
 			}
 		}
 
-		sel := unwrapStepInvariantExpr(e.Args[matrixArgIndex]).(*parser.MatrixSelector)
+		unwrapParenExpr(&e.Args[matrixArgIndex])
+		arg := unwrapStepInvariantExpr(e.Args[matrixArgIndex])
+		unwrapParenExpr(&arg)
+		sel := arg.(*parser.MatrixSelector)
 		selVS := sel.VectorSelector.(*parser.VectorSelector)
 
 		ws, err := checkAndExpandSeriesSet(ev.ctx, sel)
@@ -2558,11 +2565,6 @@ func preprocessExprHelper(expr parser.Expr, start, end time.Time) bool {
 }
 
 func newStepInvariantExpr(expr parser.Expr) parser.Expr {
-	if e, ok := expr.(*parser.ParenExpr); ok {
-		// Wrapping the inside of () makes it easy to unwrap the paren later.
-		// But this effectively unwraps the paren.
-		return newStepInvariantExpr(e.Expr)
-	}
 	return &parser.StepInvariantExpr{Expr: expr}
 }
 

promql/engine_test.go

@@ -1593,6 +1593,7 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 	testCases := []struct {
 		input      string      // The input to be parsed.
 		expected   parser.Expr // The expected expression AST.
+		outputTest bool
 	}{
 		{
 			input: "123.4567",
@@ -1602,7 +1603,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					PosRange: parser.PositionRange{Start: 0, End: 8},
 				},
 			},
-		}, {
+		},
+		{
 			input: `"foo"`,
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.StringLiteral{
@@ -1610,7 +1612,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					PosRange: parser.PositionRange{Start: 0, End: 5},
 				},
 			},
-		}, {
+		},
+		{
 			input: "foo * bar",
 			expected: &parser.BinaryExpr{
 				Op: parser.MUL,
@@ -1636,7 +1639,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 				},
 				VectorMatching: &parser.VectorMatching{Card: parser.CardOneToOne},
 			},
-		}, {
+		},
+		{
 			input: "foo * bar @ 10",
 			expected: &parser.BinaryExpr{
 				Op: parser.MUL,
@@ -1665,7 +1669,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 				},
 				VectorMatching: &parser.VectorMatching{Card: parser.CardOneToOne},
 			},
-		}, {
+		},
+		{
 			input: "foo @ 20 * bar @ 10",
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.BinaryExpr{
@@ -1695,7 +1700,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					VectorMatching: &parser.VectorMatching{Card: parser.CardOneToOne},
 				},
 			},
-		}, {
+		},
+		{
 			input: "test[5s]",
 			expected: &parser.MatrixSelector{
 				VectorSelector: &parser.VectorSelector{
@@ -1711,7 +1717,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 				Range:  5 * time.Second,
 				EndPos: 8,
 			},
-		}, {
+		},
+		{
 			input: `test{a="b"}[5y] @ 1603774699`,
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.MatrixSelector{
@@ -1731,7 +1738,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					EndPos: 28,
 				},
 			},
-		}, {
+		},
+		{
 			input: "sum by (foo)(some_metric)",
 			expected: &parser.AggregateExpr{
 				Op: parser.SUM,
@@ -1751,7 +1759,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					End:   25,
 				},
 			},
-		}, {
+		},
+		{
 			input: "sum by (foo)(some_metric @ 10)",
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.AggregateExpr{
@@ -1774,7 +1783,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					},
 				},
 			},
-		}, {
+		},
+		{
 			input: "sum(some_metric1 @ 10) + sum(some_metric2 @ 20)",
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.BinaryExpr{
@@ -1818,7 +1828,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					},
 				},
 			},
-		}, {
+		},
+		{
 			input: "some_metric and topk(5, rate(some_metric[1m] @ 20))",
 			expected: &parser.BinaryExpr{
 				Op: parser.LAND,
@@ -1876,7 +1887,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					},
 				},
 			},
-		}, {
+		},
+		{
 			input: "time()",
 			expected: &parser.Call{
 				Func: parser.MustGetFunction("time"),
@@ -1886,7 +1898,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					End:   6,
 				},
 			},
-		}, {
+		},
+		{
 			input: `foo{bar="baz"}[10m:6s]`,
 			expected: &parser.SubqueryExpr{
 				Expr: &parser.VectorSelector{
@@ -1904,7 +1917,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 				Step:   6 * time.Second,
 				EndPos: 22,
 			},
-		}, {
+		},
+		{
 			input: `foo{bar="baz"}[10m:6s] @ 10`,
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.SubqueryExpr{
@@ -1925,7 +1939,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					EndPos:    27,
 				},
 			},
-		}, { // Even though the subquery is step invariant, the inside is also wrapped separately.
+		},
+		{ // Even though the subquery is step invariant, the inside is also wrapped separately.
 			input: `sum(foo{bar="baz"} @ 20)[10m:6s] @ 10`,
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.SubqueryExpr{
@@ -1956,7 +1971,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					EndPos:    37,
 				},
 			},
-		}, {
+		},
+		{
 			input: `min_over_time(rate(foo{bar="baz"}[2s])[5m:] @ 1603775091)[4m:3s]`,
 			expected: &parser.SubqueryExpr{
 				Expr: &parser.StepInvariantExpr{
@@ -2003,7 +2019,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 				Step:   3 * time.Second,
 				EndPos: 64,
 			},
-		}, {
+		},
+		{
 			input: `some_metric @ 123 offset 1m [10m:5s]`,
 			expected: &parser.SubqueryExpr{
 				Expr: &parser.StepInvariantExpr{
@@ -2024,7 +2041,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 				Step:   5 * time.Second,
 				EndPos: 36,
 			},
-		}, {
+		},
+		{
 			input: `some_metric[10m:5s] offset 1m @ 123`,
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.SubqueryExpr{
@@ -2045,7 +2063,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					EndPos:         35,
 				},
 			},
-		}, {
+		},
+		{
 			input: `(foo + bar{nm="val"} @ 1234)[5m:] @ 1603775019`,
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.SubqueryExpr{
@@ -2090,7 +2109,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					EndPos:    46,
 				},
 			},
-		}, {
+		},
+		{
 			input: "abs(abs(metric @ 10))",
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.Call{
@@ -2127,7 +2147,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					},
 				},
 			},
-		}, {
+		},
+		{
 			input: "sum(sum(some_metric1 @ 10) + sum(some_metric2 @ 20))",
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.AggregateExpr{
@@ -2178,7 +2199,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					},
 				},
 			},
-		}, {
+		},
+		{
 			input: `foo @ start()`,
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.VectorSelector{
@@ -2194,7 +2216,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					StartOrEnd: parser.START,
 				},
 			},
-		}, {
+		},
+		{
 			input: `foo @ end()`,
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.VectorSelector{
@@ -2210,7 +2233,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					StartOrEnd: parser.END,
 				},
 			},
-		}, {
+		},
+		{
 			input: `test[5y] @ start()`,
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.MatrixSelector{
@@ -2230,7 +2254,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					EndPos: 18,
 				},
 			},
-		}, {
+		},
+		{
 			input: `test[5y] @ end()`,
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.MatrixSelector{
@@ -2250,7 +2275,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					EndPos: 16,
 				},
 			},
-		}, {
+		},
+		{
 			input: `some_metric[10m:5s] @ start()`,
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.SubqueryExpr{
@@ -2271,7 +2297,8 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 					EndPos:     29,
 				},
 			},
-		}, {
+		},
+		{
 			input: `some_metric[10m:5s] @ end()`,
 			expected: &parser.StepInvariantExpr{
 				Expr: &parser.SubqueryExpr{
@@ -2293,6 +2320,61 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 				},
 			},
 		},
+		{
+			input:      `floor(some_metric / (3 * 1024))`,
+			outputTest: true,
+			expected: &parser.Call{
+				Func: &parser.Function{
+					Name:       "floor",
+					ArgTypes:   []parser.ValueType{parser.ValueTypeVector},
+					ReturnType: parser.ValueTypeVector,
+				},
+				Args: parser.Expressions{
+					&parser.BinaryExpr{
+						Op: parser.DIV,
+						LHS: &parser.VectorSelector{
+							Name: "some_metric",
+							LabelMatchers: []*labels.Matcher{
+								parser.MustLabelMatcher(labels.MatchEqual, "__name__", "some_metric"),
+							},
+							PosRange: parser.PositionRange{
+								Start: 6,
+								End:   17,
+							},
+						},
+						RHS: &parser.StepInvariantExpr{
+							Expr: &parser.ParenExpr{
+								Expr: &parser.BinaryExpr{
+									Op: parser.MUL,
+									LHS: &parser.NumberLiteral{
+										Val: 3,
+										PosRange: parser.PositionRange{
+											Start: 21,
+											End:   22,
+										},
+									},
+									RHS: &parser.NumberLiteral{
+										Val: 1024,
+										PosRange: parser.PositionRange{
+											Start: 25,
+											End:   29,
+										},
+									},
+								},
+								PosRange: parser.PositionRange{
+									Start: 20,
+									End:   30,
+								},
+							},
+						},
+					},
+				},
+				PosRange: parser.PositionRange{
+					Start: 0,
+					End:   31,
+				},
+			},
+		},
 	}
 
 	for _, test := range testCases {
@@ -2300,6 +2382,9 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 			expr, err := parser.ParseExpr(test.input)
 			require.NoError(t, err)
 			expr = PreprocessExpr(expr, startTime, endTime)
+			if test.outputTest {
+				require.Equal(t, test.input, expr.String(), "error on input '%s'", test.input)
+			}
 			require.Equal(t, test.expected, expr, "error on input '%s'", test.input)
 		})
 	}

promql/functions.go

@@ -1227,5 +1227,7 @@ func createLabelsForAbsentFunction(expr parser.Expr) labels.Labels {
 }
 
 func stringFromArg(e parser.Expr) string {
-	return unwrapStepInvariantExpr(e).(*parser.StringLiteral).Val
+	tmp := unwrapStepInvariantExpr(e) // Unwrap StepInvariant
+	unwrapParenExpr(&tmp)             // Optionally unwrap ParenExpr
+	return tmp.(*parser.StringLiteral).Val
 }