Throw parser error for aggregate functions with group by time without where time clause #2540

corylanou · 2015-05-11T21:54:00Z

If you issued a query like so:

select count(value) from cpu group by time(1d)

It would effectively return an empty result set, but not error out.

This PR will now error out with:

error parsing query: COUNT with GROUP BY time requires WHERE time clause

The correct syntax is to provide a WHERE with a time condition like so:

select count(value) from cpu where time > now() - 30d group by time(1d)

Fixes #2542

otoolep · 2015-05-11T22:02:25Z

influxql/ast.go

+
+// hasWhereTime returns whether or not the select statement has at least 1
+// where condition with time as the condition
+func (s *SelectStatement) hasWhereTime(node Node) bool {


Very similar to OnlyTimeDimensions, but not exactly the same. Can we consolidate the two functions? Perhaps not. If nothing else, can we standardize on the term "time dimension"?

Good catch. They are different methods, but I can line up the naming on them.

otoolep · 2015-05-11T22:03:07Z

Makes sense at a first pass, but I think we're missing a chance to factor out some common code. Perhaps.

toddboom · 2015-05-11T22:11:31Z

+1

pauldix · 2015-05-11T22:38:03Z

This should be changed to be for any aggregate function. This isn't specific to count. Any time there is an aggregate with a GROUP BY time there needs to be a WHERE time ... clause.

…clause should fail

toddboom · 2015-05-11T23:55:25Z

Did you mean to commit the xxx.json file?

toddboom · 2015-05-11T23:56:36Z

influxql/parser.go

 	if d, _ := stmt.GroupByInterval(); stmt.IsRawQuery && d > 0 {
 		return nil, fmt.Errorf("GROUP BY requires at least one aggregate function")
 	}

+	// If we have a count function with a group by time without a where clause, it's an invalid statement


count => aggregate

toddboom · 2015-05-11T23:57:24Z

A couple small tweaks, but lgtm otherwise!

corylanou · 2015-05-11T23:57:57Z

no, I did not intend to commit xxx.json lol. Please send me the newbie award.

otoolep · 2015-05-12T00:01:56Z

influxql/ast_test.go

@@ -295,6 +295,7 @@ func TestSelectStatement_HasWildcard(t *testing.T) {

 	for i, tt := range tests {
 		// Parse statement.
+		t.Logf("statement: %s", tt.stmt)


If we're going to have this in the test output, can we at least at the testname and index, so the output has some context? Otherwise it just adds a lot of next-to useless noise to the test output. (IMHO).

I can add the indexes, but those are typically the useless part for debugging, as counting down are pretty hard. Adding this line was what enabled me to easily search for and find the offending query. I believe our opinions differ :-).

otoolep · 2015-05-12T00:08:03Z

influxql/parser.go

 	if d, _ := stmt.GroupByInterval(); stmt.IsRawQuery && d > 0 {
 		return nil, fmt.Errorf("GROUP BY requires at least one aggregate function")
 	}

+	// If we have a count function with a group by time without a where clause, it's an invalid statement
+	if tr == targetNotRequired { // ignore create continuous query statements
+		if d, _ := stmt.GroupByInterval(); !stmt.IsRawQuery && d > 0 && !stmt.hasTimeDimensions(stmt.Condition) {


You sure this can't return an error? GroupByInternal() returns 0 if there is no GROUP BY? Do also really need IsRawQuery? I am curious.

otoolep · 2015-05-12T00:09:45Z

+1, though the complex if statement tells me our AST code is definitely getting noisy, if you know what I mean.

pauldix · 2015-05-12T00:25:54Z

influxql/ast_test.go

@@ -258,7 +258,7 @@ func TestSelectStatement_HasWildcard(t *testing.T) {

 		// No GROUP BY wildcards, time only
 		{
-			stmt:     `SELECT mean(value) FROM cpu GROUP BY time(5ms)`,
+			stmt:     `SELECT mean(value) FROM cpu where time < now() GROUP BY time(5ms)`,


wait, how does this test pass? This should throw an error because you're asking for 5ms blocks from the beginning of time to now.

@pauldix this is the parser only. It would certainly fail once it hit the query engine, which is a different test.

never mind, this is the ast test. Bounds get checked in the engine :)

corylanou · 2015-05-12T14:56:01Z

influxql/ast.go

+}
+
+// Validate checks certain edge conditions to determine if this is a valid select statment
+func (s *SelectStatement) Validate(tr targetRequirement) error {


@otoolep I moved the if statements to their own function to clear up the parse function. Bottom line is if we want these checks, it requires if statements. I'm open to other suggestions if you feel the code is unbearable.

Throw parser error for aggregate functions with group by time without where time clause

neonstalwart · 2015-05-12T15:52:13Z

this fixes #2444

corylanou added the 2 - Working label May 11, 2015

otoolep reviewed May 11, 2015
View reviewed changes

corylanou force-pushed the count-group-where branch from 3f82cb0 to d821f96 Compare May 11, 2015 22:36

corylanou force-pushed the count-group-where branch from f9cef11 to 7963a85 Compare May 11, 2015 23:47

corylanou added 7 commits May 11, 2015 17:48

error out for invalid count group select without where time

892a6e8

gofmt

c9ca0ce

learn to spell claus

832ca01

hasWhereTime -> hasTimeDimensions

0155f8b

update changelog

f63611c

any aggregate function that has a group by time without a where time …

cebdfea

…clause should fail

no longer need hasCount

7963a85

toddboom reviewed May 11, 2015
View reviewed changes

corylanou added 2 commits May 11, 2015 17:57

temp file go bye bye

198b38a

update comment and changelog for count -> aggregate

93760c8

otoolep reviewed May 12, 2015
View reviewed changes

add index to test log output

882c5e1

otoolep reviewed May 12, 2015
View reviewed changes

pauldix reviewed May 12, 2015
View reviewed changes

corylanou changed the title ~~Throw parser error for invalid count group without where time~~ Throw parser error for aggregate functions with group by time without where time clause May 12, 2015

refactor validating select statement

bfdf050

corylanou reviewed May 12, 2015
View reviewed changes

refactor selectStatement validate

eb1a4f6

corylanou added a commit that referenced this pull request May 12, 2015

Merge pull request #2540 from influxdb/count-group-where

dca9859

Throw parser error for aggregate functions with group by time without where time clause

corylanou merged commit dca9859 into master May 12, 2015

corylanou deleted the count-group-where branch May 12, 2015 15:20

corylanou removed the 2 - Working label May 12, 2015

This was referenced May 12, 2015

Aggregate functions won't return result #2444

Closed

Validate that SELECT * doesn't have a group by time #1998

Closed

otoolep mentioned this pull request May 13, 2015

RC30 WHERE time filter Regression #2557

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw parser error for aggregate functions with group by time without where time clause #2540

Throw parser error for aggregate functions with group by time without where time clause #2540

corylanou commented May 11, 2015

otoolep May 11, 2015

corylanou May 11, 2015

otoolep commented May 11, 2015

toddboom commented May 11, 2015

pauldix commented May 11, 2015

toddboom commented May 11, 2015

toddboom May 11, 2015

toddboom commented May 11, 2015

corylanou commented May 11, 2015

otoolep May 12, 2015

corylanou May 12, 2015

otoolep May 12, 2015

otoolep commented May 12, 2015

pauldix May 12, 2015

corylanou May 12, 2015

pauldix May 12, 2015

corylanou May 12, 2015

neonstalwart commented May 12, 2015

Throw parser error for aggregate functions with group by time without where time clause #2540

Throw parser error for aggregate functions with group by time without where time clause #2540

Conversation

corylanou commented May 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otoolep commented May 11, 2015

toddboom commented May 11, 2015

pauldix commented May 11, 2015

toddboom commented May 11, 2015

Choose a reason for hiding this comment

toddboom commented May 11, 2015

corylanou commented May 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otoolep commented May 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neonstalwart commented May 12, 2015