fix panics when cloning a special query (#8531)

Signed-off-by: garrettlish <garrett.li.sh@gmail.com>
grafana · Feb 15, 2023 · 433d5bf · 433d5bf
1 parent 099ca23
commit 433d5bf
Show file tree

Hide file tree

Showing 6 changed files with 29 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -42,6 +42,7 @@
 * [8251](https://github.com/grafana/loki/pull/8251) **sandeepsukhani** index-store: fix indexing of chunks overlapping multiple schemas.
 * [8151](https://github.com/grafana/loki/pull/8151) **sandeepsukhani** fix log deletion with line filters.
 * [8448](https://github.com/grafana/loki/pull/8448) **chaudum**: Fix bug in LogQL parser that caused certain queries that contain a vector expression to fail.
+* [8531](https://github.com/grafana/loki/pull/8531) **garrettlish**: logql: fix panics when cloning a special query
 
 ##### Changes
 

diff --git a/pkg/logql/optimize_test.go b/pkg/logql/optimize_test.go
@@ -15,18 +15,18 @@ func Test_optimizeSampleExpr(t *testing.T) {
 		// noop
 		{`1`, `1`},
 		{`1 + 1`, `2`},
-		{`topk(10,sum by(name)(rate({region="us-east1"}[5m])))`, `topk(10,sum by(name)(rate({region="us-east1"}[5m])))`},
-		{`sum by(name)(rate({region="us-east1"}[5m]))`, `sum by(name)(rate({region="us-east1"}[5m]))`},
-		{`sum by(name)(bytes_over_time({region="us-east1"} | line_format "something else"[5m]))`, `sum by(name)(bytes_over_time({region="us-east1"} | line_format "something else"[5m]))`},
-		{`sum by(name)(rate({region="us-east1"} | json | line_format "something else" |= "something"[5m]))`, `sum by(name)(rate({region="us-east1"} | json | line_format "something else" |= "something"[5m]))`},
-		{`sum by(name)(rate({region="us-east1"} | json | line_format "something else" | logfmt[5m]))`, `sum by(name)(rate({region="us-east1"} | json | line_format "something else" | logfmt[5m]))`},
-		{`sum by(name)(count_over_time({region="us-east1"} | line_format "{{ .message }}" | json foo="bar"[5m]))`, `sum by(name)(count_over_time({region="us-east1"} | line_format "{{ .message }}" | json foo="bar"[5m]))`},
+		{`topk(10,sum by(name)(rate({region="us-east1"}[5m])))`, `topk(10,sum by (name)(rate({region="us-east1"}[5m])))`},
+		{`sum by(name)(rate({region="us-east1"}[5m]))`, `sum by (name)(rate({region="us-east1"}[5m]))`},
+		{`sum by(name)(bytes_over_time({region="us-east1"} | line_format "something else"[5m]))`, `sum by (name)(bytes_over_time({region="us-east1"} | line_format "something else"[5m]))`},
+		{`sum by(name)(rate({region="us-east1"} | json | line_format "something else" |= "something"[5m]))`, `sum by (name)(rate({region="us-east1"} | json | line_format "something else" |= "something"[5m]))`},
+		{`sum by(name)(rate({region="us-east1"} | json | line_format "something else" | logfmt[5m]))`, `sum by (name)(rate({region="us-east1"} | json | line_format "something else" | logfmt[5m]))`},
+		{`sum by(name)(count_over_time({region="us-east1"} | line_format "{{ .message }}" | json foo="bar"[5m]))`, `sum by (name)(count_over_time({region="us-east1"} | line_format "{{ .message }}" | json foo="bar"[5m]))`},
 
 		// remove line_format that is not required.
-		{`sum by(name)(rate({region="us-east1"} | line_format "something else"[5m]))`, `sum by(name)(rate({region="us-east1"}[5m]))`},
-		{`sum by(name)(rate({region="us-east1"} | json | line_format "something else" | unwrap foo[5m]))`, `sum by(name)(rate({region="us-east1"} | json | unwrap foo[5m]))`},
+		{`sum by(name)(rate({region="us-east1"} | line_format "something else"[5m]))`, `sum by (name)(rate({region="us-east1"}[5m]))`},
+		{`sum by(name)(rate({region="us-east1"} | json | line_format "something else" | unwrap foo[5m]))`, `sum by (name)(rate({region="us-east1"} | json | unwrap foo[5m]))`},
 		{`quantile_over_time(1,{region="us-east1"} | json | line_format "something else" | unwrap foo[5m])`, `quantile_over_time(1,{region="us-east1"} | json | unwrap foo[5m])`},
-		{`sum by(name)(count_over_time({region="us-east1"} | json | line_format "something else" | label_format foo=bar | line_format "boo"[5m]))`, `sum by(name)(count_over_time({region="us-east1"} | json | label_format foo=bar[5m]))`},
+		{`sum by(name)(count_over_time({region="us-east1"} | json | line_format "something else" | label_format foo=bar | line_format "boo"[5m]))`, `sum by (name)(count_over_time({region="us-east1"} | json | label_format foo=bar[5m]))`},
 	}
 	for _, tt := range tests {
 		t.Run(tt.in, func(t *testing.T) {

diff --git a/pkg/logql/syntax/ast.go b/pkg/logql/syntax/ast.go
@@ -930,9 +930,9 @@ type Grouping struct {
 func (g Grouping) String() string {
 	var sb strings.Builder
 	if g.Without {
-		sb.WriteString(" without")
+		sb.WriteString(" without ")
 	} else if len(g.Groups) > 0 {
-		sb.WriteString(" by")
+		sb.WriteString(" by ")
 	}
 
 	if len(g.Groups) > 0 {

diff --git a/pkg/logql/syntax/ast_test.go b/pkg/logql/syntax/ast_test.go
@@ -172,6 +172,12 @@ func Test_SampleExpr_String(t *testing.T) {
 			"(.*):.*"
 		)
 		`,
+		`(((
+			sum by(typename,pool,commandname,colo)(sum_over_time({_namespace_="appspace", _schema_="appspace-1min", pool=~"r1testlvs", colo=~"slc|lvs|rno", env!~"(pre-production|sandbox)"} | logfmt | status!="0" | ( ( type=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" or typename=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) or status=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) | commandname=~"(?i).*|UNSET" | unwrap sumcount[5m])) / 60) 
+				or on ()  
+				((sum by(typename,pool,commandname,colo)(sum_over_time({_namespace_="appspace", _schema_="appspace-15min", pool=~"r1testlvs", colo=~"slc|lvs|rno", env!~"(pre-production|sandbox)"} | logfmt | status!="0" | ( ( type=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" or typename=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) or status=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) | commandname=~"(?i).*|UNSET" | unwrap sumcount[5m])) / 15) / 60)) 
+				or on ()  
+				((sum by(typename,pool,commandname,colo) (sum_over_time({_namespace_="appspace", _schema_="appspace-1h", pool=~"r1testlvs", colo=~"slc|lvs|rno", env!~"(pre-production|sandbox)"} | logfmt | status!="0" | ( ( type=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" or typename=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) or status=~"(?i)^(Error|Exception|Fatal|ERRPAGE|ValidationError)$" ) | commandname=~"(?i).*|UNSET" | unwrap sumcount[5m])) / 60) / 60))`,
 	} {
 		t.Run(tc, func(t *testing.T) {
 			expr, err := ParseExpr(tc)

diff --git a/pkg/logql/syntax/walk_test.go b/pkg/logql/syntax/walk_test.go
@@ -49,7 +49,7 @@ func Test_AppendMatchers(t *testing.T) {
 		{
 			desc: "vector range query",
 			expr: `sum by(cluster)(rate({job="foo"} |= "bar" | logfmt | bazz="buzz"[5m]))`,
-			want: `sum by(cluster)(rate({job="foo", namespace="a"} |= "bar" | logfmt | bazz="buzz"[5m]))`,
+			want: `sum by (cluster)(rate({job="foo", namespace="a"} |= "bar" | logfmt | bazz="buzz"[5m]))`,
 			matchers: []*labels.Matcher{
 				{
 					Name:  "namespace",
@@ -61,7 +61,7 @@ func Test_AppendMatchers(t *testing.T) {
 		{
 			desc: "bin op query",
 			expr: `sum by(cluster)(rate({job="foo"} |= "bar" | logfmt | bazz="buzz"[5m])) / sum by(cluster)(rate({job="foo"} |= "bar" | logfmt | bazz="buzz"[5m]))`,
-			want: `(sum by(cluster)(rate({job="foo", namespace="a"} |= "bar" | logfmt | bazz="buzz"[5m])) / sum by(cluster)(rate({job="foo", namespace="a"} |= "bar" | logfmt | bazz="buzz"[5m])))`,
+			want: `(sum by (cluster)(rate({job="foo", namespace="a"} |= "bar" | logfmt | bazz="buzz"[5m])) / sum by (cluster)(rate({job="foo", namespace="a"} |= "bar" | logfmt | bazz="buzz"[5m])))`,
 			matchers: []*labels.Matcher{
 				{
 					Name:  "namespace",

diff --git a/pkg/querier/queryrange/split_by_range_test.go b/pkg/querier/queryrange/split_by_range_test.go
@@ -52,9 +52,9 @@ func Test_RangeVectorSplit(t *testing.T) {
 				Path:   "/loki/api/v1/query",
 			},
 			subQueries: []queryrangebase.RequestResponse{
-				subQueryRequestResponse(`sum by(bar)(bytes_over_time({app="foo"}[1m]))`, 10),
-				subQueryRequestResponse(`sum by(bar)(bytes_over_time({app="foo"}[1m] offset 1m0s))`, 20),
-				subQueryRequestResponse(`sum by(bar)(bytes_over_time({app="foo"}[1m] offset 2m0s))`, 30),
+				subQueryRequestResponse(`sum by (bar)(bytes_over_time({app="foo"}[1m]))`, 10),
+				subQueryRequestResponse(`sum by (bar)(bytes_over_time({app="foo"}[1m] offset 1m0s))`, 20),
+				subQueryRequestResponse(`sum by (bar)(bytes_over_time({app="foo"}[1m] offset 2m0s))`, 30),
 			},
 			expected: expectedMergedResponse(10 + 20 + 30),
 		},
@@ -78,9 +78,9 @@ func Test_RangeVectorSplit(t *testing.T) {
 				Path:   "/loki/api/v1/query",
 			},
 			subQueries: []queryrangebase.RequestResponse{
-				subQueryRequestResponse(`sum by(bar)(count_over_time({app="foo"}[1m]))`, 0),
-				subQueryRequestResponse(`sum by(bar)(count_over_time({app="foo"}[1m] offset 1m0s))`, 0),
-				subQueryRequestResponse(`sum by(bar)(count_over_time({app="foo"}[1m] offset 2m0s))`, 0),
+				subQueryRequestResponse(`sum by (bar)(count_over_time({app="foo"}[1m]))`, 0),
+				subQueryRequestResponse(`sum by (bar)(count_over_time({app="foo"}[1m] offset 1m0s))`, 0),
+				subQueryRequestResponse(`sum by (bar)(count_over_time({app="foo"}[1m] offset 2m0s))`, 0),
 			},
 			expected: expectedMergedResponse(0 + 0 + 0),
 		},
@@ -104,9 +104,9 @@ func Test_RangeVectorSplit(t *testing.T) {
 				Path:   "/loki/api/v1/query",
 			},
 			subQueries: []queryrangebase.RequestResponse{
-				subQueryRequestResponse(`sum by(bar)(sum_over_time({app="foo"} | unwrap bar[1m]))`, 1),
-				subQueryRequestResponse(`sum by(bar)(sum_over_time({app="foo"} | unwrap bar[1m] offset 1m0s))`, 2),
-				subQueryRequestResponse(`sum by(bar)(sum_over_time({app="foo"} | unwrap bar[1m] offset 2m0s))`, 3),
+				subQueryRequestResponse(`sum by (bar)(sum_over_time({app="foo"} | unwrap bar[1m]))`, 1),
+				subQueryRequestResponse(`sum by (bar)(sum_over_time({app="foo"} | unwrap bar[1m] offset 1m0s))`, 2),
+				subQueryRequestResponse(`sum by (bar)(sum_over_time({app="foo"} | unwrap bar[1m] offset 2m0s))`, 3),
 			},
 			expected: expectedMergedResponse(1 + 2 + 3),
 		},