-
Notifications
You must be signed in to change notification settings - Fork 49
Add support for INSERT INTO ... SET ... syntax
#273
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
Conversation
| $is_node = $child instanceof WP_Parser_Node; | ||
|
|
||
| // Skip the SET keyword in "INSERT INTO ... SET ..." syntax. | ||
| if ( $is_token && WP_MySQL_Lexer::SET_SYMBOL === $child->id ) { |
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.
Any chance we ever see a SET symbol in an unexpected place? The tokenizing parser made me hesitant about this pattern of going through all the child nodes up to a symbol, e.g. INSERT INTO x (SELECT ... SET ...). I expect the answer is "no" since a) I don't think there's a way to cram in SET keyword into a select statement and b) that would likely be handled in a different AST node handler and c) get_children() is just for direct children, not descendants, right? Still, I wanted to voice it.
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.
@adamziel Yeah, it is safe, because we're iterating direct children and the grammar is:
insertStatement:
INSERT_SYMBOL insertLockOption? IGNORE_SYMBOL? INTO_SYMBOL? tableRef usePartition? (
insertFromConstructor ({ serverVersion >= 80018}? valuesReference)?
| SET_SYMBOL updateList ({ serverVersion >= 80018}? valuesReference)?
| insertQueryExpression
) insertUpdateList?
;
That said, I may refactor this to something nicer in a subsequent PR with a fix for #268.
| && ( | ||
| 'insertFromConstructor' === $child->rule_name | ||
| || 'insertQueryExpression' === $child->rule_name | ||
| || 'updateList' === $child->rule_name |
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.
Oh, so we handle updateList both in this else/if branch and in the next one, just in different ways?
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.
@adamziel At the moment, it is twice—once for the "STRICT" SQL mode and once for the WP-like "non-STRICT" mode.
For the non-STRICT mode, we apply implicit defaults, type casting, etc. based on the data from the information schema. For the STRICT mode, we just pass it "directly" to SQLite.
That said, I will have to apply type casting in both STRICT and non-STRICT mode due to issues like #268, so in a subsequent PR, I will try to unify this better. It will also allow us rejecting/trimming values that are too long, based on the current SQL mode.
| foreach ( $fields_node->get_child_nodes() as $field ) { | ||
| $insert_list[] = $this->unquote_sqlite_identifier( $this->translate( $field ) ); | ||
| } | ||
| } elseif ( 'updateList' === $node->rule_name ) { |
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.
Aha! so this is for INSERT INTO table VALUES (val1, val2) and the other code branch is for INSERT INTO table (col1, col2) VALUES (val1, val2). Do we need to support both? Or could we just go with the one that lists columns explicitly?
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.
In MySQL, you can use all of INSERT INTO table VALUES (...), INSERT INTO table (col1, col2, ...) VALUES (...), and INSERT INTO table SET col1 = val1, ..., so we need to support all of them.
|
I left a few thoughts, but nothing important. Thanks Jan! |
|
I will go ahead and merge this one and keep the remarks in mind for the subsequent PR. |
Some popular plugins use the
INSERT INTO … SET …syntax that is specific to MySQL.In SQLite, this needs to be translated into the standard
INSERT INTO ( … ) VALUES ( … )syntax.