preserve parens if op prioiry not the same

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"}`)

parser.go

@@ -1977,13 +1977,9 @@ func (be *BinaryOpExpr) appendModifiers(dst []byte) []byte {
 func needBinaryOpArgParens(arg Expr, parentOp string) bool {
 	switch t := arg.(type) {
 	case *BinaryOpExpr:
-		// 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 != "/" {
+		// Parens are required when the child op priority not equal to parent o one.
+		// For example, a + b / c - d should be a + (b / c) - d.
+		if binaryOpPriority(t.Op) != binaryOpPriority(parentOp) {
 			return true
 		}
 
@@ -2005,6 +2001,8 @@ func isBinaryOpLeafSimple(arg Expr) bool {
 		return true
 	case *MetricExpr:
 		metricName := t.getMetricName()
+		// Parens should be added if metric name equals to a reserved word, such as group_left
+		// For example, a + group_left should become a + (group_left). Otherwise, query won't be parsed.
 		return !isReservedBinaryOpIdent(metricName)
 	case *BinaryOpExpr:
 		if t.GroupModifier.Op != "" || t.KeepMetricNames || t.JoinModifier.Op != "" {

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))`)
@@ -488,7 +488,7 @@ func TestParseSuccess(t *testing.T) {
 	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`)
 
@@ -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`)
@@ -284,19 +284,44 @@ abc / x + WITH (rt = 234 + 234) 2 * rt + poasdfklkjlkjfdsfjklfdfdsfdsfddfsfd`)
 
 	// see https://github.com/VictoriaMetrics/metricsql/issues/54
 	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_MemFree_bytes + node_memory_Cached_bytes
+        +
+      node_memory_Buffers_bytes
+        +
+      node_memory_SReclaimable_bytes
+    )
+      /
+    node_memory_MemTotal_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
+`
 }