-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
MDEV-29919: Support INSERT ... AS alias ON DUPLICATE KEY UPDATE #4544
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
base: main
Are you sure you want to change the base?
Conversation
b3b3fa7 to
d724e69
Compare
d724e69 to
ff235ab
Compare
FooBarrior
left a comment
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.
Hi @MooSayed1! Thanks for your contribution.
I think this needs a better high-level detalization. What happens if a table has a field with the same name? This is a MySQL compatibility task -- so it's important to know what MySQL does in this case.
These details on the approach to name resolution will be important to be mentioned in the commit comment.
| When we are in ON DUPLICATE KEY UPDATE and the table qualifier matches | ||
| the insert_values_alias, we should resolve this as VALUES(column). | ||
| */ | ||
| if (select && |
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.
- No need to specify MDEV in the comments.
- It's enough to have a meaningful comment.
- In general, better not to refer to an exact sql_command value.
- We need to make sure the syntax can't be used in the unsupposed ways, like that REPLACE...AS would fail with a syntax error, and CREATE ... VALUES would.
- I'm surprised you have to check
select. Why? - What does this code placement mean? it's in
if (!field), meaning that it would apply only if field is not early-provided[?], but beforefind_field_in_tables, meaning that it would supersede a name resolution?
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.
first thanks for the review
then
Q1, Q2, Q3: Got it, will fix all of these.
Q4: Currently the grammar allows REPLACE ... AS alias to parse (since table_value_constructor is shared), but it's ignored at runtime because duplicates != DUP_UPDATE. but i think it would be better to get a syntax error at parse time instead of silent acceptance? and maybe adding tests for these scenarios
Q5: The select check was me being cautious - ensuring we have a valid query context. It's probably redundant.
Q6: It's placed before find_field_in_tables() so the alias takes priority. If a table and alias share the same name, new.b should resolve to the alias (inserted value), not the table. This matches MySQL behavior.
and should we continue the discussion here or move to Zulip?
MySQL 8.0.20 introduced a new syntax for
INSERT ... ON DUPLICATE KEY UPDATEthat allows referencing the inserted row using an alias instead of theVALUES()function:The alias syntax is cleaner and more readable, especially when referencing multiple columns or using expressions like
new.a + new.b.Implementation
opt_values_row_aliasrule to acceptAS aliasafter VALUESinsert_values_aliasfield to store the aliasItem_field::fix_fields(), when we're in ON DUPLICATE KEY UPDATE and the table qualifier matches the alias, we convert the reference to anItem_insert_value(equivalent toVALUES())The traditional
VALUES()function continues to work unchanged.Tests
Added
mysql-test/main/insert_update_alias.testcovering:INSERT ... AS alias ON DUPLICATE KEY UPDATEnew.a + new.b)VALUES()function