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.

Sign up for GitHub

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

Enum value parsing: do not parse by whitespace #15493

Merged
merged 3 commits into from
Mar 17, 2024
Merged

Enum value parsing: do not parse by whitespace #15493

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8dcf432
Enum value parsing: do not parse by whitespace
shlomi-noach Mar 14, 2024
53b09f7
Add parser for enum / set values
dbussink Mar 15, 2024
2b4a10c
Fix failing tests
dbussink Mar 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-whitespace/alter
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
change e e enum('red', 'light green', 'blue', 'orange', 'yellow') collate 'utf8_bin' null default null
27 changes: 27 additions & 0 deletions go/test/endtoend/onlineddl/vrepl_suite/testdata/enum-whitespace/create.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
drop table if exists onlineddl_test;
create table onlineddl_test (
id int auto_increment,
i int not null,
e enum('red', 'light green', 'blue', 'orange') null default null collate 'utf8_bin',
primary key(id)
) auto_increment=1;

drop event if exists onlineddl_test;
delimiter ;;
create event onlineddl_test
on schedule every 1 second
starts current_timestamp
ends current_timestamp + interval 60 second
on completion not preserve
enable
do
begin
insert into onlineddl_test values (null, 11, 'red');
insert into onlineddl_test values (null, 13, 'light green');
insert into onlineddl_test values (null, 17, 'blue');
set @last_insert_id := last_insert_id();
update onlineddl_test set e='orange' where id = @last_insert_id;
insert into onlineddl_test values (null, 23, null);
set @last_insert_id := last_insert_id();
update onlineddl_test set i=i+1, e=null where id = @last_insert_id;
end ;;
3 changes: 1 addition & 2 deletions go/vt/schema/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"strconv"
"strings"

"vitess.io/vitess/go/textutil"
"vitess.io/vitess/go/vt/sqlparser"
)

Expand Down Expand Up @@ -122,7 +121,7 @@ func parseEnumOrSetTokens(enumOrSetValues string) (tokens []string) {
// input should not contain `enum(...)` column definition, just the comma delimited list
return tokens
}
tokens = textutil.SplitDelimitedList(enumOrSetValues)
tokens = strings.Split(enumOrSetValues, ",")
Copy link
Contributor

@mattlord mattlord Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Any reason(s) not to use the csv encoding packing in the stdlib? https://pkg.go.dev/encoding/csv

Copy link
Contributor

Choose a reason for hiding this comment

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

The main issue would be that quoting works differently there with " and not ' like in SQL. Also escaping of quotes is different, so we'd have to write something specific for this or leverage instead sqlparser as @shlomi-noach mentioned.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I did notice that the csv encoder simply removes any commas in the values which surprised me. I do think that we should take this opportunity to address this more fully though as MySQL happily accepts enum string values with commas:

mysql> create table t1 (id int not null auto_increment primary key, ev enum('hi there', 'with, comma'));
Query OK, 0 rows affected (0.02 sec)

mysql> show create table t1;
+-------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                                                                                                        |
+-------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| t1    | CREATE TABLE `t1` (
  `id` int NOT NULL AUTO_INCREMENT,
  `ev` enum('hi there','with, comma') DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci |
+-------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.01 sec)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, there's a bunch of things that still fail here. I agree we should fix this properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shlomi-noach I pushed up a little specific parser for this in f90e797 that deals also with quotes inside the values as well.

I also added some low level test there with stuff like newlines, I think we should also add those to the end-to-end test too to try all the exotic things possible here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Realized my parser was not enough and also added a proper decoder for string values so we handle things line newlines correctly.

for i := range tokens {
if strings.HasPrefix(tokens[i], `'`) && strings.HasSuffix(tokens[i], `'`) {
tokens[i] = strings.Trim(tokens[i], `'`)
Expand Down
19 changes: 19 additions & 0 deletions go/vt/schema/parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,19 @@ func TestParseEnumValues(t *testing.T) {
assert.Equal(t, input, enumValues)
}
}

{
inputs := []string{
``,
`abc`,
`func('x small','small','medium','large','x large')`,
`set('x small','small','medium','large','x large')`,
}
for _, input := range inputs {
enumValues := ParseEnumValues(input)
assert.Equal(t, input, enumValues)
}
}
}

func TestParseSetValues(t *testing.T) {
Expand Down Expand Up @@ -125,6 +138,12 @@ func TestParseEnumTokens(t *testing.T) {
expect := []string{"x-small", "small", "medium", "large", "x-large"}
assert.Equal(t, expect, enumTokens)
}
{
input := `'x small','small','medium','large','x large'`
enumTokens := parseEnumOrSetTokens(input)
expect := []string{"x small", "small", "medium", "large", "x large"}
assert.Equal(t, expect, enumTokens)
}
{
input := `enum('x-small','small','medium','large','x-large')`
enumTokens := parseEnumOrSetTokens(input)
Expand Down
Loading