Adds support for prettifying PromQL expression (#10544)

* Implement Pretty() function for AST nodes.

Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>

This commit adds .Pretty() for all nodes of PromQL AST.
Each .Pretty() prettifies the node it belongs to, and under
no circustance, the parent or child node is touch/prettified.

Read more in the "Approach" part in `prettier.go`

* Refactor functions between printer.go & prettier.go

Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>

This commit removes redundancy between printer.go and prettier.go
by taking out the common code into separate private functions.

* Add more unit tests for Prettier.

Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>

* Add support for spliting function calls with 1 arg & unary expressions.

Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>

This commit does 2 things:
1. It adds support to split function calls that have 1 arg and exceeds the max_characters_per_line
to multiple lines.
2. Splits Unary expressions that exceed the max_characters_per_line. This is done by formatting the child node
and then removing the prefix indent, which is already applied before the unary operator.

promql/parser/ast.go

@@ -40,6 +40,11 @@ type Node interface {
 	// as part of a valid query.
 	String() string
 
+	// Pretty returns the prettified representation of the node.
+	// It uses the level information to determine at which level/depth the current
+	// node is in the AST and uses this to apply indentation.
+	Pretty(level int) string
+
 	// PositionRange returns the position of the AST Node in the query string.
 	PositionRange() PositionRange
 }
@@ -205,8 +210,9 @@ type VectorSelector struct {
 // of an arbitrary function during handling. It is used to test the Engine.
 type TestStmt func(context.Context) error
 
-func (TestStmt) String() string { return "test statement" }
+func (TestStmt) String() string      { return "test statement" }
-func (TestStmt) PromQLStmt()    {}
+func (TestStmt) PromQLStmt()         {}
+func (t TestStmt) Pretty(int) string { return t.String() }
 
 func (TestStmt) PositionRange() PositionRange {
 	return PositionRange{

promql/parser/lex.go

@@ -48,6 +48,10 @@ func (i Item) String() string {
 	return fmt.Sprintf("%q", i.Val)
 }
 
+// Pretty returns the prettified form of an item.
+// This is same as the item's stringified format.
+func (i Item) Pretty(int) string { return i.String() }
+
 // IsOperator returns true if the Item corresponds to a arithmetic or set operator.
 // Returns false otherwise.
 func (i ItemType) IsOperator() bool { return i > operatorsStart && i < operatorsEnd }

promql/parser/prettier.go

@@ -0,0 +1,166 @@
+// Copyright 2022 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package parser
+
+import (
+	"fmt"
+	"strings"
+)
+
+// Approach
+// --------
+// When a PromQL query is parsed, it is converted into PromQL AST,
+// which is a nested structure of nodes. Each node has a depth/level
+// (distance from the root), that is passed by its parent.
+//
+// While prettifying, a Node considers 2 things:
+// 1. Did the current Node's parent add a new line?
+// 2. Does the current Node needs to be prettified?
+//
+// The level of a Node determines if it should be indented or not.
+// The answer to the 1 is NO if the level passed is 0. This means, the
+// parent Node did not apply a new line, so the current Node must not
+// apply any indentation as prefix.
+// If level > 1, a new line is applied by the parent. So, the current Node
+// should prefix an indentation before writing any of its content. This indentation
+// will be ([level/depth of current Node] * "  ").
+//
+// The answer to 2 is YES if the normalized length of the current Node exceeds
+// the maxCharactersPerLine limit. Hence, it applies the indentation equal to
+// its depth and increments the level by 1 before passing down the child.
+// If the answer is NO, the current Node returns the normalized string value of itself.
+
+var maxCharactersPerLine = 100
+
+func Prettify(n Node) string {
+	return n.Pretty(0)
+}
+
+func (e *AggregateExpr) Pretty(level int) string {
+	s := indent(level)
+	if !needsSplit(e) {
+		s += e.String()
+		return s
+	}
+
+	s += e.getAggOpStr()
+	s += "(\n"
+
+	if e.Op.IsAggregatorWithParam() {
+		s += fmt.Sprintf("%s,\n", e.Param.Pretty(level+1))
+	}
+	s += fmt.Sprintf("%s\n%s)", e.Expr.Pretty(level+1), indent(level))
+	return s
+}
+
+func (e *BinaryExpr) Pretty(level int) string {
+	s := indent(level)
+	if !needsSplit(e) {
+		s += e.String()
+		return s
+	}
+	returnBool := ""
+	if e.ReturnBool {
+		returnBool = " bool"
+	}
+
+	matching := e.getMatchingStr()
+	return fmt.Sprintf("%s\n%s%s%s%s\n%s", e.LHS.Pretty(level+1), indent(level), e.Op, returnBool, matching, e.RHS.Pretty(level+1))
+}
+
+func (e *Call) Pretty(level int) string {
+	s := indent(level)
+	if !needsSplit(e) {
+		s += e.String()
+		return s
+	}
+	s += fmt.Sprintf("%s(\n%s\n%s)", e.Func.Name, e.Args.Pretty(level+1), indent(level))
+	return s
+}
+
+func (e *EvalStmt) Pretty(_ int) string {
+	return "EVAL " + e.Expr.String()
+}
+
+func (e Expressions) Pretty(level int) string {
+	// Do not prefix the indent since respective nodes will indent itself.
+	s := ""
+	for i := range e {
+		s += fmt.Sprintf("%s,\n", e[i].Pretty(level))
+	}
+	return s[:len(s)-2]
+}
+
+func (e *ParenExpr) Pretty(level int) string {
+	s := indent(level)
+	if !needsSplit(e) {
+		s += e.String()
+		return s
+	}
+	return fmt.Sprintf("%s(\n%s\n%s)", s, e.Expr.Pretty(level+1), indent(level))
+}
+
+func (e *StepInvariantExpr) Pretty(level int) string {
+	return e.Expr.Pretty(level)
+}
+
+func (e *MatrixSelector) Pretty(level int) string {
+	return getCommonPrefixIndent(level, e)
+}
+
+func (e *SubqueryExpr) Pretty(level int) string {
+	if !needsSplit(e) {
+		return e.String()
+	}
+	return fmt.Sprintf("%s%s", e.Expr.Pretty(level), e.getSubqueryTimeSuffix())
+}
+
+func (e *VectorSelector) Pretty(level int) string {
+	return getCommonPrefixIndent(level, e)
+}
+
+func (e *NumberLiteral) Pretty(level int) string {
+	return getCommonPrefixIndent(level, e)
+}
+
+func (e *StringLiteral) Pretty(level int) string {
+	return getCommonPrefixIndent(level, e)
+}
+
+func (e *UnaryExpr) Pretty(level int) string {
+	child := e.Expr.Pretty(level)
+	// Remove the indent prefix from child since we attach the prefix indent before Op.
+	child = strings.TrimSpace(child)
+	return fmt.Sprintf("%s%s%s", indent(level), e.Op, child)
+}
+
+func getCommonPrefixIndent(level int, current Node) string {
+	return fmt.Sprintf("%s%s", indent(level), current.String())
+}
+
+// needsSplit normalizes the node and then checks if the node needs any split.
+// This is necessary to remove any trailing whitespaces.
+func needsSplit(n Node) bool {
+	if n == nil {
+		return false
+	}
+	return len(n.String()) > maxCharactersPerLine
+}
+
+const indentString = "  "
+
+// indent adds the indentString n number of times.
+func indent(n int) string {
+	return strings.Repeat(indentString, n)
+}

promql/parser/prettier_rules.md

@@ -0,0 +1,16 @@
+# Prettifying PromQL expressions
+This files contains rules for prettifying PromQL expressions.
+
+Note: The current version of prettier does not preserve comments.
+
+### Keywords
+`max_characters_per_line`: Maximum number of characters that will be allowed on a single line in a prettified PromQL expression.
+
+### Rules
+1. A node exceeding the `max_characters_per_line` will qualify for split unless
+   1. It is a `MatrixSelector`
+   2. It is a `VectorSelector`. Label sets in a `VectorSelector` will be in the same line as metric_name, separated by commas and a space
+   Note: Label groupings like `by`, `without`, `on`, `ignoring` will remain on the same line as their parent node
+2. Nodes that are nested within another node will be prettified only if they exceed the `max_characters_per_line`
+3. Expressions like `sum(expression) without (label_matchers)` will be modified to `sum without(label_matchers) (expression)`
+4. Functional call args will be split to different lines if they exceed the `max_characters_per_line`

promql/parser/prettier_test.go

@@ -0,0 +1,666 @@
+// Copyright 2022 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package parser
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestAggregateExprPretty(t *testing.T) {
+	maxCharactersPerLine = 10
+	inputs := []struct {
+		in, out string
+	}{
+		{
+			in:  `sum(foo)`,
+			out: `sum(foo)`,
+		},
+		{
+			in: `sum by() (task:errors:rate10s{job="s"})`,
+			out: `sum(
+  task:errors:rate10s{job="s"}
+)`,
+		},
+		{
+			in: `sum without(job,foo) (task:errors:rate10s{job="s"})`,
+			out: `sum without(job, foo) (
+  task:errors:rate10s{job="s"}
+)`,
+		},
+		{
+			in: `sum(task:errors:rate10s{job="s"}) without(job,foo)`,
+			out: `sum without(job, foo) (
+  task:errors:rate10s{job="s"}
+)`,
+		},
+		{
+			in: `sum by(job,foo) (task:errors:rate10s{job="s"})`,
+			out: `sum by(job, foo) (
+  task:errors:rate10s{job="s"}
+)`,
+		},
+		{
+			in: `sum (task:errors:rate10s{job="s"}) by(job,foo)`,
+			out: `sum by(job, foo) (
+  task:errors:rate10s{job="s"}
+)`,
+		},
+		{
+			in: `topk(10, ask:errors:rate10s{job="s"})`,
+			out: `topk(
+  10,
+  ask:errors:rate10s{job="s"}
+)`,
+		},
+		{
+			in: `sum by(job,foo) (sum by(job,foo) (task:errors:rate10s{job="s"}))`,
+			out: `sum by(job, foo) (
+  sum by(job, foo) (
+    task:errors:rate10s{job="s"}
+  )
+)`,
+		},
+		{
+			in: `sum by(job,foo) (sum by(job,foo) (sum by(job,foo) (task:errors:rate10s{job="s"})))`,
+			out: `sum by(job, foo) (
+  sum by(job, foo) (
+    sum by(job, foo) (
+      task:errors:rate10s{job="s"}
+    )
+  )
+)`,
+		},
+		{
+			in: `sum by(job,foo)
+(sum by(job,foo) (task:errors:rate10s{job="s"}))`,
+			out: `sum by(job, foo) (
+  sum by(job, foo) (
+    task:errors:rate10s{job="s"}
+  )
+)`,
+		},
+		{
+			in: `sum by(job,foo)
+(sum(task:errors:rate10s{job="s"}) without(job,foo))`,
+			out: `sum by(job, foo) (
+  sum without(job, foo) (
+    task:errors:rate10s{job="s"}
+  )
+)`,
+		},
+		{
+			in: `sum by(job,foo) # Comment 1.
+(sum by(job,foo) ( # Comment 2.
+task:errors:rate10s{job="s"}))`,
+			out: `sum by(job, foo) (
+  sum by(job, foo) (
+    task:errors:rate10s{job="s"}
+  )
+)`,
+		},
+	}
+	for _, test := range inputs {
+		expr, err := ParseExpr(test.in)
+		require.NoError(t, err)
+
+		require.Equal(t, test.out, Prettify(expr))
+	}
+}
+
+func TestBinaryExprPretty(t *testing.T) {
+	maxCharactersPerLine = 10
+	inputs := []struct {
+		in, out string
+	}{
+		{
+			in:  `a+b`,
+			out: `a + b`,
+		},
+		{
+			in: `a == bool 1`,
+			out: `  a
+== bool
+  1`,
+		},
+		{
+			in: `a + ignoring(job) b`,
+			out: `  a
++ ignoring(job)
+  b`,
+		},
+		{
+			in: `foo_1 + foo_2`,
+			out: `  foo_1
++
+  foo_2`,
+		},
+		{
+			in: `foo_1 + foo_2 + foo_3`,
+			out: `    foo_1
+  +
+    foo_2
++
+  foo_3`,
+		},
+		{
+			in: `foo + baar + foo_3`,
+			out: `  foo + baar
++
+  foo_3`,
+		},
+		{
+			in: `foo_1 + foo_2 + foo_3 + foo_4`,
+			out: `      foo_1
+    +
+      foo_2
+  +
+    foo_3
++
+  foo_4`,
+		},
+		{
+			in: `foo_1 + ignoring(foo) foo_2 + ignoring(job) group_left foo_3 + on(instance) group_right foo_4`,
+			out: `      foo_1
+    + ignoring(foo)
+      foo_2
+  + ignoring(job) group_left()
+    foo_3
++ on(instance) group_right()
+  foo_4`,
+		},
+	}
+	for _, test := range inputs {
+		expr, err := ParseExpr(test.in)
+		require.NoError(t, err)
+
+		require.Equal(t, test.out, Prettify(expr))
+	}
+}
+
+func TestCallExprPretty(t *testing.T) {
+	maxCharactersPerLine = 10
+	inputs := []struct {
+		in, out string
+	}{
+		{
+			in: `rate(foo[1m])`,
+			out: `rate(
+  foo[1m]
+)`,
+		},
+		{
+			in: `sum_over_time(foo[1m])`,
+			out: `sum_over_time(
+  foo[1m]
+)`,
+		},
+		{
+			in: `rate(long_vector_selector[10m:1m] @ start() offset 1m)`,
+			out: `rate(
+  long_vector_selector[10m:1m] @ start() offset 1m
+)`,
+		},
+		{
+			in: `histogram_quantile(0.9, rate(foo[1m]))`,
+			out: `histogram_quantile(
+  0.9,
+  rate(
+    foo[1m]
+  )
+)`,
+		},
+		{
+			in: `max_over_time(rate(demo_api_request_duration_seconds_count[1m])[1m:] @ start() offset 1m)`,
+			out: `max_over_time(
+  rate(
+    demo_api_request_duration_seconds_count[1m]
+  )[1m:] @ start() offset 1m
+)`,
+		},
+		{
+			in: `label_replace(up{job="api-server",service="a:c"}, "foo", "$1", "service", "(.*):.*")`,
+			out: `label_replace(
+  up{job="api-server",service="a:c"},
+  "foo",
+  "$1",
+  "service",
+  "(.*):.*"
+)`,
+		},
+		{
+			in: `label_replace(label_replace(up{job="api-server",service="a:c"}, "foo", "$1", "service", "(.*):.*"), "foo", "$1", "service", "(.*):.*")`,
+			out: `label_replace(
+  label_replace(
+    up{job="api-server",service="a:c"},
+    "foo",
+    "$1",
+    "service",
+    "(.*):.*"
+  ),
+  "foo",
+  "$1",
+  "service",
+  "(.*):.*"
+)`,
+		},
+	}
+	for _, test := range inputs {
+		expr, err := ParseExpr(test.in)
+		require.NoError(t, err)
+
+		fmt.Println("=>", expr.String())
+		require.Equal(t, test.out, Prettify(expr))
+	}
+}
+
+func TestParenExprPretty(t *testing.T) {
+	maxCharactersPerLine = 10
+	inputs := []struct {
+		in, out string
+	}{
+		{
+			in:  `(foo)`,
+			out: `(foo)`,
+		},
+		{
+			in: `(_foo_long_)`,
+			out: `(
+  _foo_long_
+)`,
+		},
+		{
+			in: `((foo_long))`,
+			out: `(
+  (foo_long)
+)`,
+		},
+		{
+			in: `((_foo_long_))`,
+			out: `(
+  (
+    _foo_long_
+  )
+)`,
+		},
+		{
+			in: `(((foo_long)))`,
+			out: `(
+  (
+    (foo_long)
+  )
+)`,
+		},
+	}
+	for _, test := range inputs {
+		expr, err := ParseExpr(test.in)
+		require.NoError(t, err)
+
+		require.Equal(t, test.out, Prettify(expr))
+	}
+}
+
+func TestStepInvariantExpr(t *testing.T) {
+	maxCharactersPerLine = 10
+	inputs := []struct {
+		in, out string
+	}{
+		{
+			in:  `a @ 1`,
+			out: `a @ 1.000`,
+		},
+		{
+			in:  `a @ start()`,
+			out: `a @ start()`,
+		},
+		{
+			in:  `vector_selector @ start()`,
+			out: `vector_selector @ start()`,
+		},
+	}
+	for _, test := range inputs {
+		expr, err := ParseExpr(test.in)
+		require.NoError(t, err)
+
+		require.Equal(t, test.out, Prettify(expr))
+	}
+}
+
+func TestExprPretty(t *testing.T) {
+	maxCharactersPerLine = 10
+	inputs := []struct {
+		in, out string
+	}{
+		{
+			in:  `(1 + 2)`,
+			out: `(1 + 2)`,
+		},
+		{
+			in: `(foo + bar)`,
+			out: `(
+  foo + bar
+)`,
+		},
+		{
+			in: `(foo_long + bar_long)`,
+			out: `(
+    foo_long
+  +
+    bar_long
+)`,
+		},
+		{
+			in: `(foo_long + bar_long + bar_2_long)`,
+			out: `(
+      foo_long
+    +
+      bar_long
+  +
+    bar_2_long
+)`,
+		},
+		{
+			in: `((foo_long + bar_long) + bar_2_long)`,
+			out: `(
+    (
+        foo_long
+      +
+        bar_long
+    )
+  +
+    bar_2_long
+)`,
+		},
+		{
+			in: `(1111 + 2222)`,
+			out: `(
+    1111
+  +
+    2222
+)`,
+		},
+		{
+			in: `(sum_over_time(foo[1m]))`,
+			out: `(
+  sum_over_time(
+    foo[1m]
+  )
+)`,
+		},
+		{
+			in: `histogram_quantile(0.9, rate(foo[1m] @ start()))`,
+			out: `histogram_quantile(
+  0.9,
+  rate(
+    foo[1m] @ start()
+  )
+)`,
+		},
+		{
+			in: `(label_replace(up{job="api-server",service="a:c"}, "foo", "$1", "service", "(.*):.*"))`,
+			out: `(
+  label_replace(
+    up{job="api-server",service="a:c"},
+    "foo",
+    "$1",
+    "service",
+    "(.*):.*"
+  )
+)`,
+		},
+		{
+			in: `(label_replace(label_replace(up{job="api-server",service="a:c"}, "foo", "$1", "service", "(.*):.*"), "foo", "$1", "service", "(.*):.*"))`,
+			out: `(
+  label_replace(
+    label_replace(
+      up{job="api-server",service="a:c"},
+      "foo",
+      "$1",
+      "service",
+      "(.*):.*"
+    ),
+    "foo",
+    "$1",
+    "service",
+    "(.*):.*"
+  )
+)`,
+		},
+		{
+			in: `(label_replace(label_replace((up{job="api-server",service="a:c"}), "foo", "$1", "service", "(.*):.*"), "foo", "$1", "service", "(.*):.*"))`,
+			out: `(
+  label_replace(
+    label_replace(
+      (
+        up{job="api-server",service="a:c"}
+      ),
+      "foo",
+      "$1",
+      "service",
+      "(.*):.*"
+    ),
+    "foo",
+    "$1",
+    "service",
+    "(.*):.*"
+  )
+)`,
+		},
+		// Following queries have been taken from https://monitoring.mixins.dev/
+		{
+			in: `(node_filesystem_avail_bytes{job="node",fstype!=""} / node_filesystem_size_bytes{job="node",fstype!=""} * 100 < 40 and predict_linear(node_filesystem_avail_bytes{job="node",fstype!=""}[6h], 24*60*60) < 0 and node_filesystem_readonly{job="node",fstype!=""} == 0)`,
+			out: `(
+            node_filesystem_avail_bytes{fstype!="",job="node"}
+          /
+            node_filesystem_size_bytes{fstype!="",job="node"}
+        *
+          100
+      <
+        40
+    and
+        predict_linear(
+          node_filesystem_avail_bytes{fstype!="",job="node"}[6h],
+            24 * 60
+          *
+            60
+        )
+      <
+        0
+  and
+      node_filesystem_readonly{fstype!="",job="node"}
+    ==
+      0
+)`,
+		},
+		{
+			in: `(node_filesystem_avail_bytes{job="node",fstype!=""} / node_filesystem_size_bytes{job="node",fstype!=""} * 100 < 20 and predict_linear(node_filesystem_avail_bytes{job="node",fstype!=""}[6h], 4*60*60) < 0 and node_filesystem_readonly{job="node",fstype!=""} == 0)`,
+			out: `(
+            node_filesystem_avail_bytes{fstype!="",job="node"}
+          /
+            node_filesystem_size_bytes{fstype!="",job="node"}
+        *
+          100
+      <
+        20
+    and
+        predict_linear(
+          node_filesystem_avail_bytes{fstype!="",job="node"}[6h],
+            4 * 60
+          *
+            60
+        )
+      <
+        0
+  and
+      node_filesystem_readonly{fstype!="",job="node"}
+    ==
+      0
+)`,
+		},
+		{
+			in: `(node_timex_offset_seconds > 0.05 and deriv(node_timex_offset_seconds[5m]) >= 0) or (node_timex_offset_seconds < -0.05 and deriv(node_timex_offset_seconds[5m]) <= 0)`,
+			out: `  (
+        node_timex_offset_seconds
+      >
+        0.05
+    and
+        deriv(
+          node_timex_offset_seconds[5m]
+        )
+      >=
+        0
+  )
+or
+  (
+        node_timex_offset_seconds
+      <
+        -0.05
+    and
+        deriv(
+          node_timex_offset_seconds[5m]
+        )
+      <=
+        0
+  )`,
+		},
+		{
+			in: `1 - ((node_memory_MemAvailable_bytes{job="node"} or (node_memory_Buffers_bytes{job="node"} + node_memory_Cached_bytes{job="node"} + node_memory_MemFree_bytes{job="node"} + node_memory_Slab_bytes{job="node"}) ) / node_memory_MemTotal_bytes{job="node"})`,
+			out: `  1
+-
+  (
+      (
+          node_memory_MemAvailable_bytes{job="node"}
+        or
+          (
+                  node_memory_Buffers_bytes{job="node"}
+                +
+                  node_memory_Cached_bytes{job="node"}
+              +
+                node_memory_MemFree_bytes{job="node"}
+            +
+              node_memory_Slab_bytes{job="node"}
+          )
+      )
+    /
+      node_memory_MemTotal_bytes{job="node"}
+  )`,
+		},
+		{
+			in: `min by (job, integration) (rate(alertmanager_notifications_failed_total{job="alertmanager", integration=~".*"}[5m]) / rate(alertmanager_notifications_total{job="alertmanager", integration="~.*"}[5m])) > 0.01`,
+			out: `  min by(job, integration) (
+      rate(
+        alertmanager_notifications_failed_total{integration=~".*",job="alertmanager"}[5m]
+      )
+    /
+      rate(
+        alertmanager_notifications_total{integration="~.*",job="alertmanager"}[5m]
+      )
+  )
+>
+  0.01`,
+		},
+		{
+			in: `(count by (job) (changes(process_start_time_seconds{job="alertmanager"}[10m]) > 4) / count by (job) (up{job="alertmanager"})) >= 0.5`,
+			out: `  (
+      count by(job) (
+          changes(
+            process_start_time_seconds{job="alertmanager"}[10m]
+          )
+        >
+          4
+      )
+    /
+      count by(job) (
+        up{job="alertmanager"}
+      )
+  )
+>=
+  0.5`,
+		},
+	}
+	for _, test := range inputs {
+		expr, err := ParseExpr(test.in)
+		require.NoError(t, err)
+		require.Equal(t, test.out, Prettify(expr))
+	}
+}
+
+func TestUnaryPretty(t *testing.T) {
+	maxCharactersPerLine = 10
+	inputs := []struct {
+		in, out string
+	}{
+		{
+			in:  `-1`,
+			out: `-1`,
+		},
+		{
+			in:  `-vector_selector`,
+			out: `-vector_selector`,
+		},
+		{
+			in: `(-vector_selector)`,
+			out: `(
+  -vector_selector
+)`,
+		},
+		{
+			in: `-histogram_quantile(0.9,rate(foo[1m]))`,
+			out: `-histogram_quantile(
+  0.9,
+  rate(
+    foo[1m]
+  )
+)`,
+		},
+		{
+			in: `-histogram_quantile(0.99, sum by (le) (rate(foo[1m])))`,
+			out: `-histogram_quantile(
+  0.99,
+  sum by(le) (
+    rate(
+      foo[1m]
+    )
+  )
+)`,
+		},
+		{
+			in: `-histogram_quantile(0.9, -rate(foo[1m] @ start()))`,
+			out: `-histogram_quantile(
+  0.9,
+  -rate(
+    foo[1m] @ start()
+  )
+)`,
+		},
+		{
+			in: `(-histogram_quantile(0.9, -rate(foo[1m] @ start())))`,
+			out: `(
+  -histogram_quantile(
+    0.9,
+    -rate(
+      foo[1m] @ start()
+    )
+  )
+)`,
+		},
+	}
+	for _, test := range inputs {
+		expr, err := ParseExpr(test.in)
+		require.NoError(t, err)
+		require.Equal(t, test.out, Prettify(expr))
+	}
+}

promql/parser/printer.go

@@ -62,16 +62,7 @@ func (es Expressions) String() (s string) {
 }
 
 func (node *AggregateExpr) String() string {
-	aggrString := node.Op.String()
+	aggrString := node.getAggOpStr()
-
-	if node.Without {
-		aggrString += fmt.Sprintf(" without(%s) ", strings.Join(node.Grouping, ", "))
-	} else {
-		if len(node.Grouping) > 0 {
-			aggrString += fmt.Sprintf(" by(%s) ", strings.Join(node.Grouping, ", "))
-		}
-	}
-
 	aggrString += "("
 	if node.Op.IsAggregatorWithParam() {
 		aggrString += fmt.Sprintf("%s, ", node.Param)
@@ -81,31 +72,48 @@ func (node *AggregateExpr) String() string {
 	return aggrString
 }
 
+func (node *AggregateExpr) getAggOpStr() string {
+	aggrString := node.Op.String()
+
+	switch {
+	case node.Without:
+		aggrString += fmt.Sprintf(" without(%s) ", strings.Join(node.Grouping, ", "))
+	case len(node.Grouping) > 0:
+		aggrString += fmt.Sprintf(" by(%s) ", strings.Join(node.Grouping, ", "))
+	}
+
+	return aggrString
+}
+
 func (node *BinaryExpr) String() string {
 	returnBool := ""
 	if node.ReturnBool {
 		returnBool = " bool"
 	}
 
+	matching := node.getMatchingStr()
+	return fmt.Sprintf("%s %s%s%s %s", node.LHS, node.Op, returnBool, matching, node.RHS)
+}
+
+func (node *BinaryExpr) getMatchingStr() string {
 	matching := ""
 	vm := node.VectorMatching
 	if vm != nil && (len(vm.MatchingLabels) > 0 || vm.On) {
+		vmTag := "ignoring"
 		if vm.On {
-			matching = fmt.Sprintf(" on(%s)", strings.Join(vm.MatchingLabels, ", "))
+			vmTag = "on"
-		} else {
-			matching = fmt.Sprintf(" ignoring(%s)", strings.Join(vm.MatchingLabels, ", "))
 		}
+		matching = fmt.Sprintf(" %s(%s)", vmTag, strings.Join(vm.MatchingLabels, ", "))
+
 		if vm.Card == CardManyToOne || vm.Card == CardOneToMany {
-			matching += " group_"
+			vmCard := "right"
 			if vm.Card == CardManyToOne {
-				matching += "left"
+				vmCard = "left"
-			} else {
-				matching += "right"
 			}
-			matching += fmt.Sprintf("(%s)", strings.Join(vm.Include, ", "))
+			matching += fmt.Sprintf(" group_%s(%s)", vmCard, strings.Join(vm.Include, ", "))
 		}
 	}
-	return fmt.Sprintf("%s %s%s%s %s", node.LHS, node.Op, returnBool, matching, node.RHS)
+	return matching
 }
 
 func (node *Call) String() string {
@@ -144,6 +152,11 @@ func (node *MatrixSelector) String() string {
 }
 
 func (node *SubqueryExpr) String() string {
+	return fmt.Sprintf("%s%s", node.Expr.String(), node.getSubqueryTimeSuffix())
+}
+
+// getSubqueryTimeSuffix returns the '[<range>:<step>] @ <timestamp> offset <offset>' suffix of the subquery.
+func (node *SubqueryExpr) getSubqueryTimeSuffix() string {
 	step := ""
 	if node.Step != 0 {
 		step = model.Duration(node.Step).String()
@@ -162,7 +175,7 @@ func (node *SubqueryExpr) String() string {
 	} else if node.StartOrEnd == END {
 		at = " @ end()"
 	}
-	return fmt.Sprintf("%s[%s:%s]%s%s", node.Expr.String(), model.Duration(node.Range), step, at, offset)
+	return fmt.Sprintf("[%s:%s]%s%s", model.Duration(node.Range), step, at, offset)
 }
 
 func (node *NumberLiteral) String() string {