-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Implement comprehensive query wildcard re-write #1632
Conversation
0a7a151
to
28f0f5a
Compare
This changes uses dedicated code for |
Addresses issue #1624. |
@dgnorton -- could do with your review too. |
@@ -1977,6 +1977,12 @@ func (s *Server) ExecuteQuery(q *influxql.Query, database string, user *User) Re | |||
|
|||
// executeSelectStatement plans and executes a select statement against a database. | |||
func (s *Server) executeSelectStatement(stmt *influxql.SelectStatement, database string, user *User) *Result { | |||
// Perform any necessary query re-writing. | |||
stmt, err := s.rewriteSelectStatement(stmt) |
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.
We should only rewrite if the statement actually has a wildcard. The statement struct should have a method that indicates if it has a wildcard.
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.
👍
+1 |
Are we going to handle wildcard expansion for multiple source measurements? e.g. |
Wildcard method added @pauldix @benbjohnson -- there are a couple of different places the check could be placed, of course. |
@@ -1508,6 +1508,8 @@ func (p *Parser) parseUnaryExpr() (Expr, error) { | |||
case DURATION_VAL: | |||
v, _ := ParseDuration(lit) | |||
return &DurationLiteral{Val: v}, nil | |||
case MUL: | |||
return &Wildcard{}, nil |
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.
Wondering if wildcard expressions should be parsed separately so we don't allow weird stuff like SELECT value + *,* FROM cpu
, which will get expanded to look something like SELECT value + *, value1, value2 FROM cpu
. /cc @benbjohnson
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.
Sounds like a reasonable suggestion, but outside the scope of this change I'd say. Happy to open an issue.
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 agree that we should do a lookahead for wildcards before parsing expressions but we can push that to a separate PR.
@pauldix is refactoring a chunk of the query engine, so this PR is on hold. He and @benbjohnson have promised to merge (or possibly rework) this when ready. |
This code, when given a SELECT statement, returns another SELECT statement such that all "query" and GROUP BY wildcards have been replaced with all Measurement fields and tag keys respectively.
GROUP BY * functionality is now in place.
7b916cd
to
4d896fa
Compare
Rebased on master, and green build now. |
Implement comprehensive query wildcard re-write
This patch implements comprehensive rewrite of wildcards, for both the "source" of the
SELECT
statement, and theGROUP BY
. The parser prevents statements such asSELECT *,value
, so those unit tests are commented out. However, the query parser should be fixed up eventually to allow this, as other SQL-based query engines support this.