-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
add support for DEFAULT in insert statements for sequences to the query planner #5277
Conversation
a9ce776
to
24be5d0
Compare
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.
Not following here.
INSERT INTO t VALUES(DEFAULT);
is not the same as
INSERT INTO t VALUES(null);
Shouldn't the planner check that the column is a sequence or something else we have a default value for before accepting this query?
The only place swapBindVariables is called is for swapping out sequences. |
LGTM |
Closed by mistake. So sorry. |
24be5d0
to
22833a9
Compare
go/vt/vtgate/planbuilder/insert.go
Outdated
@@ -232,6 +232,9 @@ func modifyForAutoinc(ins *sqlparser.Insert, eins *engine.Insert) error { | |||
func swapBindVariables(rows sqlparser.Values, colNum int, baseName string) (sqltypes.PlanValue, error) { | |||
pv := sqltypes.PlanValue{} | |||
for rowNum, row := range rows { | |||
if _, ok := row[colNum].(*sqlparser.Default); ok { |
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.
One minor nit: It's better to move this code into modifyForAutoinc
because it's now specific to that context. Otherwise, there's a risk that someone may accidentally reuse this function for other purposes.
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 with this conceptually. I was just trying to not add another loop for the sake of performance. Should I rename this function instead?
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.
Rename sounds good.
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.
Sorry, I think I misread what you suggested. If I move this code into modifyForAutoinc, I think we'd like this function to be deleted as well. Is that correct?
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.
Right. I think this function was previously reused, but it looks like I inlined it for the other use case.
…ry planner This will treat DEFAULT as "null" for the purposes of sequences when running insert statements. This matches with MySQL's documented behavior and observed resulted. See for more information: https://dev.mysql.com/doc/refman/8.0/en/data-type-defaults.html Signed-off-by: Mark Caudill <mark@sharpspring.com>
22833a9
to
53949d3
Compare
This will treat DEFAULT as "null" for the purposes of sequences when
running insert statements. This matches with MySQL's documented
behavior and observed resulted.
See for more information:
https://dev.mysql.com/doc/refman/8.0/en/data-type-defaults.html