Change parser to allow SET SQL_AUTO_IS_NULL=0, NAMES utf8#3776
Change parser to allow SET SQL_AUTO_IS_NULL=0, NAMES utf8#3776arthurnn wants to merge 1 commit intovitessio:masterfrom
SET SQL_AUTO_IS_NULL=0, NAMES utf8#3776Conversation
Before this change the parser would allow `SET VAR=VALUE` or `SET NAMES charsetvalue`, but would not allow both combined. MySQL allows them, and in fact our Ruby on Rails app connection diver sends that to each connection. This fixes the parser to allow to set a var an a charset in the same statment. Signed-off-by: Arthur Neves <arthurnn@gmail.com>
| } | ||
| if len(vals) > 0 && charset != "" { | ||
| return &sqltypes.Result{}, vterrors.New(vtrpcpb.Code_INVALID_ARGUMENT, "unexpected key values and charset, must specify one") | ||
| } |
There was a problem hiding this comment.
This check was there before to catch a charset and var change when they happened in the same statement. Now that is handled fine by the parser, so we can remove the check.
cc @bbeaudreault as you added to initial check in here 5faaac2
|
I compiled and tested this locally: Seems like it works |
vmg
left a comment
vmg
left a comment
There was a problem hiding this comment.
This looks great to me. Handling this on the parser is definitely the right approach. 👌
| }, { | ||
| input: "set charset default", | ||
| output: "set ", | ||
| }, { |
There was a problem hiding this comment.
I think it would make sense to modify Set.Format to print both the Exprs and the charset (if non-null) so that this test (and all the ones above it) would more clearly show that the parser is preserving the charset.
https://github.com/tinyspeck/vitess/blame/master/go/vt/sqlparser/ast.go#L579-L587
|
|
||
| set_statement: | ||
| SET comment_opt update_list | ||
| SET comment_opt update_list ',' charset_or_character_set charset_value force_eof |
There was a problem hiding this comment.
I don't think we should have a force_eof here since it seems to me it would allow anything after the character set, which is more lenient than I think we want this to be.
More generally I'm somewhat surprised this doesn't introduce grammar conflicts since the update_list is also a comma-separated set of expressions. As such, I don't off-hand see how the parser can figure out whether to continue processing the expressions as part of the update list as opposed to the character set case.
I suppose if it works, it works, but can you please confirm you don't get any shift-reduce or reduce-reduce conflicts when you build the grammar?
I do, however, think we should have a test case that shows the grammar won't parse this:
SET foo=1, NAMES 'utf8', bar=1
There was a problem hiding this comment.
I think there's a more generic way to fix this. Let me spin up a PR.
| err: "sql_auto_is_null is not currently supported", | ||
| }, { | ||
| in: "set sql_auto_is_null = 0, names 'utf8'", | ||
| out: &vtgatepb.Session{Autocommit: true}, |
There was a problem hiding this comment.
We should probably have a test case of the form:
}, {
in: "set sql_auto_is_null = 0, character set ascii",
err: "unexpected value for charset: ascii",
}, {
|
The 'better' fix was more complicated than expected, but it should give us more runway for the SET statement: #3777. |
4 participants
Before this change, the parser would allow
SET VAR=VALUEorSET NAMES charsetvalue, but would not allow both combined.MySQL allows them and in fact our Ruby on Rails app connection driver
sends that to each connection.
This fixes the parser to allow to set a var and a charset in the same statement.
See
SQL_AUTO_IS_NULL=0was already added to the executor in another PR. So no need to add that variable option there.cc @vmg @bryanaknight @jaredonline as this is related to #3611