-
-
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
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.
sql/item.cc
Outdated
| 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?
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.
This matches MySQL behavior.
Please show the related MySQL output
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.
MariaDB> SELECT VERSION();
+----------------------+
| VERSION() |
+----------------------+
| 12.3.0-MariaDB-debug |
+----------------------+
MariaDB> CREATE TABLE t1 (a INT PRIMARY KEY, b INT, c INT);
MariaDB> INSERT INTO t1 VALUES (1, 10, 100);
MariaDB> SELECT * FROM t1;
+---+------+------+
| a | b | c |
+---+------+------+
| 1 | 10 | 100 |
+---+------+------+
MariaDB> INSERT INTO t1 VALUES (1, 20, 200) AS new
ON DUPLICATE KEY UPDATE b = new.b, c = new.c;
MariaDB> SELECT * FROM t1;
+---+------+------+
| a | b | c |
+---+------+------+
| 1 | 20 | 200 |
+---+------+------+
MariaDB> INSERT INTO t1 VALUES (1, 5, 50) AS new
ON DUPLICATE KEY UPDATE b = b + new.b, c = c + new.c;
MariaDB> SELECT * FROM t1;
+---+------+------+
| a | b | c |
+---+------+------+
| 1 | 25 | 250 |
+---+------+------+```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 was asking about
If a table and alias share the same name, new.b should resolve to the alias (inserted value), not the table.
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 okay mb
MySQL 8.0:
mysql> CREATE TABLE new (a INT PRIMARY KEY, b INT);
mysql> INSERT INTO new VALUES (1, 999);
mysql> CREATE TABLE t1 (a INT PRIMARY KEY, b INT);
mysql> INSERT INTO t1 VALUES (1, 10);
mysql> INSERT INTO t1 VALUES (1, 50) AS new ON DUPLICATE KEY UPDATE b = new.b;
mysql> SELECT * FROM t1;
+---+------+
| a | b |
+---+------+
| 1 | 50 | -- Alias wins (not 999 from table)
+---+------+MariaDB the new implementation
MariaDB> -- Same test
MariaDB> INSERT INTO t1 VALUES (1, 50) AS new ON DUPLICATE KEY UPDATE b = new.b;
MariaDB> SELECT * FROM t1;
+---+------+
| a | b |
+---+------+
| 1 | 50 | -- Alias wins too Same as MySQL
+---+------+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 think new couldn't be a table reference in the same request with no/different alias anyway.
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.
Can you clarify this further?
From what I understand, my approach is incorrect when it comes to treating this as an alias. For example, consider the following case:
INSERT INTO t1 VALUES (1, 10);
SELECT * FROM t1;
INSERT INTO t1 VALUES (1, 50) AS t1
ON DUPLICATE KEY UPDATE b = t1.b;In this situation, I should get a syntax error. However, with my current implementation, it continues without any issues and incorrectly treats the newly inserted row as a valid table alias.
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.
@FooBarrior do u prefer making a new commit to fix this or u prefer doing only one commit to get the issue done?
43a115c to
4dcef93
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.
Please add test with CREATE..VALUES AS ident and REPLACE...VALUES AS ident producing a syntax error
| ); | ||
|
|
||
| --echo # | ||
| --echo # Test 1: Basic INSERT AS alias ON DUPLICATE KEY UPDATE |
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.
Let's remove the numbers: if we'll add some test in the middle, we'll have to renumerate everything.
Just:
--echo # Basic INSERT AS alias ON DUPLICATE KEY UPDATE
| ensures the alias takes priority over any existing table with | ||
| the same name, matching MySQL behavior. | ||
| */ | ||
| if (thd->lex->duplicates == DUP_UPDATE && |
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 think this line should be rather an assertion inside the if. If we've got to here with lex->insert_values_alias.str, then it should only be a valid case.
| SELECT * FROM t1; | ||
|
|
||
| --echo # | ||
| --echo # Test 4: Backward compatibility - VALUES() function still works |
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 in this one. VALUES (a) is tested even in main.insert.
| --echo # | ||
| --echo # Test 9: Alias takes priority over existing table with same name | ||
| --echo # | ||
| DROP TABLE IF EXISTS new_values; |
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.
junk drop
sql/item.cc
Outdated
| /* | ||
| Create a field reference without the alias qualifier, | ||
| then wrap it in Item_insert_value to get the inserted value (basically converting alis.column to Values(column)). | ||
| Use change_item_tree() for prepared statement compatibility (this's passing ps mode testing). |
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.
Remove the comment as it doesn't complement the code with any new information (or change it to do so, without stating what's written in the code).
sql/sql_lex.h
Outdated
|
|
||
| enum enum_duplicates duplicates; | ||
| /* Alias for INSERT ... To support the new syntax as MySQL */ | ||
| LEX_CSTRING insert_values_alias; |
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.
Make it a Lex_ident derivative as a modern alternative.
sql/sql_lex.h
Outdated
| const char *clause_that_disallows_subselect; | ||
|
|
||
| enum enum_duplicates duplicates; | ||
| /* Alias for INSERT ... To support the new syntax as MySQL */ |
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.
/* Represents INSERT...VALUES as <alias> */
No need to mention MySQL "new" syntax, available yet in MySQL 8.0.19 dated year 2020 (6 years ago)
sql/sql_yacc.yy
Outdated
| */ | ||
| opt_values_row_alias: | ||
| { | ||
| Lex->insert_values_alias= null_clex_str; // if AS..alias provided after Values() |
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.
remove junk (and partially false) comments
| @@ -0,0 +1,106 @@ | |||
| # | |||
| # Test for INSERT ... VALUES (...) AS alias ON DUPLICATE KEY UPDATE syntax | |||
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 think this test can be moved to main.insert (@vuvova agree?). ODKU is also covered there.
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.
Add MDEV and jira title with --echo #
4dcef93 to
cdc6fe5
Compare
|
@FooBarrior Done. |
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.
Please be more careful with making changes. MariaDB is used on millions of machines, and we pay extra attention to the code quality. I hope you made that mistake just in a hurry.
| table_name.str && | ||
| table_name.streq(thd->lex->insert_values_alias)) | ||
| { | ||
| DBUG_ASSERT(thd->lex->insert_values_alias.str); |
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.
that's... wrong?
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.
Just to confirm: should I remove the condition (thd->lex->duplicates == DUP_UPDATE) and instead move it into a DBUG_ASSERT inside the if block?
| TRUNCATE TABLE t1; | ||
| INSERT INTO t1 VALUES (1, 10, 100); | ||
| --error ER_PARSE_ERROR | ||
| REPLACE INTO t1 VALUES (1, 50, 500) AS new ON DUPLICATE KEY UPDATE b = new.b; |
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.
ON DUPLICATE KEY UPDATE was never possible for REPLACE, so that's out of question.
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.
Thanks for the clarification. I realized my previous test included ON DUPLICATE KEY UPDATE with REPLACE, which caused a syntax error before even reaching the alias check.
I have updated the test to simply: REPLACE INTO t1 VALUES (...) AS new;
This should correctly verify that the alias syntax is rejected for REPLACE statements. Does this look correct to you?
|
|
||
| enum enum_duplicates duplicates; | ||
| /* Represents INSERT...VALUES as <alias> */ | ||
| Lex_ident_sys_st insert_values_alias; |
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.
can you plase explain the reason to choose Lex_ident_sys_st
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.
this's what i understand from this comment
Make it a Lex_ident derivative as a modern alternative.
when i searched i found this and i though it's the same as u mean
and it was matching what the parser's ident rule returns
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