update LAST_INSERT_ID when auto incrementing from empty, NULL, and DEFAULT#2614
update LAST_INSERT_ID when auto incrementing from empty, NULL, and DEFAULT#2614
LAST_INSERT_ID when auto incrementing from empty, NULL, and DEFAULT#2614Conversation
jycor
changed the title
LAST_INSERT_ID when auto incrementing from empty, NULL, and DEFAULT
fulghum
left a comment
fulghum
left a comment
There was a problem hiding this comment.
Nice job fixing this one!! 🙌
Only two real requests from me:
- see if we can improve the naming a bit and make it really consistent across all the types where we're using
autoAuto...stuff. - double check if the latest versions of MySQL really allow empty values for an auto_inc column.
|
|
||
| { | ||
| Query: "insert into t(pk) values (10), (default);", | ||
| Expected: []sql.Row{{types.OkResult{RowsAffected: 2, InsertID: 10}}}, |
There was a problem hiding this comment.
(minor) should InsertID be 11? I couldn't see where MySQL was returning this field, but it seems like it would be the same as last_insert_id? (all I saw MySQL return was the RowsAffected)
Not a big deal, and I don't think we need to change it here, since it isn't really user facing, but figured I'd mention it.
There was a problem hiding this comment.
It definitely seems like it should be 11.
Fixing it breaks a bunch of tests, and I'm not sure if they are wrong or not.
Will look further into this in a future PR.
| { | ||
| Query: "insert into t(pk) values ();", | ||
| Expected: []sql.Row{{types.OkResult{RowsAffected: 1, InsertID: 26}}}, | ||
| }, |
There was a problem hiding this comment.
In MySQL 8.3, I get an error for this one:
insert into t(pk) values ();
ERROR 1136 (21S01): Column count doesn't match value count at row 1There was a problem hiding this comment.
So, I think this is actually an existing bug that's unrelated to auto_increment and any of the changes in this PR.
The test should be insert into t values (), which doesn't error in MySQL
Will make an issue for this case.
2 participants
Our logic for determining whether or not we needed to update last insert id only looked at the insertSource schema.
This does not take into consideration
empty,NULLorDEFAULTvalues.Additionally, the value that last insert id is set to depends on what the auto increment value will be.
This PR addresses those issues.
Also, has some refactoring for readability.
fixes: dolthub/dolt#7565