-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Group by should require an aggregate #2034
Conversation
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was failing a vet check from a previous commit.
Several tests were not valid due to not having an aggregate specified. They have been updated as part of this PR. |
@@ -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 needs at least one aggregate function") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to a better error message if someone has ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps capitalize it, to make it clear it's influxql, and I think "requires" reads better. I.e. "GROUP BY requires at least one aggregate function"
I have some minor feedback, which I think would improve the code. +1 on a green build. |
bc056d2
to
dd6bb91
Compare
Group by should require an aggregate
fixes #2017