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

Parse COLLATE BINARY on individual columns. #292

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

nicktobey
Copy link

We should be able to parse statements like:

create table test (pk varchar(255) collate binary)

This particular example will eventually get rewritten as create table test (pk varbinary(255)), but that doesn't happen during parsing, so the added vitess tests still expect varchar.

@nicktobey nicktobey requested a review from jycor November 30, 2023 21:28
@nicktobey nicktobey requested a review from zachmu as a code owner November 30, 2023 21:28
Copy link

@jycor jycor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, maybe add tests in GMS using SHOW CREATE TABLE to make sure these are actually rewritten to varbinary.

tmp> create table t2 (i varchar(10) charset binary);
tmp> show create table t2;
+-------+------------------------------------------------------------------+
| Table | Create Table                                                     |
+-------+------------------------------------------------------------------+
| t2    | CREATE TABLE `t2` (                                              |
|       |   `i` varbinary(10)                                              |
|       | ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin |
+-------+------------------------------------------------------------------+
1 row in set (0.00 sec)

@nicktobey
Copy link
Author

LGTM, maybe add tests in GMS using SHOW CREATE TABLE to make sure these are actually rewritten to varbinary.

tmp> create table t2 (i varchar(10) charset binary);
tmp> show create table t2;
+-------+------------------------------------------------------------------+
| Table | Create Table                                                     |
+-------+------------------------------------------------------------------+
| t2    | CREATE TABLE `t2` (                                              |
|       |   `i` varbinary(10)                                              |
|       | ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin |
+-------+------------------------------------------------------------------+
1 row in set (0.00 sec)

I'll include a test like that in the GMS bump.

@nicktobey nicktobey merged commit 8b522c3 into main Dec 1, 2023
4 checks passed
@nicktobey nicktobey deleted the nicktobey/collatebinary branch December 1, 2023 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants