From 1e0cd28bf6d1168f3636e7a9e113f26f2496e02b Mon Sep 17 00:00:00 2001
From: Tobias Guggenmos <tguggenm@redhat.com>
Date: Fri, 20 Dec 2019 12:28:56 +0100
Subject: [PATCH] PromQL: Support trailing commas in grouping opts (#6480)

* PromQL: Support trailing commas in grouping opts

Fixes #6470

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>
---
 docs/querying/basics.md      |   4 +-
 docs/querying/operators.md   |   9 ++-
 promql/generated_parser.y    |   2 +
 promql/generated_parser.y.go | 107 +++++++++++++++++++----------------
 promql/parse_test.go         |  32 +++++++++++
 5 files changed, 101 insertions(+), 53 deletions(-)

diff --git a/docs/querying/basics.md b/docs/querying/basics.md
index 4021a6580b..0b1bc09552 100644
--- a/docs/querying/basics.md
+++ b/docs/querying/basics.md
@@ -74,8 +74,8 @@ name:
 
     http_requests_total
 
-It is possible to filter these time series further by appending a set of labels
-to match in curly braces (`{}`).
+It is possible to filter these time series further by appending a comma separated list of label
+matchers in curly braces (`{}`).
 
 This example selects only those time series with the `http_requests_total`
 metric name that also have the `job` label set to `prometheus` and their
diff --git a/docs/querying/operators.md b/docs/querying/operators.md
index 99143ec8a1..757037276e 100644
--- a/docs/querying/operators.md
+++ b/docs/querying/operators.md
@@ -205,12 +205,17 @@ or
 
     <aggr-op>([parameter,] <vector expression>) [without|by (<label list>)]
 
-`parameter` is only required for `count_values`, `quantile`, `topk` and
-`bottomk`. `without` removes the listed labels from the result vector, while
+`label list` is a list of unquoted labels that may include a trailing comma, i.e.
+both `(label1, label2)` and `(label1, label2,)` are valid syntax.
+
+`without` removes the listed labels from the result vector, while
 all other labels are preserved the output. `by` does the opposite and drops
 labels that are not listed in the `by` clause, even if their label values are
 identical between all elements of the vector.
 
+`parameter` is only required for `count_values`, `quantile`, `topk` and
+`bottomk`.
+
 `count_values` outputs one time series per unique sample value. Each series has
 an additional label. The name of that label is given by the aggregation
 parameter, and the label value is the unique sample value.  The value of each
diff --git a/promql/generated_parser.y b/promql/generated_parser.y
index 8a18ac4885..306056a546 100644
--- a/promql/generated_parser.y
+++ b/promql/generated_parser.y
@@ -225,6 +225,8 @@ label_set_item  :
 grouping_labels :
                 LEFT_PAREN grouping_label_list RIGHT_PAREN
                         { $$ = $2 }
+                | LEFT_PAREN grouping_label_list COMMA RIGHT_PAREN
+                        { $$ = $2 }
                 | LEFT_PAREN RIGHT_PAREN
                         { $$ = []string{} }
                 | error
diff --git a/promql/generated_parser.y.go b/promql/generated_parser.y.go
index 02c36a7ee3..59f0bbb6da 100644
--- a/promql/generated_parser.y.go
+++ b/promql/generated_parser.y.go
@@ -175,7 +175,7 @@ const yyEofCode = 1
 const yyErrCode = 2
 const yyInitialStackSize = 16
 
-//line promql/generated_parser.y:284
+//line promql/generated_parser.y:286
 
 //line yacctab:1
 var yyExca = [...]int{
@@ -189,59 +189,62 @@ var yyExca = [...]int{
 
 const yyPrivate = 57344
 
-const yyLast = 117
+const yyLast = 142
 
 var yyAct = [...]int{
 
-	35, 6, 33, 27, 72, 36, 37, 83, 32, 79,
-	22, 29, 75, 24, 10, 8, 28, 29, 23, 1,
-	73, 80, 28, 76, 82, 74, 78, 26, 24, 38,
-	39, 40, 19, 23, 31, 35, 71, 17, 21, 18,
-	36, 37, 11, 41, 42, 43, 44, 45, 46, 47,
-	48, 49, 50, 51, 25, 34, 52, 53, 12, 54,
-	55, 56, 57, 58, 38, 39, 40, 2, 3, 4,
-	5, 77, 63, 81, 62, 20, 7, 84, 41, 42,
-	43, 44, 45, 46, 47, 48, 49, 50, 51, 70,
-	61, 52, 53, 0, 54, 55, 56, 57, 58, 68,
-	59, 9, 69, 60, 64, 65, 13, 14, 0, 0,
-	66, 67, 16, 15, 30, 0, 10,
+	35, 27, 33, 63, 22, 36, 37, 6, 84, 83,
+	79, 75, 1, 29, 10, 70, 31, 8, 28, 73,
+	17, 11, 72, 80, 74, 68, 82, 78, 69, 38,
+	39, 40, 19, 25, 34, 64, 65, 12, 62, 18,
+	20, 66, 67, 41, 42, 43, 44, 45, 46, 47,
+	48, 49, 50, 51, 71, 7, 52, 53, 0, 54,
+	55, 56, 57, 58, 35, 77, 0, 0, 61, 36,
+	37, 81, 32, 2, 3, 4, 5, 85, 59, 24,
+	29, 60, 24, 0, 23, 28, 0, 23, 0, 76,
+	26, 14, 21, 38, 39, 40, 16, 15, 0, 0,
+	10, 0, 0, 0, 0, 0, 0, 41, 42, 43,
+	44, 45, 46, 47, 48, 49, 50, 51, 0, 0,
+	52, 53, 0, 54, 55, 56, 57, 58, 9, 0,
+	0, 0, 0, 13, 0, 0, 0, 0, 0, 0,
+	0, 30,
 }
 var yyPact = [...]int{
 
-	-1, -1000, 4, 3, 105, 30, -1000, -1000, 26, -1000,
-	15, -1000, 3, -1000, -1000, -1000, -1000, -1000, -2, -1000,
-	88, -1000, -1000, 70, -1000, 87, -1000, -1000, 2, -1000,
-	-1000, 10, -1000, -1000, -1000, -1000, -1000, -1000, -1000, -1000,
+	5, -1000, 6, 3, 89, 30, -1000, -1000, 80, -1000,
+	78, -1000, 3, -1000, -1000, -1000, -1000, -1000, 62, -1000,
+	66, -1000, -1000, 1, -1000, 13, -1000, -1000, 20, -1000,
+	-1000, 9, -1000, -1000, -1000, -1000, -1000, -1000, -1000, -1000,
 	-1000, -1000, -1000, -1000, -1000, -1000, -1000, -1000, -1000, -1000,
 	-1000, -1000, -1000, -1000, -1000, -1000, -1000, -1000, -1000, -1000,
-	11, -1000, 7, -1000, -1000, -1000, -1000, -1000, -1000, 9,
-	-1000, 5, -1000, -1000, 33, -1000, -1000, -1000, -1000, -1000,
-	-1000, -1000, -1000, -1000, -1000,
+	77, -1000, 8, -1000, -1000, -1000, -1000, -1000, -1000, 11,
+	-1000, 7, -1000, -1000, -2, -1000, -1000, -1000, -1000, -1000,
+	-1000, -1000, -1000, -1000, -1000, -1000,
 }
 var yyPgo = [...]int{
 
-	0, 76, 75, 10, 74, 58, 2, 55, 54, 101,
-	42, 3, 37, 34, 19,
+	0, 55, 40, 4, 38, 37, 2, 34, 33, 128,
+	21, 1, 20, 16, 12,
 }
 var yyR1 = [...]int{
 
 	0, 14, 14, 14, 14, 14, 1, 1, 1, 2,
 	2, 2, 3, 3, 3, 3, 4, 4, 4, 4,
 	10, 10, 10, 5, 5, 9, 9, 9, 9, 8,
-	8, 8, 11, 11, 11, 11, 12, 12, 12, 13,
-	13, 13, 6, 6, 7, 7, 7, 7, 7, 7,
+	8, 8, 11, 11, 11, 11, 12, 12, 12, 12,
+	13, 13, 13, 6, 6, 7, 7, 7, 7, 7,
 	7, 7, 7, 7, 7, 7, 7, 7, 7, 7,
-	7, 7, 7, 7, 7, 7, 7,
+	7, 7, 7, 7, 7, 7, 7, 7,
 }
 var yyR2 = [...]int{
 
 	0, 2, 2, 2, 2, 1, 3, 4, 2, 3,
 	1, 2, 3, 3, 2, 1, 1, 1, 1, 1,
 	2, 1, 1, 1, 1, 3, 4, 2, 0, 3,
-	1, 2, 3, 3, 2, 1, 3, 2, 1, 3,
-	1, 2, 1, 1, 1, 1, 1, 1, 1, 1,
+	1, 2, 3, 3, 2, 1, 3, 4, 2, 1,
+	3, 1, 2, 1, 1, 1, 1, 1, 1, 1,
 	1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
-	1, 1, 1, 1, 1, 1, 1,
+	1, 1, 1, 1, 1, 1, 1, 1,
 }
 var yyChk = [...]int{
 
@@ -253,19 +256,19 @@ var yyChk = [...]int{
 	54, 55, 58, 59, 61, 62, 63, 64, 65, 12,
 	15, 2, -4, 2, 34, 35, 40, 41, 12, 15,
 	2, 34, 2, 10, 15, 2, 12, -3, 19, 2,
-	12, -11, 19, 2, -6,
+	12, -11, 19, 2, 10, -6,
 }
 var yyDef = [...]int{
 
 	0, -2, 0, 28, -2, 0, 5, 1, 0, 2,
-	0, 3, 28, 21, 22, 23, 24, 4, 0, 38,
+	0, 3, 28, 21, 22, 23, 24, 4, 0, 39,
 	0, 8, 10, 0, 15, 0, 27, 30, 0, 35,
-	20, 0, 37, 40, 42, 43, 44, 45, 46, 47,
-	48, 49, 50, 51, 52, 53, 54, 55, 56, 57,
-	58, 59, 60, 61, 62, 63, 64, 65, 66, 6,
+	20, 0, 38, 41, 43, 44, 45, 46, 47, 48,
+	49, 50, 51, 52, 53, 54, 55, 56, 57, 58,
+	59, 60, 61, 62, 63, 64, 65, 66, 67, 6,
 	0, 11, 0, 14, 16, 17, 18, 19, 25, 0,
-	31, 0, 34, 36, 0, 41, 7, 9, 12, 13,
-	26, 29, 32, 33, 39,
+	31, 0, 34, 36, 0, 42, 7, 9, 12, 13,
+	26, 29, 32, 33, 37, 40,
 }
 var yyTok1 = [...]int{
 
@@ -841,47 +844,53 @@ yydefault:
 			yyVAL.strings = yyDollar[2].strings
 		}
 	case 37:
-		yyDollar = yyS[yypt-2 : yypt+1]
+		yyDollar = yyS[yypt-4 : yypt+1]
 //line promql/generated_parser.y:229
+		{
+			yyVAL.strings = yyDollar[2].strings
+		}
+	case 38:
+		yyDollar = yyS[yypt-2 : yypt+1]
+//line promql/generated_parser.y:231
 		{
 			yyVAL.strings = []string{}
 		}
-	case 38:
+	case 39:
 		yyDollar = yyS[yypt-1 : yypt+1]
-//line promql/generated_parser.y:231
+//line promql/generated_parser.y:233
 		{
 			yylex.(*parser).unexpected("grouping opts", "\"(\"")
 		}
-	case 39:
+	case 40:
 		yyDollar = yyS[yypt-3 : yypt+1]
-//line promql/generated_parser.y:237
+//line promql/generated_parser.y:239
 		{
 			yyVAL.strings = append(yyDollar[1].strings, yyDollar[3].item.Val)
 		}
-	case 40:
+	case 41:
 		yyDollar = yyS[yypt-1 : yypt+1]
-//line promql/generated_parser.y:239
+//line promql/generated_parser.y:241
 		{
 			yyVAL.strings = []string{yyDollar[1].item.Val}
 		}
-	case 41:
+	case 42:
 		yyDollar = yyS[yypt-2 : yypt+1]
-//line promql/generated_parser.y:241
+//line promql/generated_parser.y:243
 		{
 			yylex.(*parser).unexpected("grouping opts", "\",\" or \"}\"")
 		}
-	case 42:
+	case 43:
 		yyDollar = yyS[yypt-1 : yypt+1]
-//line promql/generated_parser.y:246
+//line promql/generated_parser.y:248
 		{
 			if !isLabel(yyDollar[1].item.Val) {
 				yylex.(*parser).unexpected("grouping opts", "label")
 			}
 			yyVAL.item = yyDollar[1].item
 		}
-	case 43:
+	case 44:
 		yyDollar = yyS[yypt-1 : yypt+1]
-//line promql/generated_parser.y:253
+//line promql/generated_parser.y:255
 		{
 			yylex.(*parser).unexpected("grouping opts", "label")
 		}
diff --git a/promql/parse_test.go b/promql/parse_test.go
index 5d021bc00e..c2c6ae2223 100644
--- a/promql/parse_test.go
+++ b/promql/parse_test.go
@@ -1190,6 +1190,30 @@ var testExpr = []struct {
 			},
 			Grouping: []string{},
 		},
+	}, {
+		input: "sum by (foo,bar,)(some_metric)",
+		expected: &AggregateExpr{
+			Op: SUM,
+			Expr: &VectorSelector{
+				Name: "some_metric",
+				LabelMatchers: []*labels.Matcher{
+					mustLabelMatcher(labels.MatchEqual, string(model.MetricNameLabel), "some_metric"),
+				},
+			},
+			Grouping: []string{"foo", "bar"},
+		},
+	}, {
+		input: "sum by (foo,)(some_metric)",
+		expected: &AggregateExpr{
+			Op: SUM,
+			Expr: &VectorSelector{
+				Name: "some_metric",
+				LabelMatchers: []*labels.Matcher{
+					mustLabelMatcher(labels.MatchEqual, string(model.MetricNameLabel), "some_metric"),
+				},
+			},
+			Grouping: []string{"foo"},
+		},
 	}, {
 		input: "topk(5, some_metric)",
 		expected: &AggregateExpr{
@@ -1232,6 +1256,14 @@ var testExpr = []struct {
 		input:  "sum without(==)(some_metric)",
 		fail:   true,
 		errMsg: "unexpected <op:==> in grouping opts, expected label",
+	}, {
+		input:  "sum without(,)(some_metric)",
+		fail:   true,
+		errMsg: `unexpected "," in grouping opts, expected label`,
+	}, {
+		input:  "sum without(foo,,)(some_metric)",
+		fail:   true,
+		errMsg: `unexpected "," in grouping opts, expected label`,
 	}, {
 		input:  `sum some_metric by (test)`,
 		fail:   true,