Allow other options to precede COLLATE in column type defintions #195
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hydrocharged
force-pushed
the
daylon/not-null-collate-ordering
branch
from
82f888a
to
d4cdb1d
Compare
zachmu
approved these changes
Sep 28, 2022
| }, { | ||
| // test utc_timestamp with and without () | ||
| input: "create table t (\n" + | ||
| " time1 timestamp default utc_timestamp,\n" + |
There was a problem hiding this comment.
Where did the tests for these go?
There was a problem hiding this comment.
You didn't read my description sir lol
| @@ -2361,89 +2365,116 @@ column_type_options: | |||
| { | |||
| opt := ColumnType{Null: BoolVal(true), NotNull: BoolVal(false), sawnull: true} | |||
| if err := $1.merge(opt); err != nil { | |||
| yylex.Error(err.Error()) | |||
| return 1 | |||
| yylex.Error(err.Error()) | |||
There was a problem hiding this comment.
Probably should preserve the default 4 space indents here and below (better corresponds to the golang tab)
There was a problem hiding this comment.
All other indents in the file (or at least the majority) use two spaces rather than tabs, so I changed it to be more consistent. Makes sense that we'd use tab instead, but I wanna keep consistency for now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes dolthub/dolt#4403
Previously,
COLLATEhad to immediately follow the column type in a column definition. As per the above issue, this is not a required rule in MySQL. The fix involved moving all collation-related options from thecolumn_typerule to thecolumn_type_options. This caused a conflict incolumn_type_options, ascolumn_defaultalso has aCOLLATErule. The conflicting rule resolved to the following:For reference, this is the new
COLLATErule incolumn_type_options:Given the MySQL expression
DEFAULT "xyz" COLLATE utf8mb4_bin, it could match either rule, which caused the conflict.value_expressionis too permissive in this context, and although we filter out invalid expressions in GMS, we need to be more restrictive here in the parser to prevent conflicts. In addition, the above example should put the collation on the column, as it is not possible to add a collation to a default string literal (must use the expression form:DEFAULT ("xyz" COLLATE utf8mb4_bin)).To fix this, the
column_defaultrule was updated to be vastly more restrictive. This also highlighted some tests in GMS that enforce incorrect behavior, but those have been fixed ahead-of-time, and will be incorporated into the bump PR.NOTE: For some reason, we had tests allowing the use of
utc_timestamp,utc_date, etc. as default values without the need for parentheses. This is not allowed in MySQL, so I am unsure as to why we allowed them in the first place. Perhaps because they're similar toCURRENT_TIMESTAMP, so we just allowed all of them? Regardless, the tests have been removed as they no longer pass (as they should not pass).