Remove keep_common modifier.

See #3060

promql/ast.go

@@ -99,12 +99,11 @@ type Expressions []Expr
 
 // AggregateExpr represents an aggregation operation on a Vector.
 type AggregateExpr struct {
-	Op               itemType // The used aggregation operation.
-	Expr             Expr     // The Vector expression over which is aggregated.
-	Param            Expr     // Parameter used by some aggregators.
-	Grouping         []string // The labels by which to group the Vector.
-	Without          bool     // Whether to drop the given labels rather than keep them.
-	KeepCommonLabels bool     // Whether to keep common labels among result elements.
+	Op       itemType // The used aggregation operation.
+	Expr     Expr     // The Vector expression over which is aggregated.
+	Param    Expr     // Parameter used by some aggregators.
+	Grouping []string // The labels by which to group the Vector.
+	Without  bool     // Whether to drop the given labels rather than keep them.
 }
 
 // BinaryExpr represents a binary expression between two child expressions.

promql/engine.go

@@ -679,7 +679,7 @@ func (ev *evaluator) eval(expr Expr) Value {
 	switch e := expr.(type) {
 	case *AggregateExpr:
 		Vector := ev.evalVector(e.Expr)
-		return ev.aggregation(e.Op, e.Grouping, e.Without, e.KeepCommonLabels, e.Param, Vector)
+		return ev.aggregation(e.Op, e.Grouping, e.Without, e.Param, Vector)
 
 	case *BinaryExpr:
 		lhs := ev.evalOneOf(e.LHS, ValueTypeScalar, ValueTypeVector)
@@ -1234,7 +1234,7 @@ type groupedAggregation struct {
 }
 
 // aggregation evaluates an aggregation operation on a Vector.
-func (ev *evaluator) aggregation(op itemType, grouping []string, without bool, keepCommon bool, param Expr, vec Vector) Vector {
+func (ev *evaluator) aggregation(op itemType, grouping []string, without bool, param Expr, vec Vector) Vector {
 
 	result := map[uint64]*groupedAggregation{}
 	var k int64
@@ -1282,9 +1282,7 @@ func (ev *evaluator) aggregation(op itemType, grouping []string, without bool, k
 		if !ok {
 			var m labels.Labels
 
-			if keepCommon {
-				m = lb.Del(labels.MetricName).Labels()
-			} else if without {
+			if without {
 				m = metric
 			} else {
 				m = make(labels.Labels, 0, len(grouping))
@@ -1319,10 +1317,6 @@ func (ev *evaluator) aggregation(op itemType, grouping []string, without bool, k
 			}
 			continue
 		}
-		// Add the sample to the existing group.
-		if keepCommon {
-			group.labels = intersection(group.labels, s.Metric)
-		}
 
 		switch op {
 		case itemSum:

promql/lex.go

@@ -187,7 +187,6 @@ const (
 	itemFor
 	itemLabels
 	itemAnnotations
-	itemKeepCommon
 	itemOffset
 	itemBy
 	itemWithout
@@ -227,7 +226,6 @@ var key = map[string]itemType{
 	"offset":      itemOffset,
 	"by":          itemBy,
 	"without":     itemWithout,
-	"keep_common": itemKeepCommon,
 	"on":          itemOn,
 	"ignoring":    itemIgnoring,
 	"group_left":  itemGroupLeft,

promql/lex_test.go

@@ -251,9 +251,6 @@ var tests = []struct {
 	{
 		input:    "alert",
 		expected: []item{{itemAlert, 0, "alert"}},
-	}, {
-		input:    "keep_common",
-		expected: []item{{itemKeepCommon, 0, "keep_common"}},
 	}, {
 		input:    "if",
 		expected: []item{{itemIf, 0, "if"}},

promql/parse.go

@@ -703,8 +703,8 @@ func (p *parser) labels() []string {
 
 // aggrExpr parses an aggregation expression.
 //
-//		<aggr_op> (<Vector_expr>) [by <labels>] [keep_common]
-//		<aggr_op> [by <labels>] [keep_common] (<Vector_expr>)
+//		<aggr_op> (<Vector_expr>) [by|without <labels>]
+//		<aggr_op> [by|without <labels>] (<Vector_expr>)
 //
 func (p *parser) aggrExpr() *AggregateExpr {
 	const ctx = "aggregation"
@@ -714,7 +714,7 @@ func (p *parser) aggrExpr() *AggregateExpr {
 		p.errorf("expected aggregation operator but got %s", agop)
 	}
 	var grouping []string
-	var keepCommon, without bool
+	var without bool
 
 	modifiersFirst := false
 
@@ -726,11 +726,6 @@ func (p *parser) aggrExpr() *AggregateExpr {
 		grouping = p.labels()
 		modifiersFirst = true
 	}
-	if p.peek().typ == itemKeepCommon {
-		p.next()
-		keepCommon = true
-		modifiersFirst = true
-	}
 
 	p.expect(itemLeftParen, ctx)
 	var param Expr
@@ -752,23 +747,14 @@ func (p *parser) aggrExpr() *AggregateExpr {
 			p.next()
 			grouping = p.labels()
 		}
-		if p.peek().typ == itemKeepCommon {
-			p.next()
-			keepCommon = true
-		}
-	}
-
-	if keepCommon && without {
-		p.errorf("cannot use 'keep_common' with 'without'")
 	}
 
 	return &AggregateExpr{
-		Op:               agop.typ,
-		Expr:             e,
-		Param:            param,
-		Grouping:         grouping,
-		Without:          without,
-		KeepCommonLabels: keepCommon,
+		Op:       agop.typ,
+		Expr:     e,
+		Param:    param,
+		Grouping: grouping,
+		Without:  without,
 	}
 }
 

promql/parse_test.go

@@ -1068,32 +1068,6 @@ var testExpr = []struct {
 			},
 			Grouping: []string{"foo"},
 		},
-	}, {
-		input: "sum by (foo) keep_common (some_metric)",
-		expected: &AggregateExpr{
-			Op:               itemSum,
-			KeepCommonLabels: true,
-			Expr: &VectorSelector{
-				Name: "some_metric",
-				LabelMatchers: []*labels.Matcher{
-					mustLabelMatcher(labels.MatchEqual, string(model.MetricNameLabel), "some_metric"),
-				},
-			},
-			Grouping: []string{"foo"},
-		},
-	}, {
-		input: "sum (some_metric) by (foo,bar) keep_common",
-		expected: &AggregateExpr{
-			Op:               itemSum,
-			KeepCommonLabels: true,
-			Expr: &VectorSelector{
-				Name: "some_metric",
-				LabelMatchers: []*labels.Matcher{
-					mustLabelMatcher(labels.MatchEqual, string(model.MetricNameLabel), "some_metric"),
-				},
-			},
-			Grouping: []string{"foo", "bar"},
-		},
 	}, {
 		input: "avg by (foo)(some_metric)",
 		expected: &AggregateExpr{
@@ -1106,32 +1080,6 @@ var testExpr = []struct {
 			},
 			Grouping: []string{"foo"},
 		},
-	}, {
-		input: "COUNT by (foo) keep_common (some_metric)",
-		expected: &AggregateExpr{
-			Op: itemCount,
-			Expr: &VectorSelector{
-				Name: "some_metric",
-				LabelMatchers: []*labels.Matcher{
-					mustLabelMatcher(labels.MatchEqual, string(model.MetricNameLabel), "some_metric"),
-				},
-			},
-			Grouping:         []string{"foo"},
-			KeepCommonLabels: true,
-		},
-	}, {
-		input: "MIN (some_metric) by (foo) keep_common",
-		expected: &AggregateExpr{
-			Op: itemMin,
-			Expr: &VectorSelector{
-				Name: "some_metric",
-				LabelMatchers: []*labels.Matcher{
-					mustLabelMatcher(labels.MatchEqual, string(model.MetricNameLabel), "some_metric"),
-				},
-			},
-			Grouping:         []string{"foo"},
-			KeepCommonLabels: true,
-		},
 	}, {
 		input: "max by (foo)(some_metric)",
 		expected: &AggregateExpr{
@@ -1264,17 +1212,13 @@ var testExpr = []struct {
 		fail:   true,
 		errMsg: "no valid expression found",
 	}, {
-		input:  "MIN keep_common (some_metric) by (foo)",
+		input:  "MIN keep_common (some_metric)",
 		fail:   true,
-		errMsg: "could not parse remaining input \"by (foo)\"...",
+		errMsg: "parse error at char 5: unexpected identifier \"keep_common\" in aggregation, expected \"(\"",
 	}, {
-		input:  "MIN by(test) (some_metric) keep_common",
+		input:  "MIN (some_metric) keep_common",
 		fail:   true,
 		errMsg: "could not parse remaining input \"keep_common\"...",
-	}, {
-		input:  `sum (some_metric) without (test) keep_common`,
-		fail:   true,
-		errMsg: "cannot use 'keep_common' with 'without'",
 	}, {
 		input:  `sum (some_metric) without (test) by (test)`,
 		fail:   true,

promql/printer.go

@@ -148,9 +148,6 @@ func (node *AggregateExpr) String() string {
 		}
 		aggrString = fmt.Sprintf(format, aggrString, strings.Join(node.Grouping, ", "))
 	}
-	if node.KeepCommonLabels {
-		aggrString += " KEEP_COMMON"
-	}
 	return aggrString
 }
 

promql/printer_test.go

@@ -63,12 +63,6 @@ func TestExprString(t *testing.T) {
 		{
 			in: `sum(task:errors:rate10s{job="s"}) BY (code)`,
 		},
-		{
-			in: `sum(task:errors:rate10s{job="s"}) BY (code) KEEP_COMMON`,
-		},
-		{
-			in: `sum(task:errors:rate10s{job="s"}) KEEP_COMMON`,
-		},
 		{
 			in: `sum(task:errors:rate10s{job="s"}) WITHOUT (instance)`,
 		},

promql/testdata/aggregators.test

@@ -42,7 +42,7 @@ eval instant at 50m sum by () (http_requests{job="api-server"})
   {} 1000
 
 # No by/without.
-eval instant at 50m sum (http_requests{job="api-server"})
+eval instant at 50m sum(http_requests{job="api-server"})
   {} 1000
 
 # Empty without.
@@ -62,16 +62,16 @@ eval instant at 50m sum(http_requests) by (job) + min(http_requests) by (job) +
   {job="app-server"} 4550
   {job="api-server"} 1750
 
-# Test alternative "by"-clause order with "keep_common".
-eval instant at 50m sum by (group) keep_common (http_requests{job="api-server"})
-  {group="canary", job="api-server"} 700
-  {group="production", job="api-server"} 300
+# Test alternative "by"-clause order.
+eval instant at 50m sum by (group) (http_requests{job="api-server"})
+  {group="canary"} 700
+  {group="production"} 300
 
 # Test both alternative "by"-clause orders in one expression.
 # Public health warning: stick to one form within an expression (or even
 # in an organization), or risk serious user confusion.
-eval instant at 50m sum(sum by (group) keep_common (http_requests{job="api-server"})) by (job)
-  {job="api-server"} 1000
+eval instant at 50m sum(sum by (group) (http_requests{job="api-server"})) by (job)
+  {} 1000
 
 
 

promql/testdata/legacy.test

@@ -45,10 +45,6 @@ eval instant at 50m SUM(http_requests{instance="0"}) BY(job)
 	{job="api-server"} 400
 	{job="app-server"} 1200
 
-eval instant at 50m SUM(http_requests{instance="0"}) BY(job) KEEP_COMMON
-	{instance="0", job="api-server"} 400
-	{instance="0", job="app-server"} 1200
-
 eval instant at 50m SUM(http_requests) BY (job)
 	{job="api-server"} 1000
 	{job="app-server"} 2600
@@ -353,17 +349,6 @@ eval instant at 50m log10(vector_matching_a - 20)
 
 
 # Matrix tests.
-clear
-load 1h
-	testmetric{testlabel="1"} 1 1
-	testmetric{testlabel="2"} 2 _
-
-eval instant at 0h sum(testmetric) keep_common
-	{} 3
-
-eval instant at 1h sum(testmetric) keep_common
-	{testlabel="1"} 1
-
 clear
 load 1h
 	testmetric{aa="bb"} 1