Avoid unnecessary parentheses in simple binary expressions

Fixes #54

needBinaryOpArgParens now takes the parent operator as a parameter and
uses three-tier logic: add parens when (1) the child op has lower
priority than the parent, (2) the parent is a
  non-arithmetic op (or, and, comparisons, etc.), or (3) the child
subtree contains non-trivial nodes (function calls, group/join
modifiers, etc.). Plain arithmetic chains of numbers and
  metric names — 1 + 2 + 3, a + b + c — no longer get wrapped in
redundant parentheses.

Tests updated accordingly.

w

Author: makasim

optimizer_test.go

@@ -50,7 +50,7 @@ func TestPushdownBinaryOpFilters(t *testing.T) {
 	f(`foo * ignoring(x) bar`, `{a="b"}`, `foo{a="b"} * ignoring(x) bar{a="b"}`)
 	f(`foo{f1!~"x"} UNLEss bar{f2=~"y.+"}`, `{a="b",x=~"y"}`, `foo{a="b",f1!~"x",x=~"y"} unless bar{a="b",f2=~"y.+",x=~"y"}`)
 	f(`a / sum(x)`, `{a="b",c=~"foo|bar"}`, `a{a="b",c=~"foo|bar"} / sum(x)`)
-	f(`round(rate(x[5m] offset -1h)) + 123 / {a="b"}`, `{x!="y"}`, `round(rate(x{x!="y"}[5m] offset -1h)) + (123 / {a="b",x!="y"})`)
+	f(`round(rate(x[5m] offset -1h)) + 123 / {a="b"}`, `{x!="y"}`, `round(rate(x{x!="y"}[5m] offset -1h)) + 123 / {a="b",x!="y"}`)
 	f(`scalar(foo)+bar`, `{a="b"}`, `scalar(foo) + bar{a="b"}`)
 	f(`vector(foo)`, `{a="b"}`, `vector(foo{a="b"})`)
 	f(`{a="b"} + on() group_left() {c="d"}`, `{a="b"}`, `{a="b"} + on() group_left() {c="d"}`)
@@ -382,9 +382,9 @@ func TestOptimize(t *testing.T) {
 	f(`rate(sum(foo[5m:]) by (baz)) + bar{baz="a"}`, `rate(sum(foo{baz="a"}[5m:]) by(baz)) + bar{baz="a"}`)
 
 	// binary ops with constants or scalars
-	f(`100 * foo / bar{baz="a"}`, `(100 * foo{baz="a"}) / bar{baz="a"}`)
-	f(`foo * 100 / bar{baz="a"}`, `(foo{baz="a"} * 100) / bar{baz="a"}`)
-	f(`foo / bar{baz="a"} * 100`, `(foo{baz="a"} / bar{baz="a"}) * 100`)
+	f(`100 * foo / bar{baz="a"}`, `100 * foo{baz="a"} / bar{baz="a"}`)
+	f(`foo * 100 / bar{baz="a"}`, `foo{baz="a"} * 100 / bar{baz="a"}`)
+	f(`foo / bar{baz="a"} * 100`, `foo{baz="a"} / bar{baz="a"} * 100`)
 	f(`scalar(x) * foo / bar{baz="a"}`, `(scalar(x) * foo{baz="a"}) / bar{baz="a"}`)
 	f(`SCALAR(x) * foo / bar{baz="a"}`, `(SCALAR(x) * foo{baz="a"}) / bar{baz="a"}`)
 	f(`100 * on(foo) bar{baz="z"} + a`, `(100 * on(foo) bar{baz="z"}) + a`)

parser.go

@@ -1933,11 +1933,11 @@ func (be *BinaryOpExpr) appendStringNoKeepMetricNames(dst []byte) []byte {
 }
 
 func (be *BinaryOpExpr) needLeftParens() bool {
-	return needBinaryOpArgParens(be.Left)
+	return needBinaryOpArgParens(be.Left, be.Op)
 }
 
 func (be *BinaryOpExpr) needRightParens() bool {
-	if needBinaryOpArgParens(be.Right) {
+	if needBinaryOpArgParens(be.Right, be.Op) {
 		return true
 	}
 	switch t := be.Right.(type) {
@@ -1974,10 +1974,21 @@ func (be *BinaryOpExpr) appendModifiers(dst []byte) []byte {
 	return dst
 }
 
-func needBinaryOpArgParens(arg Expr) bool {
+func needBinaryOpArgParens(arg Expr, parentOp string) bool {
 	switch t := arg.(type) {
 	case *BinaryOpExpr:
-		return true
+		// Parens are required when the child op has lower priority than the parent op,
+		// since removing them would change the evaluation order.
+		if binaryOpPriority(t.Op) < binaryOpPriority(parentOp) {
+			return true
+		}
+
+		if parentOp != "+" && parentOp != "-" && parentOp != "*" && parentOp != "/" {
+			return true
+		}
+
+		// Same op: parens are only needed when the sub-expression is not a simple leaf chain.
+		return !isBinaryOpLeafSimple(t)
 	case *RollupExpr:
 		if be, ok := t.Expr.(*BinaryOpExpr); ok && be.KeepMetricNames {
 			return true
@@ -1988,6 +1999,24 @@ func needBinaryOpArgParens(arg Expr) bool {
 	}
 }
 
+func isBinaryOpLeafSimple(arg Expr) bool {
+	switch t := arg.(type) {
+	case *NumberExpr:
+		return true
+	case *MetricExpr:
+		metricName := t.getMetricName()
+		return !isReservedBinaryOpIdent(metricName)
+	case *BinaryOpExpr:
+		if t.GroupModifier.Op != "" || t.KeepMetricNames || t.JoinModifier.Op != "" {
+			return false
+		}
+
+		return isBinaryOpLeafSimple(t.Left) && isBinaryOpLeafSimple(t.Right)
+	default:
+		return false
+	}
+}
+
 func isReservedBinaryOpIdent(s string) bool {
 	return isBinaryOpGroupModifier(s) || isBinaryOpJoinModifier(s) || isBinaryOpBoolModifier(s) || isPrefixModifier(s)
 }

parser_test.go

@@ -397,8 +397,8 @@ func TestParseSuccess(t *testing.T) {
 	another(`group_left / (sum(1, 2))`, `group_left / sum(1, 2)`)
 
 	// parensExpr
-	another(`(-foo + ((bar) / (baz))) + ((23))`, `((0 - foo) + (bar / baz)) + 23`)
-	another(`(FOO + ((Bar) / (baZ))) + ((23))`, `(FOO + (Bar / baZ)) + 23`)
+	another(`(-foo + ((bar) / (baz))) + ((23))`, `0 - foo + bar / baz + 23`)
+	another(`(FOO + ((Bar) / (baZ))) + ((23))`, `FOO + Bar / baZ + 23`)
 	same(`(foo, bar)`)
 	another(`((foo, bar),(baz))`, `((foo, bar), baz)`)
 	same(`(foo, (bar, baz), ((x, y), (z, y), xx))`)
@@ -479,16 +479,16 @@ func TestParseSuccess(t *testing.T) {
 	another(`with (foo = bar) baz`, `baz`)
 	another(`with (foo = bar) foo + foo{a="b"}`, `bar + bar{a="b"}`)
 	another(`with (foo = bar, bar=baz + f()) test`, `test`)
-	another(`with (ct={job="test"}) a{ct} + ct() + ceil({ct="x"})`, `(a{job="test"} + {job="test"}) + ceil({ct="x"})`)
+	another(`with (ct={job="test"}) a{ct} + ct() + ceil({ct="x"})`, `a{job="test"} + {job="test"} + ceil({ct="x"})`)
 	another(`with (ct={job="test", i="bar"}) ct + {ct, x="d"} + foo{ct, ct} + count(1)`,
-		`(({job="test",i="bar"} + {job="test",i="bar",x="d"}) + foo{job="test",i="bar"}) + count(1)`)
+		`{job="test",i="bar"} + {job="test",i="bar",x="d"} + foo{job="test",i="bar"} + count(1)`)
 	another(`with (foo = bar) {__name__=~"foo"}`, `{__name__=~"foo"}`)
 	another(`with (foo = bar) foo{__name__="foo"}`, `bar`)
 	another(`with (foo = bar) {__name__="foo", x="y"}`, `bar{x="y"}`)
 	another(`with (foo(bar) = {__name__!="bar"}) foo(x)`, `{__name__!="bar"}`)
 	another(`with (foo(bar) = bar{__name__="bar"}) foo(x)`, `x`)
 	another(`with (foo\-bar(baz) = baz + baz) foo\-bar((x,y))`, `(x, y) + (x, y)`)
-	another(`with (foo\-bar(baz) = baz + baz) foo\-bar(x*y)`, `(x * y) + (x * y)`)
+	another(`with (foo\-bar(baz) = baz + baz) foo\-bar(x*y)`, `x * y + x * y`)
 	another(`with (foo\-bar(baz) = baz + baz) foo\-bar(x\*y)`, `x\*y + x\*y`)
 	another(`with (foo\-bar(b\ az) = b\ az + b\ az) foo\-bar(x\*y)`, `x\*y + x\*y`)
 
@@ -521,7 +521,7 @@ func TestParseSuccess(t *testing.T) {
 	another(`with (ttf = ru(m, n)) ttf`, `(clamp_min(n - clamp_min(m, 0), 0) / clamp_min(n, 0)) * 100`)
 
 	// Verify withExpr recursion and forward reference
-	another(`with (x = x+y, y = x+x) y ^ 2`, `((x + y) + (x + y)) ^ 2`)
+	another(`with (x = x+y, y = x+x) y ^ 2`, `(x + y + x + y) ^ 2`)
 	another(`with (f1(x)=ceil(x), ceil(x)=f1(x)^2) f1(foobar)`, `ceil(foobar)`)
 	another(`with (f1(x)=ceil(x), ceil(x)=f1(x)^2) ceil(foobar)`, `ceil(foobar) ^ 2`)
 
@@ -533,7 +533,7 @@ func TestParseSuccess(t *testing.T) {
 	another(`with (x(a) = sum(a) by (b)) x(xx) / x(y)`, `sum(xx) by(b) / sum(y) by(b)`)
 	another(`with (f(a,f,x)=clamp(x,f,a)) f(f(x,y,z),1,2)`, `clamp(2, 1, clamp(z, y, x))`)
 	another(`with (f(x)=1+sum(x)) f(foo{bar="baz"})`, `1 + sum(foo{bar="baz"})`)
-	another(`with (a=foo, y=bar, f(a)= a+a+y) f(x)`, `(x + x) + bar`)
+	another(`with (a=foo, y=bar, f(a)= a+a+y) f(x)`, `x + x + bar`)
 	another(`with (f(a, b) = m{a, b}) f({a="x", b="y"}, {c="d"})`, `m{a="x",b="y",c="d"}`)
 	another(`with (xx={a="x"}, f(a, b) = m{a, b}) f({xx, b="y"}, {c="d"})`, `m{a="x",b="y",c="d"}`)
 	another(`with (x() = {b="c"}) foo{x}`, `foo{b="c"}`)
@@ -572,7 +572,7 @@ func TestParseSuccess(t *testing.T) {
 	// Verify nested with exprs
 	another(`with (f(x) = (with(x=y) x) + x) f(z)`, `y + z`)
 	another(`with (x=foo) clamp_min(a, with (y=x) y)`, `clamp_min(a, foo)`)
-	another(`with (x=foo) a * x + (with (y=x) y) / y`, `(a * foo) + (foo / y)`)
+	another(`with (x=foo) a * x + (with (y=x) y) / y`, `a * foo + foo / y`)
 	another(`with (x = with (y = foo) y + x) x/x`, `(foo + x) / (foo + x)`)
 	another(`with (
 		x = {foo="bar"},
@@ -583,7 +583,7 @@ func TestParseSuccess(t *testing.T) {
 			)
 			z(foo) / changes(x)
 	)
-	f(a)`, `(a + (foo * m{foo="bar",y="1"})) / changes(a)`)
+	f(a)`, `(a + foo * m{foo="bar",y="1"}) / changes(a)`)
 
 	// complex withExpr
 	another(`WITH (
@@ -601,7 +601,7 @@ func TestParseSuccess(t *testing.T) {
 		f(x, y) = x2(x) + x*y + x2(y)
 	)
 	f(a, 3)
-	`, `((a ^ 2) + (a * 3)) + 9`)
+	`, `a ^ 2 + a * 3 + 9`)
 	another(`WITH (
 		x2(x) = x^2,
 		f(x, y) = x2(x) + x*y + x2(y)
@@ -633,7 +633,7 @@ func TestParseSuccess(t *testing.T) {
 	another(`with (rate(a,b)=a+b) rate(1,2)`, `3`)
 	another(`with (now=now(), sum=sum()) x`, `x`)
 	another(`with (rate(a) = b) c`, `c`)
-	another(`rate(x) + with (rate(a,b)=a*b) rate(2,b)`, `rate(x) + (2 * b)`)
+	another(`rate(x) + with (rate(a,b)=a*b) rate(2,b)`, `rate(x) + 2 * b`)
 	another(`with (sum(a,b)=a+b) sum(c,d)`, `c + d`)
 
 	// $__interval and $__rate_interval must be replaced with 1i

prettifier_test.go

@@ -258,7 +258,7 @@ x + sum(y)`)
 		`WITH (
   x = a{b="c"} + WITH (q = we{rt="z"}) q,
 )
-(abc / x) + WITH (rt = 234 + 234) (2 * rt) + poasdfklkjlkjfdsfjklfdfdsfdsfddfsfd`)
+abc / x + WITH (rt = 234 + 234) 2 * rt + poasdfklkjlkjfdsfjklfdfdsfdsfddfsfd`)
 
 	// duration replacement in WITH expression
 	another(`WITH(BAR=1m,x(BAZ)=sum(rate({a="b"}[BAR:BAZ])) offset BAR) x`, `WITH (BAR = 1m, x(BAZ) = sum(rate({a="b"}[BAR:BAZ])) offset BAR) x`)
@@ -275,4 +275,26 @@ x + sum(y)`)
 	// Verify that exact match __name__ still works
 	another(`{__name__="foo"}`, `foo`)
 	another(`{__name__="foo",bar="baz"}`, `foo{bar="baz"}`)
+
+	same(`1 + 2 + 3 + 4 + 5 + 6 + 7 + 8 + 9 + 10`)
+	same(`(1 + 2) * (3 + 4)`)
+	same(`1 + 2 + foo + bar + baz`)
+
+	another(`a offset 5m + b @ start()`, `(a offset 5m) + (b @ start())`)
+	another(`(1 - (node_memory_MemFree_bytes + node_memory_Cached_bytes + node_memory_Buffers_bytes + node_memory_SReclaimable_bytes) / node_memory_MemTotal_bytes) * 100`,
+		`(
+  1
+    -
+  (
+    node_memory_MemFree_bytes + node_memory_Cached_bytes
+      +
+    node_memory_Buffers_bytes
+      +
+    node_memory_SReclaimable_bytes
+  )
+    /
+  node_memory_MemTotal_bytes
+)
+  *
+100`)
 }

utils_example_test.go

@@ -24,5 +24,5 @@ func ExampleExpandWithExprs() {
 	fmt.Printf("%s\n", pql)
 
 	// Output:
-	// 100 * (disk_free_bytes{job="$job",instance="$instance"} / disk_total_bytes{job="$job",instance="$instance"})
+	// 100 * disk_free_bytes{job="$job",instance="$instance"} / disk_total_bytes{job="$job",instance="$instance"}
 }