Merge pull request #16041 from prymitive/parenExprEnd

Add a test for aggregation wrapped in ParenExpr

promql/engine_test.go

@@ -2895,7 +2895,7 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 							},
 							PosRange: posrange.PositionRange{
 								Start: 29,
-								End:   52,
+								End:   51,
 							},
 						},
 					},

promql/parser/generated_parser.y

@@ -243,9 +243,23 @@ expr            :
  */
 
 aggregate_expr  : aggregate_op aggregate_modifier function_call_body
-                        { $$ = yylex.(*parser).newAggregateExpr($1, $2, $3) }
+                        {
+                        // Need to consume the position of the first RIGHT_PAREN. It might not exist on garbage input
+                        // like 'sum (some_metric) by test'
+                        if len(yylex.(*parser).closingParens) > 1 {
+                                yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
+                        }
+                        $$ = yylex.(*parser).newAggregateExpr($1, $2, $3)
+                        }
                 | aggregate_op function_call_body aggregate_modifier
-                        { $$ = yylex.(*parser).newAggregateExpr($1, $3, $2) }
+                        {
+                        // Need to consume the position of the first RIGHT_PAREN. It might not exist on garbage input
+                        // like 'sum by test (some_metric)'
+                        if len(yylex.(*parser).closingParens) > 1 {
+                                yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
+                        }
+                        $$ = yylex.(*parser).newAggregateExpr($1, $3, $2)
+                        }
                 | aggregate_op function_call_body
                         { $$ = yylex.(*parser).newAggregateExpr($1, &AggregateExpr{}, $2) }
                 | aggregate_op error
@@ -399,9 +413,10 @@ function_call   : IDENTIFIER function_call_body
                                 Args: $2.(Expressions),
                                 PosRange: posrange.PositionRange{
                                         Start: $1.Pos,
-                                        End:   yylex.(*parser).lastClosing,
+                                        End:   yylex.(*parser).closingParens[0],
                                 },
                         }
+                        yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
                         }
                 ;
 
@@ -427,7 +442,10 @@ function_call_args: function_call_args COMMA expr
  */
 
 paren_expr      : LEFT_PAREN expr RIGHT_PAREN
-                        { $$ = &ParenExpr{Expr: $2.(Expr), PosRange: mergeRanges(&$1, &$3)} }
+                        {
+                        $$ = &ParenExpr{Expr: $2.(Expr), PosRange: mergeRanges(&$1, &$3)}
+                        yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
+                        }
                 ;
 
 /*

promql/parser/generated_parser.y.go

@@ -1087,11 +1087,21 @@ yydefault:
 	case 21:
 		yyDollar = yyS[yypt-3 : yypt+1]
 		{
+			// Need to consume the position of the first RIGHT_PAREN. It might not exist on garbage input
+			// like 'sum (some_metric) by test'
+			if len(yylex.(*parser).closingParens) > 1 {
+				yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
+			}
 			yyVAL.node = yylex.(*parser).newAggregateExpr(yyDollar[1].item, yyDollar[2].node, yyDollar[3].node)
 		}
 	case 22:
 		yyDollar = yyS[yypt-3 : yypt+1]
 		{
+			// Need to consume the position of the first RIGHT_PAREN. It might not exist on garbage input
+			// like 'sum by test (some_metric)'
+			if len(yylex.(*parser).closingParens) > 1 {
+				yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
+			}
 			yyVAL.node = yylex.(*parser).newAggregateExpr(yyDollar[1].item, yyDollar[3].node, yyDollar[2].node)
 		}
 	case 23:
@@ -1319,9 +1329,10 @@ yydefault:
 				Args: yyDollar[2].node.(Expressions),
 				PosRange: posrange.PositionRange{
 					Start: yyDollar[1].item.Pos,
-					End:   yylex.(*parser).lastClosing,
+					End:   yylex.(*parser).closingParens[0],
 				},
 			}
+			yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
 		}
 	case 63:
 		yyDollar = yyS[yypt-3 : yypt+1]
@@ -1353,6 +1364,7 @@ yydefault:
 		yyDollar = yyS[yypt-3 : yypt+1]
 		{
 			yyVAL.node = &ParenExpr{Expr: yyDollar[2].node.(Expr), PosRange: mergeRanges(&yyDollar[1].item, &yyDollar[3].item)}
+			yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
 		}
 	case 69:
 		yyDollar = yyS[yypt-1 : yypt+1]

promql/parser/parse.go

@@ -59,6 +59,13 @@ type parser struct {
 	// Everytime an Item is lexed that could be the end
 	// of certain expressions its end position is stored here.
 	lastClosing posrange.Pos
+	// Keep track of closing parentheses in addition, because sometimes the
+	// parser needs to read past a closing parenthesis to find the end of an
+	// expression, e.g. reading ony '(sum(foo)' cannot tell the end of the
+	// aggregation expression, since it could continue with either
+	// '(sum(foo))' or '(sum(foo) by (bar))' by which time we set lastClosing
+	// to the last paren.
+	closingParens []posrange.Pos
 
 	yyParser yyParserImpl
 
@@ -82,6 +89,7 @@ func NewParser(input string, opts ...Opt) *parser { //nolint:revive // unexporte
 	p.injecting = false
 	p.parseErrors = nil
 	p.generatedParserResult = nil
+	p.closingParens = make([]posrange.Pos, 0)
 
 	// Clear lexer struct before reusing.
 	p.lex = Lexer{
@@ -171,6 +179,11 @@ func EnrichParseError(err error, enrich func(parseErr *ParseErr)) {
 func ParseExpr(input string) (expr Expr, err error) {
 	p := NewParser(input)
 	defer p.Close()
+
+	if len(p.closingParens) > 0 {
+		return nil, fmt.Errorf("internal parser error, not all closing parens consumed: %v", p.closingParens)
+	}
+
 	return p.ParseExpr()
 }
 
@@ -374,7 +387,10 @@ func (p *parser) Lex(lval *yySymType) int {
 	case EOF:
 		lval.item.Typ = EOF
 		p.InjectItem(0)
-	case RIGHT_BRACE, RIGHT_PAREN, RIGHT_BRACKET, DURATION, NUMBER:
+	case RIGHT_PAREN:
+		p.closingParens = append(p.closingParens, lval.item.Pos+posrange.Pos(len(lval.item.Val)))
+		fallthrough
+	case RIGHT_BRACE, RIGHT_BRACKET, DURATION, NUMBER:
 		p.lastClosing = lval.item.Pos + posrange.Pos(len(lval.item.Val))
 	}
 
@@ -437,8 +453,9 @@ func (p *parser) newAggregateExpr(op Item, modifier, args Node) (ret *AggregateE
 
 	ret.PosRange = posrange.PositionRange{
 		Start: op.Pos,
-		End:   p.lastClosing,
+		End:   p.closingParens[0],
 	}
+	p.closingParens = p.closingParens[1:]
 
 	ret.Op = op.Typ
 

promql/parser/parse_test.go

@@ -3997,6 +3997,7 @@ var testExpr = []struct {
 			},
 		},
 	},
+	// Test that nested parentheses result in the correct position range.
 	{
 		input: `foo[11s+10s-5*2^2]`,
 		expected: &MatrixSelector{
@@ -4295,6 +4296,68 @@ var testExpr = []struct {
 		fail:   true,
 		errMsg: `duration out of range`,
 	},
+	{
+		input: "(sum(foo))",
+		expected: &ParenExpr{
+			Expr: &AggregateExpr{
+				Op: SUM,
+				Expr: &VectorSelector{
+					Name: "foo",
+					LabelMatchers: []*labels.Matcher{
+						MustLabelMatcher(labels.MatchEqual, model.MetricNameLabel, "foo"),
+					},
+					PosRange: posrange.PositionRange{
+						Start: 5,
+						End:   8,
+					},
+				},
+				PosRange: posrange.PositionRange{Start: 1, End: 9},
+			},
+			PosRange: posrange.PositionRange{Start: 0, End: 10},
+		},
+	},
+	{
+		input: "(sum(foo) by (bar))",
+		expected: &ParenExpr{
+			Expr: &AggregateExpr{
+				Op:       SUM,
+				Grouping: []string{"bar"},
+				Expr: &VectorSelector{
+					Name: "foo",
+					LabelMatchers: []*labels.Matcher{
+						MustLabelMatcher(labels.MatchEqual, model.MetricNameLabel, "foo"),
+					},
+					PosRange: posrange.PositionRange{
+						Start: 5,
+						End:   8,
+					},
+				},
+				PosRange: posrange.PositionRange{Start: 1, End: 18},
+			},
+			PosRange: posrange.PositionRange{Start: 0, End: 19},
+		},
+	},
+	{
+		input: "(sum by (bar) (foo))",
+		expected: &ParenExpr{
+			Expr: &AggregateExpr{
+				Op:       SUM,
+				Grouping: []string{"bar"},
+				Expr: &VectorSelector{
+					Name: "foo",
+					LabelMatchers: []*labels.Matcher{
+						MustLabelMatcher(labels.MatchEqual, model.MetricNameLabel, "foo"),
+					},
+					PosRange: posrange.PositionRange{
+						Start: 15,
+						End:   18,
+					},
+				},
+				PosRange: posrange.PositionRange{Start: 1, End: 19},
+			},
+			PosRange: posrange.PositionRange{Start: 0, End: 20},
+		},
+	},
 }
 
 func makeInt64Pointer(val int64) *int64 {