From c36961130b08624a2f51a7e0cc2d4abc35864cb4 Mon Sep 17 00:00:00 2001
From: Brian Brazil <brian.brazil@robustperception.io>
Date: Sat, 10 Oct 2015 16:19:14 +0100
Subject: [PATCH] promql: Remove scalar/scalar comparisons.

This change is breaking, use the 'bool' modifier for such comprisons.

After this change all comparisons without 'bool' will filter, and all
comparisons with 'bool' will return 0/1. This makes the language more
consistent and orthogonal, and ultimately easier to learn and use.

If we ever figure out sane semantics for filtering scalar/scalar
comparisons we can add them in, which will most likely come out of how
the new vector() function is used.
---
 promql/lex.go                   | 11 +++++++++++
 promql/parse.go                 | 12 ++++++++----
 promql/parse_test.go            | 27 ++++++++++++++-------------
 promql/testdata/comparison.test |  6 ------
 4 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/promql/lex.go b/promql/lex.go
index 52957b2d9..c2c850f3e 100644
--- a/promql/lex.go
+++ b/promql/lex.go
@@ -60,6 +60,17 @@ func (i itemType) isAggregator() bool { return i > aggregatorsStart && i < aggre
 // Returns false otherwise.
 func (i itemType) isKeyword() bool { return i > keywordsStart && i < keywordsEnd }
 
+// isCompairsonOperator returns true if the item corresponds to a comparison operator.
+// Returns false otherwise.
+func (i itemType) isComparisonOperator() bool {
+	switch i {
+	case itemEQL, itemNEQ, itemLTE, itemLSS, itemGTE, itemGTR:
+		return true
+	default:
+		return false
+	}
+}
+
 // Constants for operator precedence in expressions.
 //
 const LowestPrec = 0 // Non-operators.
diff --git a/promql/parse.go b/promql/parse.go
index 6903c3acc..c15ee6aed 100644
--- a/promql/parse.go
+++ b/promql/parse.go
@@ -488,10 +488,7 @@ func (p *parser) expr() Expr {
 		returnBool := false
 		// Parse bool modifier.
 		if p.peek().typ == itemBool {
-			switch op {
-			case itemEQL, itemNEQ, itemLTE, itemLSS, itemGTE, itemGTR:
-				break
-			default:
+			if !op.isComparisonOperator() {
 				p.errorf("bool modifier can only be used on comparison operators")
 			}
 			p.next()
@@ -540,6 +537,9 @@ func (p *parser) expr() Expr {
 				},
 				VectorMatching: lhs.VectorMatching,
 			}
+			if op.isComparisonOperator() && !returnBool && rhs.Type() == model.ValScalar && lhs.RHS.Type() == model.ValScalar {
+				p.errorf("comparisons between scalars must use BOOL modifier")
+			}
 		} else {
 			expr = &BinaryExpr{
 				Op:             op,
@@ -548,7 +548,11 @@ func (p *parser) expr() Expr {
 				VectorMatching: vecMatching,
 				ReturnBool:     returnBool,
 			}
+			if op.isComparisonOperator() && !returnBool && rhs.Type() == model.ValScalar && expr.Type() == model.ValScalar {
+				p.errorf("comparisons between scalars must use BOOL modifier")
+			}
 		}
+
 	}
 }
 
diff --git a/promql/parse_test.go b/promql/parse_test.go
index ecbc95da8..ab9b78d03 100644
--- a/promql/parse_test.go
+++ b/promql/parse_test.go
@@ -85,23 +85,20 @@ var testExpr = []struct {
 		input:    "1 / 1",
 		expected: &BinaryExpr{itemDIV, &NumberLiteral{1}, &NumberLiteral{1}, nil, false},
 	}, {
-		input:    "1 == 1",
-		expected: &BinaryExpr{itemEQL, &NumberLiteral{1}, &NumberLiteral{1}, nil, false},
+		input:    "1 == bool 1",
+		expected: &BinaryExpr{itemEQL, &NumberLiteral{1}, &NumberLiteral{1}, nil, true},
 	}, {
-		input:    "1 != 1",
-		expected: &BinaryExpr{itemNEQ, &NumberLiteral{1}, &NumberLiteral{1}, nil, false},
+		input:    "1 != bool 1",
+		expected: &BinaryExpr{itemNEQ, &NumberLiteral{1}, &NumberLiteral{1}, nil, true},
 	}, {
-		input:    "1 > 1",
-		expected: &BinaryExpr{itemGTR, &NumberLiteral{1}, &NumberLiteral{1}, nil, false},
+		input:    "1 > bool 1",
+		expected: &BinaryExpr{itemGTR, &NumberLiteral{1}, &NumberLiteral{1}, nil, true},
 	}, {
-		input:    "1 >= 1",
-		expected: &BinaryExpr{itemGTE, &NumberLiteral{1}, &NumberLiteral{1}, nil, false},
+		input:    "1 >= bool 1",
+		expected: &BinaryExpr{itemGTE, &NumberLiteral{1}, &NumberLiteral{1}, nil, true},
 	}, {
-		input:    "1 < 1",
-		expected: &BinaryExpr{itemLSS, &NumberLiteral{1}, &NumberLiteral{1}, nil, false},
-	}, {
-		input:    "1 <= 1",
-		expected: &BinaryExpr{itemLTE, &NumberLiteral{1}, &NumberLiteral{1}, nil, false},
+		input:    "1 < bool 1",
+		expected: &BinaryExpr{itemLSS, &NumberLiteral{1}, &NumberLiteral{1}, nil, true},
 	}, {
 		input:    "1 <= bool 1",
 		expected: &BinaryExpr{itemLTE, &NumberLiteral{1}, &NumberLiteral{1}, nil, true},
@@ -203,6 +200,10 @@ var testExpr = []struct {
 		input:  "1 and 1",
 		fail:   true,
 		errMsg: "AND and OR not allowed in binary scalar expression",
+	}, {
+		input:  "1 == 1",
+		fail:   true,
+		errMsg: "parse error at char 7: comparisons between scalars must use BOOL modifier",
 	}, {
 		input:  "1 or 1",
 		fail:   true,
diff --git a/promql/testdata/comparison.test b/promql/testdata/comparison.test
index 4ce49daa7..012d5891a 100644
--- a/promql/testdata/comparison.test
+++ b/promql/testdata/comparison.test
@@ -40,12 +40,6 @@ eval instant at 50m SUM(http_requests) BY (job) != bool SUM(http_requests) BY (j
 	{job="app-server"} 0
 
 
-eval instant at 50m 0 == 1
-	0
-
-eval instant at 50m 1 == 1
-	1
-
 eval instant at 50m 0 == bool 1
 	0