Merge pull request #2034 from influxdb/groupby-requires-aggregate

Group by should require an aggregate

CHANGELOG.md

@@ -1,8 +1,9 @@
 ## v0.9.0-rc16 [unreleased]
 
-## Bugfixes
-- [#2037](https://github.com/influxdb/influxdb/pull/2037) Don't check 'configExists' at Run() level
-- [#2039](https://github.com/influxdb/influxdb/pull/2039) Don't panic if getting current use fails
+### Bugfixes
+- [#2037](https://github.com/influxdb/influxdb/pull/2037): Don't check 'configExists' at Run() level
+- [#2039](https://github.com/influxdb/influxdb/pull/2039): Don't panic if getting current use fails
+- [#2034](https://github.com/influxdb/influxdb/pull/2034): Group by should require an aggregate.
 
 ## v0.9.0-rc15 [2015-03-19]
 

httpd/handler.go

@@ -217,7 +217,7 @@ type Batch struct {
 // Return all the measurements from the given DB
 func (h *Handler) showMeasurements(db string, user *influxdb.User) ([]string, error) {
 	var measurements []string
-	results := h.server.ExecuteQuery(&influxql.Query{[]influxql.Statement{&influxql.ShowMeasurementsStatement{}}}, db, user)
+	results := h.server.ExecuteQuery(&influxql.Query{Statements: []influxql.Statement{&influxql.ShowMeasurementsStatement{}}}, db, user)
 	if results.Err != nil {
 		return measurements, results.Err
 	}

influxql/ast_test.go

@@ -258,7 +258,7 @@ func TestSelectStatement_HasWildcard(t *testing.T) {
 
 		// No GROUP BY wildcards, time only
 		{
-			stmt:     `SELECT value FROM cpu GROUP BY time(5ms)`,
+			stmt:     `SELECT mean(value) FROM cpu GROUP BY time(5ms)`,
 			wildcard: false,
 		},
 
@@ -270,7 +270,7 @@ func TestSelectStatement_HasWildcard(t *testing.T) {
 
 		// GROUP BY wildcard with time
 		{
-			stmt:     `SELECT value FROM cpu GROUP BY *,time(1m)`,
+			stmt:     `SELECT mean(value) FROM cpu GROUP BY *,time(1m)`,
 			wildcard: true,
 		},
 
@@ -357,8 +357,8 @@ func TestSelectStatement_RewriteWildcards(t *testing.T) {
 
 		// No GROUP BY wildcards, time only
 		{
-			stmt:    `SELECT value FROM cpu GROUP BY time(5ms)`,
-			rewrite: `SELECT value FROM cpu GROUP BY time(5ms)`,
+			stmt:    `SELECT mean(value) FROM cpu GROUP BY time(5ms)`,
+			rewrite: `SELECT mean(value) FROM cpu GROUP BY time(5ms)`,
 		},
 
 		// GROUP BY wildcard
@@ -369,14 +369,14 @@ func TestSelectStatement_RewriteWildcards(t *testing.T) {
 
 		// GROUP BY wildcard with time
 		{
-			stmt:    `SELECT value FROM cpu GROUP BY *,time(1m)`,
-			rewrite: `SELECT value FROM cpu GROUP BY host, region, time(1m)`,
+			stmt:    `SELECT mean(value) FROM cpu GROUP BY *,time(1m)`,
+			rewrite: `SELECT mean(value) FROM cpu GROUP BY host, region, time(1m)`,
 		},
 
 		// GROUP BY wildarde with fill
 		{
-			stmt:    `SELECT value FROM cpu GROUP BY *,time(1m) fill(0)`,
-			rewrite: `SELECT value FROM cpu GROUP BY host, region, time(1m) fill(0)`,
+			stmt:    `SELECT mean(value) FROM cpu GROUP BY *,time(1m) fill(0)`,
+			rewrite: `SELECT mean(value) FROM cpu GROUP BY host, region, time(1m) fill(0)`,
 		},
 
 		// GROUP BY wildcard with explicit

influxql/engine.go

@@ -98,7 +98,7 @@ func (m *MapReduceJob) Execute(out chan *Row, filterEmptyResults bool) {
 		pointCountInResult = 1
 	} else {
 		intervalTop := m.TMax/m.interval*m.interval + m.interval
-		intervalBottom := m.TMin/m.interval*m.interval
+		intervalBottom := m.TMin / m.interval * m.interval
 		pointCountInResult = int((intervalTop - intervalBottom) / m.interval)
 	}
 

influxql/parser.go

@@ -585,6 +585,10 @@ func (p *Parser) parseSelectStatement(tr targetRequirement) (*SelectStatement, e
 		}
 	})
 
+	if d, _ := stmt.GroupByInterval(); stmt.IsRawQuery && d > 0 {
+		return nil, fmt.Errorf("GROUP BY requires at least one aggregate function")
+	}
+
 	return stmt, nil
 }
 

influxql/parser_test.go

@@ -71,13 +71,13 @@ func TestParser_ParseStatement(t *testing.T) {
 
 		// SELECT statement
 		{
-			s: `SELECT field1, field2 ,field3 AS field_x FROM myseries WHERE host = 'hosta.influxdb.org' GROUP BY time(10h) ORDER BY ASC LIMIT 20 OFFSET 10;`,
+			s: `SELECT mean(field1), sum(field2) ,count(field3) AS field_x FROM myseries WHERE host = 'hosta.influxdb.org' GROUP BY time(10h) ORDER BY ASC LIMIT 20 OFFSET 10;`,
 			stmt: &influxql.SelectStatement{
-				IsRawQuery: true,
+				IsRawQuery: false,
 				Fields: []*influxql.Field{
-					{Expr: &influxql.VarRef{Val: "field1"}},
-					{Expr: &influxql.VarRef{Val: "field2"}},
-					{Expr: &influxql.VarRef{Val: "field3"}, Alias: "field_x"},
+					{Expr: &influxql.Call{Name: "mean", Args: []influxql.Expr{&influxql.VarRef{Val: "field1"}}}},
+					{Expr: &influxql.Call{Name: "sum", Args: []influxql.Expr{&influxql.VarRef{Val: "field2"}}}},
+					{Expr: &influxql.Call{Name: "count", Args: []influxql.Expr{&influxql.VarRef{Val: "field3"}}}, Alias: "field_x"},
 				},
 				Sources: []influxql.Source{&influxql.Measurement{Name: "myseries"}},
 				Condition: &influxql.BinaryExpr{
@@ -787,6 +787,7 @@ func TestParser_ParseStatement(t *testing.T) {
 		{s: `SELECT field1 FROM myseries ORDER BY 1`, err: `found 1, expected identifier, ASC, or DESC at line 1, char 38`},
 		{s: `SELECT field1 AS`, err: `found EOF, expected identifier at line 1, char 18`},
 		{s: `SELECT field1 FROM 12`, err: `found 12, expected identifier, regex at line 1, char 20`},
+		{s: `SELECT field1 FROM foo group by time(1s)`, err: `GROUP BY requires at least one aggregate function`},
 		{s: `SELECT 1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 FROM myseries`, err: `unable to parse number at line 1, char 8`},
 		{s: `SELECT 10.5h FROM myseries`, err: `found h, expected FROM at line 1, char 12`},
 		{s: `DELETE`, err: `found EOF, expected FROM at line 1, char 8`},
@@ -852,6 +853,10 @@ func TestParser_ParseStatement(t *testing.T) {
 	for i, tt := range tests {
 		stmt, err := influxql.NewParser(strings.NewReader(tt.s)).ParseStatement()
 
+		// We are memoizing a field so for testing we need to...
+		if s, ok := tt.stmt.(*influxql.SelectStatement); ok {
+			s.GroupByInterval()
+		}
 		if !reflect.DeepEqual(tt.err, errstring(err)) {
 			t.Errorf("%d. %q: error mismatch:\n  exp=%s\n  got=%s\n\n", i, tt.s, tt.err, err)
 		} else if st, ok := stmt.(*influxql.CreateContinuousQueryStatement); ok { // if it's a CQ, there is a non-exported field that gets memoized during parsing that needs to be set