-
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
Parser fix, only allow ORDER BY ASC and ORDER BY time ASC #3409
Conversation
+1 |
return nil, errors.New("only ORDER BY time ASC supported at this time") | ||
} | ||
return append(fields, &SortField{Ascending: (tok == ASC)}), nil | ||
} else if tok != IDENT && tok != STRING { |
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.
Should only accept IDENT
. ORDER BY "foo" ASC
is okay but not ORDER BY 'foo' ASC
.
if tok != IDENT && tok != STRING { | ||
return nil, newParseError(tokstr(tok, lit), []string{"identifier"}, pos) | ||
} | ||
field.Name = lit |
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.
The // Scan for IDENT...
line through field.Name = lit
should be changed to...
// Parse field name
ident, err := p.parseIdent()
if err != nil {
return nil, err
}
field.Name = ident
@dgnorton made the |
+1 |
Parser fix, only allow ORDER BY ASC and ORDER BY time ASC
Fixes #3380, which was introduced in #2912, which itself was a fix for #2731.
This PR makes the parser accept
ORDER BY ASC
andORDER BY time ASC
as the only validORDER BY
clauses. All otherORDER BY
clauses will return an error.This also adds some parsing logic for
ORDER BY
clauses and fixesSortField.String()
to output correctly so the query can be rewritten correctly in therewriteSelectStatement
step of the query executor.