Skip to content
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

Modifying a column is order-sensitive for NOT NULL and COLLATE #4403

Closed
dphil opened this issue Sep 26, 2022 · 3 comments · Fixed by dolthub/vitess#195
Closed

Modifying a column is order-sensitive for NOT NULL and COLLATE #4403

dphil opened this issue Sep 26, 2022 · 3 comments · Fixed by dolthub/vitess#195
Assignees
Labels
bug Something isn't working sql Issue with SQL

Comments

@dphil
Copy link

dphil commented Sep 26, 2022

In MySQL, when modifying a column in an ALTER TABLE operation, you can specify the column as being NOT NULL as well as its collation in any order.

In Dolt, it reports a syntax error if NOT NULL is defined before COLLATE.

MySQL:

mysql> CREATE TABLE `users` (`name` varchar(255) NOT NULL) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin;
Query OK, 0 rows affected (0.04 sec)

mysql> ALTER TABLE `users` modify `name` varchar(255) NOT NULL COLLATE 'utf8mb4_0900_ai_ci';
Query OK, 0 rows affected (0.02 sec)
Records: 0  Duplicates: 0  Warnings: 0

mysql> show full columns from users;
+-------+--------------+--------------------+------+-----+---------+-------+---------------------------------+---------+
| Field | Type         | Collation          | Null | Key | Default | Extra | Privileges                      | Comment |
+-------+--------------+--------------------+------+-----+---------+-------+---------------------------------+---------+
| name  | varchar(255) | utf8mb4_0900_ai_ci | NO   |     | NULL    |       | select,insert,update,references |         |
+-------+--------------+--------------------+------+-----+---------+-------+---------------------------------+---------+
1 row in set (0.00 sec)

Dolt:

dolt> CREATE TABLE `users` (`name` varchar(255) NOT NULL) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin;

dolt> ALTER TABLE `users` modify `name` varchar(255) NOT NULL COLLATE 'utf8mb4_0900_ai_ci';
Error parsing SQL
syntax error at position 64 near 'COLLATE'
ALTER TABLE `users` modify `name` varchar(255) NOT NULL COLLATE 'utf8mb4_0900_ai_ci'
                                                                ^

dolt> ALTER TABLE `users` modify `name` varchar(255) COLLATE 'utf8mb4_0900_ai_ci' NOT NULL;

dolt> describe users;
+-------+-----------------------------------------+------+-----+---------+-------+
| Field | Type                                    | Null | Key | Default | Extra |
+-------+-----------------------------------------+------+-----+---------+-------+
| name  | varchar(255) COLLATE utf8mb4_0900_ai_ci | NO   |     | NULL    |       |
+-------+-----------------------------------------+------+-----+---------+-------+
1 row in set (0.00 sec)
@dphil
Copy link
Author

dphil commented Sep 26, 2022

Easily worked around by just ordering the column properties as Dolt requires, though can be a little inconvenient with some connectors such as knex's schema builder which apparently always orders NOT NULL before COLLATE, thus requiring to break out into a raw sql statement to reorder them.

@timsehn
Copy link
Contributor

timsehn commented Sep 26, 2022

Thanks @Hydrocharged will pick this up tonight.

@Hydrocharged
Copy link
Contributor

I apologize for the delay, I thought this would take 30 min but ended up taking almost 2 days. To put it short, allowing NOT NULL to come before COLLATE made the parser ambiguous, and finding the issue was not obvious at all (along with figuring out a fix). Turns out our handling of DEFAULT values was completely wrong, and we accounted for it in go-mysql-server, but that incorrect-ness was the root cause of the ambiguity in our parser.

The PR fixing this issue is linked in the sidebar. We'll do another release once the change makes its way into Dolt later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working sql Issue with SQL
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants