From 411ca4dba1dcf51df47755a2769b96c8a7b70518 Mon Sep 17 00:00:00 2001
From: Tobias Schmidt <ts@soundcloud.com>
Date: Sun, 24 Jan 2016 22:50:46 -0500
Subject: [PATCH] Consolidate offset modifier parsing

Remove duplicated offset modifier parsing and ensure offset can only
appear at the end of a selector statement.
---
 promql/parse.go      | 68 +++++++++++++++++++++++++-------------------
 promql/parse_test.go | 16 +++++------
 2 files changed, 46 insertions(+), 38 deletions(-)

diff --git a/promql/parse.go b/promql/parse.go
index 55a1b10b2..7ab41d109 100644
--- a/promql/parse.go
+++ b/promql/parse.go
@@ -598,6 +598,21 @@ func (p *parser) unaryExpr() Expr {
 		}
 		e = p.rangeSelector(vs)
 	}
+
+	// Parse optional offset.
+	if p.peek().typ == itemOffset {
+		offset := p.offset()
+
+		switch s := e.(type) {
+		case *VectorSelector:
+			s.Offset = offset
+		case *MatrixSelector:
+			s.Offset = offset
+		default:
+			p.errorf("offset modifier must be preceded by a metric or range selector, but follows a %T instead", e)
+		}
+	}
+
 	return e
 }
 
@@ -609,7 +624,7 @@ func (p *parser) rangeSelector(vs *VectorSelector) *MatrixSelector {
 	const ctx = "matrix selector"
 	p.next()
 
-	var erange, offset time.Duration
+	var erange time.Duration
 	var err error
 
 	erangeStr := p.expect(itemDuration, ctx).val
@@ -620,27 +635,15 @@ func (p *parser) rangeSelector(vs *VectorSelector) *MatrixSelector {
 
 	p.expect(itemRightBracket, ctx)
 
-	// Parse optional offset.
-	if p.peek().typ == itemOffset {
-		p.next()
-		offi := p.expect(itemDuration, ctx)
-
-		offset, err = parseDuration(offi.val)
-		if err != nil {
-			p.error(err)
-		}
-	}
-
 	e := &MatrixSelector{
 		Name:          vs.Name,
 		LabelMatchers: vs.LabelMatchers,
 		Range:         erange,
-		Offset:        offset,
 	}
 	return e
 }
 
-// parseNumber parses a number.
+// number parses a number.
 func (p *parser) number(val string) float64 {
 	n, err := strconv.ParseInt(val, 0, 64)
 	f := float64(n)
@@ -927,10 +930,28 @@ func (p *parser) metric() model.Metric {
 	return m
 }
 
-// metricSelector parses a new metric selector.
+// offset parses an offset modifier.
 //
-//		<metric_identifier> [<label_matchers>] [ offset <duration> ]
-//		[<metric_identifier>] <label_matchers> [ offset <duration> ]
+//		offset <duration>
+//
+func (p *parser) offset() time.Duration {
+	const ctx = "offset"
+
+	p.next()
+	offi := p.expect(itemDuration, ctx)
+
+	offset, err := parseDuration(offi.val)
+	if err != nil {
+		p.error(err)
+	}
+
+	return offset
+}
+
+// vectorSelector parses a new vector selector.
+//
+//		<metric_identifier> [<label_matchers>]
+//		[<metric_identifier>] <label_matchers>
 //
 func (p *parser) vectorSelector(name string) *VectorSelector {
 	const ctx = "metric selector"
@@ -978,22 +999,9 @@ func (p *parser) vectorSelector(name string) *VectorSelector {
 		p.errorf("vector selector must contain at least one non-empty matcher")
 	}
 
-	var err error
-	var offset time.Duration
-	// Parse optional offset.
-	if p.peek().typ == itemOffset {
-		p.next()
-		offi := p.expect(itemDuration, ctx)
-
-		offset, err = parseDuration(offi.val)
-		if err != nil {
-			p.error(err)
-		}
-	}
 	return &VectorSelector{
 		Name:          name,
 		LabelMatchers: matchers,
-		Offset:        offset,
 	}
 }
 
diff --git a/promql/parse_test.go b/promql/parse_test.go
index 52a3381e1..020886b3e 100644
--- a/promql/parse_test.go
+++ b/promql/parse_test.go
@@ -758,7 +758,7 @@ var testExpr = []struct {
 	}, {
 		input:  `some_metric[5m] OFFSET 1`,
 		fail:   true,
-		errMsg: "unexpected number \"1\" in matrix selector, expected duration",
+		errMsg: "unexpected number \"1\" in offset, expected duration",
 	}, {
 		input:  `some_metric[5m] OFFSET 1mm`,
 		fail:   true,
@@ -766,7 +766,11 @@ var testExpr = []struct {
 	}, {
 		input:  `some_metric[5m] OFFSET`,
 		fail:   true,
-		errMsg: "unexpected end of input in matrix selector, expected duration",
+		errMsg: "unexpected end of input in offset, expected duration",
+	}, {
+		input:  `some_metric OFFSET 1m[5m]`,
+		fail:   true,
+		errMsg: "could not parse remaining input \"[5m]\"...",
 	}, {
 		input:  `(foo + bar)[5m]`,
 		fail:   true,
@@ -900,10 +904,6 @@ var testExpr = []struct {
 		input:  `sum (some_metric) by test`,
 		fail:   true,
 		errMsg: "unexpected identifier \"test\" in grouping opts, expected \"(\"",
-	}, {
-		input:  `some_metric[5m] OFFSET`,
-		fail:   true,
-		errMsg: "unexpected end of input in matrix selector, expected duration",
 	}, {
 		input:  `sum () by (test)`,
 		fail:   true,
@@ -1147,7 +1147,7 @@ var testStatement = []struct {
 		input: `
 			# A simple test recording rule.
 			dc:http_request:rate5m = sum(rate(http_request_count[5m])) by (dc)
-	
+
 			# A simple test alerting rule.
 			ALERT GlobalRequestRateLow IF(dc:http_request:rate5m < 10000) FOR 5m
 			  LABELS {
@@ -1315,7 +1315,7 @@ var testStatement = []struct {
 			ALERT GlobalRequestRateLow IF(dc:http_request:rate5m < 10000) FOR 5
 			  LABELS {
 			    service = "testservice"
-			    # ... more fields here ... 
+			    # ... more fields here ...
 			  }
 			  ANNOTATIONS {
 			    summary = "Global request rate low"