From 73ae88924423c2787c76a206bb8fd13268cc35c9 Mon Sep 17 00:00:00 2001
From: Aliaksandr Valialkin <valyala@gmail.com>
Date: Tue, 9 Jul 2019 23:20:38 +0300
Subject: [PATCH] app/vmselect/promql: extract `rmoeveGroupTags` function for
 removing unneeded tags from MetricName according to the given modifierExpr

---
 app/vmselect/promql/aggr.go      | 36 ++++++++++++++------------------
 app/vmselect/promql/exec_test.go | 18 ++++++++++++++++
 2 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/app/vmselect/promql/aggr.go b/app/vmselect/promql/aggr.go
index 2732fb029b..51eaec99a1 100644
--- a/app/vmselect/promql/aggr.go
+++ b/app/vmselect/promql/aggr.go
@@ -6,6 +6,9 @@ import (
 	"sort"
 	"strconv"
 	"strings"
+
+	"github.com/VictoriaMetrics/VictoriaMetrics/lib/logger"
+	"github.com/VictoriaMetrics/VictoriaMetrics/lib/storage"
 )
 
 var aggrFuncs = map[string]aggrFunc{
@@ -67,33 +70,26 @@ func newAggrFunc(afe func(tss []*timeseries) []*timeseries) aggrFunc {
 	}
 }
 
+func removeGroupTags(metricName *storage.MetricName, modifier *modifierExpr) {
+	groupOp := strings.ToLower(modifier.Op)
+	switch groupOp {
+	case "", "by":
+		metricName.RemoveTagsOn(modifier.Args)
+	case "without":
+		metricName.RemoveTagsIgnoring(modifier.Args)
+	default:
+		logger.Panicf("BUG: unknown group modifier: %q", groupOp)
+	}
+}
+
 func aggrFuncExt(afe func(tss []*timeseries) []*timeseries, argOrig []*timeseries, modifier *modifierExpr, keepOriginal bool) ([]*timeseries, error) {
 	arg := copyTimeseriesMetricNames(argOrig)
 
-	// Filter out superflouos tags.
-	var groupTags []string
-	groupOp := "by"
-	if modifier.Op != "" {
-		groupTags = modifier.Args
-		groupOp = strings.ToLower(modifier.Op)
-	}
-	switch groupOp {
-	case "by":
-		for _, ts := range arg {
-			ts.MetricName.RemoveTagsOn(groupTags)
-		}
-	case "without":
-		for _, ts := range arg {
-			ts.MetricName.RemoveTagsIgnoring(groupTags)
-		}
-	default:
-		return nil, fmt.Errorf(`unknown modifier: %q`, groupOp)
-	}
-
 	// Perform grouping.
 	m := make(map[string][]*timeseries)
 	bb := bbPool.Get()
 	for i, ts := range arg {
+		removeGroupTags(&ts.MetricName, modifier)
 		bb.B = marshalMetricNameSorted(bb.B[:0], &ts.MetricName)
 		if keepOriginal {
 			ts = argOrig[i]
diff --git a/app/vmselect/promql/exec_test.go b/app/vmselect/promql/exec_test.go
index e442a5cb0e..d81f8fc849 100644
--- a/app/vmselect/promql/exec_test.go
+++ b/app/vmselect/promql/exec_test.go
@@ -2218,6 +2218,17 @@ func TestExecSuccess(t *testing.T) {
 		resultExpected := []netstorage.Result{r}
 		f(q, resultExpected)
 	})
+	t.Run(`avg(scalar) wiTHout (xx, yy)`, func(t *testing.T) {
+		t.Parallel()
+		q := `avg wiTHout (xx, yy) (123)`
+		r := netstorage.Result{
+			MetricName: metricNameExpected,
+			Values:     []float64{123, 123, 123, 123, 123, 123},
+			Timestamps: timestampsExpected,
+		}
+		resultExpected := []netstorage.Result{r}
+		f(q, resultExpected)
+	})
 	t.Run(`sum(time)`, func(t *testing.T) {
 		t.Parallel()
 		q := `sum(time()/100)`
@@ -3868,6 +3879,13 @@ func TestExecError(t *testing.T) {
 		label_set(time()+200, "__name__", "bar", "a", "x"),
 	) + 10`)
 
+	// Invalid aggregates
+	f(`sum(1, 2)`)
+	f(`sum(1) foo (bar)`)
+	f(`sum foo () (bar)`)
+	f(`sum(foo) by (1)`)
+	f(`count(foo) without ("bar")`)
+
 	// With expressions
 	f(`ttf()`)
 	f(`ttf(1, 2)`)