app/vmselect/promql: recommend to use (a op b) keep_metric_names instead of a op b keep_metric_names

The `a op b keep_metric_names` is ambigouos to `a op (b keep_metric_names)` when `b` is a transform or rollup function.
For example, `a + rate(b) keep_metric_names`. So it is better to use more clear syntax: `(a op b) keep_metric_names`

Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3710

app/vmselect/promql/exec_test.go

@@ -3005,7 +3005,7 @@ func TestExecSuccess(t *testing.T) {
 	})
 	t.Run(`vector / scalar keep_metric_names`, func(t *testing.T) {
 		t.Parallel()
-		q := `sort_desc((label_set(time(), "foo", "bar", "__name__", "q1") or label_set(10, "foo", "qwert", "__name__", "q2")) / 2 keep_metric_names)`
+		q := `sort_desc(((label_set(time(), "foo", "bar", "__name__", "q1") or label_set(10, "foo", "qwert", "__name__", "q2")) / 2) keep_metric_names)`
 		r1 := netstorage.Result{
 			MetricName: storage.MetricName{
 				MetricGroup: []byte("q1"),
@@ -3068,7 +3068,7 @@ func TestExecSuccess(t *testing.T) {
 	})
 	t.Run(`scalar * vector keep_metric_names`, func(t *testing.T) {
 		t.Parallel()
-		q := `sort_desc(2 * (label_set(time(), "foo", "bar", "__name__", "q1") or label_set(10, "foo", "qwert", "__name__", "q2")) keep_metric_names)`
+		q := `sort_desc(2 * (label_set(time(), "foo", "bar", "__name__", "q1"), label_set(10, "foo", "qwert", "__name__", "q2")) keep_metric_names)`
 		r1 := netstorage.Result{
 			MetricName: storage.MetricName{
 				MetricGroup: []byte("q1"),
@@ -3120,7 +3120,7 @@ func TestExecSuccess(t *testing.T) {
 	})
 	t.Run(`scalar * on() group_right vector keep_metric_names`, func(t *testing.T) {
 		t.Parallel()
-		q := `sort_desc(2 * on() group_right() (label_set(time(), "foo", "bar", "__name__", "q1") or label_set(10, "foo", "qwert", "__name__", "q2")) keep_metric_names)`
+		q := `sort_desc(2 * on() group_right() (label_set(time(), "foo", "bar", "__name__", "q1"), label_set(10, "foo", "qwert", "__name__", "q2")) keep_metric_names)`
 		r1 := netstorage.Result{
 			MetricName: storage.MetricName{
 				MetricGroup: []byte("q1"),
@@ -3233,11 +3233,10 @@ func TestExecSuccess(t *testing.T) {
 	})
 	t.Run(`vector * on(foo) scalar keep_metric_names`, func(t *testing.T) {
 		t.Parallel()
-		q := `sort_desc(
-			(
+		q := `((
 		          label_set(time(), "foo", "bar", "xx", "yy", "__name__", "q1"),
 			  label_set(10, "foo", "qwert", "__name__", "q2")
-		        ) * on(foo) (label_set(2, "foo","bar","aa","bb", "__name__", "q2")) keep_metric_names)`
+		      ) * on(foo) label_set(2, "foo","bar","aa","bb", "__name__", "q2")) keep_metric_names`
 		r := netstorage.Result{
 			MetricName: metricNameExpected,
 			Values:     []float64{2000, 2400, 2800, 3200, 3600, 4000},
@@ -3483,11 +3482,11 @@ func TestExecSuccess(t *testing.T) {
 	})
 	t.Run(`vector + vector partial matching keep_metric_names`, func(t *testing.T) {
 		t.Parallel()
-		q := `sort_desc(
-			(label_set(time(), "t1", "v1", "__name__", "q1") or label_set(10, "t2", "v2", "__name__", "q2"))
-			+
-			(label_set(100, "t1", "v1", "__name__", "q3") or label_set(time(), "t2", "v3")) keep_metric_names
-		)`
+		q := `(
+		  (label_set(time(), "t1", "v1", "__name__", "q1") or label_set(10, "t2", "v2", "__name__", "q2"))
+		    +
+		  (label_set(100, "t1", "v1", "__name__", "q3") or label_set(time(), "t2", "v3"))
+		) keep_metric_names`
 		r := netstorage.Result{
 			MetricName: metricNameExpected,
 			Values:     []float64{1100, 1300, 1500, 1700, 1900, 2100},

docs/CHANGELOG.md

@@ -29,7 +29,7 @@ The following tip changes can be tested by building VictoriaMetrics components f
 
 * FEATURE: reduce memory usage by up to 5x for setups with [high churn rate](https://docs.victoriametrics.com/FAQ.html#what-is-high-churn-rate) and long [retention](https://docs.victoriametrics.com/#retention). See [description for this change](https://github.com/VictoriaMetrics/VictoriaMetrics/commit/7094fa38bc207c7bd7330ea8a834310a310ce5e3) for details.
 * FEATURE: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): allow selecting time series matching at least one of multiple `or` filters. For example, `{env="prod",job="a" or env="dev",job="b"}` selects series with either `{env="prod",job="a"}` or `{env="dev",job="b"}` labels. This functionality allows passing the selected series to [rollup functions](https://docs.victoriametrics.com/MetricsQL.html#rollup-functions) without the need to use [subqueries](https://docs.victoriametrics.com/MetricsQL.html#subqueries). See [these docs](https://docs.victoriametrics.com/keyConcepts.html#filtering-by-multiple-or-filters).
-* FEATURE: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): add ability to preserve metric names for binary operation results via `keep_metric_names` modifier. For example, `{__name__=~"foo|bar"} / 10 keep_metric_names` leaves `foo` and `bar` metric names in division results. See [these docs](https://docs.victoriametrics.com/MetricsQL.html#keep_metric_names). This helps to address issues like [this one](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3710).
+* FEATURE: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): add ability to preserve metric names for binary operation results via `keep_metric_names` modifier. For example, `({__name__=~"foo|bar"} / 10) keep_metric_names` leaves `foo` and `bar` metric names in division results. See [these docs](https://docs.victoriametrics.com/MetricsQL.html#keep_metric_names). This helps to address issues like [this one](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3710).
 * FEATURE: [vmctl](https://docs.victoriametrics.com/vmctl.html): add verbose output for docker installations or when TTY isn't available. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4081).
 * FEATURE: [vmctl](https://docs.victoriametrics.com/vmctl.html): interrupt backoff retries when import process is cancelled. The change makes vmctl more responsive in case of errors during the import. See [this pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4442).
 * FEATURE: [vmctl](https://docs.victoriametrics.com/vmctl.html): update backoff policy on retries to reduce probability of overloading for `source` or `destination` databases. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4402).

docs/MetricsQL.md

@@ -120,7 +120,7 @@ This error can be fixed by applying `keep_metric_names` modifier to the function
 
 For example:
 - `rate({__name__=~"foo|bar"}) keep_metric_names` leaves `foo` and `bar` metric names in the returned time series.
-- `{__name__=~"foo|bar"} / 10 keep_metric_names` leaves `foo` and `bar` metric names in the returned time series.
+- `({__name__=~"foo|bar"} / 10) keep_metric_names` leaves `foo` and `bar` metric names in the returned time series.
 
 ## MetricsQL functions
 

go.mod

@@ -12,7 +12,7 @@ require (
 	// like https://github.com/valyala/fasthttp/commit/996610f021ff45fdc98c2ce7884d5fa4e7f9199b
 	github.com/VictoriaMetrics/fasthttp v1.2.0
 	github.com/VictoriaMetrics/metrics v1.24.0
-	github.com/VictoriaMetrics/metricsql v0.58.1
+	github.com/VictoriaMetrics/metricsql v0.58.2
 	github.com/aws/aws-sdk-go-v2 v1.18.1
 	github.com/aws/aws-sdk-go-v2/config v1.18.27
 	github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.11.71

go.sum

@@ -69,8 +69,8 @@ github.com/VictoriaMetrics/fasthttp v1.2.0 h1:nd9Wng4DlNtaI27WlYh5mGXCJOmee/2c2b
 github.com/VictoriaMetrics/fasthttp v1.2.0/go.mod h1:zv5YSmasAoSyv8sBVexfArzFDIGGTN4TfCKAtAw7IfE=
 github.com/VictoriaMetrics/metrics v1.24.0 h1:ILavebReOjYctAGY5QU2F9X0MYvkcrG3aEn2RKa1Zkw=
 github.com/VictoriaMetrics/metrics v1.24.0/go.mod h1:eFT25kvsTidQFHb6U0oa0rTrDRdz4xTYjpL8+UPohys=
-github.com/VictoriaMetrics/metricsql v0.58.1 h1:zg7xN1HptC5oloWvdt4PtgDaBEyMdsgERikUg0hFZB4=
-github.com/VictoriaMetrics/metricsql v0.58.1/go.mod h1:k4UaP/+CjuZslIjd+kCigNG9TQmUqh5v0TP/nMEy90I=
+github.com/VictoriaMetrics/metricsql v0.58.2 h1:xECDKK13ZR7LCpepVM7nuzJTnEkQI3r0qNxQIY6uv60=
+github.com/VictoriaMetrics/metricsql v0.58.2/go.mod h1:k4UaP/+CjuZslIjd+kCigNG9TQmUqh5v0TP/nMEy90I=
 github.com/VividCortex/ewma v1.2.0 h1:f58SaIzcDXrSy3kWaHNvuJgJ3Nmz59Zji6XoJR/q1ow=
 github.com/VividCortex/ewma v1.2.0/go.mod h1:nz4BbCtbLyFDeC9SUHbtcT5644juEuWfUAUnGx7j5l4=
 github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=

vendor/github.com/VictoriaMetrics/metricsql/parser.go

@@ -573,6 +573,14 @@ func (p *parser) parseParensExpr() (*parensExpr, error) {
 	if err := p.lex.Next(); err != nil {
 		return nil, err
 	}
+	if len(exprs) == 1 {
+		if be, ok := exprs[0].(*BinaryOpExpr); ok && isKeepMetricNames(p.lex.Token) {
+			if err := p.lex.Next(); err != nil {
+				return nil, err
+			}
+			be.KeepMetricNames = true
+		}
+	}
 	pe := parensExpr(exprs)
 	return &pe, nil
 }
@@ -1632,7 +1640,18 @@ type BinaryOpExpr struct {
 
 // AppendString appends string representation of be to dst and returns the result.
 func (be *BinaryOpExpr) AppendString(dst []byte) []byte {
-	if _, ok := be.Left.(*BinaryOpExpr); ok {
+	if be.KeepMetricNames {
+		dst = append(dst, '(')
+		dst = be.appendStringNoKeepMetricNames(dst)
+		dst = append(dst, ") keep_metric_names"...)
+	} else {
+		dst = be.appendStringNoKeepMetricNames(dst)
+	}
+	return dst
+}
+
+func (be *BinaryOpExpr) appendStringNoKeepMetricNames(dst []byte) []byte {
+	if be.needLeftParens() {
 		dst = appendArgInParens(dst, be.Left)
 	} else {
 		dst = be.Left.AppendString(dst)
@@ -1656,30 +1675,40 @@ func (be *BinaryOpExpr) AppendString(dst []byte) []byte {
 	} else {
 		dst = be.Right.AppendString(dst)
 	}
-	if be.KeepMetricNames {
-		dst = append(dst, " keep_metric_names"...)
-	}
 	return dst
 }
 
+func (be *BinaryOpExpr) needLeftParens() bool {
+	return needBinaryOpArgParens(be.Left)
+}
+
 func (be *BinaryOpExpr) needRightParens() bool {
-	r := be.Right
-	if _, ok := r.(*BinaryOpExpr); ok {
+	if needBinaryOpArgParens(be.Right) {
 		return true
 	}
-	if me, ok := r.(*MetricExpr); ok {
-		metricName := me.getMetricName()
+	switch t := be.Right.(type) {
+	case *MetricExpr:
+		metricName := t.getMetricName()
 		return isReservedBinaryOpIdent(metricName)
-	}
-	if fe, ok := r.(*FuncExpr); ok && isReservedBinaryOpIdent(fe.Name) {
-		return true
-	}
-	if !be.KeepMetricNames {
+	case *FuncExpr:
+		if isReservedBinaryOpIdent(t.Name) {
+			return true
+		}
+		return be.KeepMetricNames
+	default:
 		return false
 	}
-	switch r.(type) {
-	case *FuncExpr:
+}
+
+func needBinaryOpArgParens(arg Expr) bool {
+	switch t := arg.(type) {
+	case *BinaryOpExpr:
 		return true
+	case *RollupExpr:
+		if be, ok := t.Expr.(*BinaryOpExpr); ok && be.KeepMetricNames {
+			return true
+		}
+		return t.Offset != nil || t.At != nil
 	default:
 		return false
 	}
@@ -1708,7 +1737,7 @@ type ModifierExpr struct {
 // AppendString appends string representation of me to dst and returns the result.
 func (me *ModifierExpr) AppendString(dst []byte) []byte {
 	dst = append(dst, me.Op...)
-	dst = append(dst, "("...)
+	dst = append(dst, '(')
 	for i, arg := range me.Args {
 		dst = appendEscapedIdent(dst, arg)
 		if i+1 < len(me.Args) {

vendor/modules.txt

@@ -99,7 +99,7 @@ github.com/VictoriaMetrics/fasthttp/stackless
 # github.com/VictoriaMetrics/metrics v1.24.0
 ## explicit; go 1.20
 github.com/VictoriaMetrics/metrics
-# github.com/VictoriaMetrics/metricsql v0.58.1
+# github.com/VictoriaMetrics/metricsql v0.58.2
 ## explicit; go 1.13
 github.com/VictoriaMetrics/metricsql
 github.com/VictoriaMetrics/metricsql/binaryop