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

single quoted default value in create table statements #1036

Merged
merged 23 commits into from
Jun 14, 2022

Conversation

jennifersp
Copy link
Contributor

@jennifersp jennifersp commented Jun 1, 2022

Current Dolt default literal values are stores in double quotes, but MySQL does not parse double quoted default literal value in CREATE TABLE statements for both sql shell and importing dumps.
Fixes dolthub/dolt#3218

Added character set and collation columns for show create view

@jennifersp jennifersp requested a review from fulghum June 6, 2022 23:34
Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Looks pretty close; just a couple questions.

sql/plan/show_create_table.go Show resolved Hide resolved
@@ -255,7 +266,13 @@ func (i *showCreateTablesIter) produceCreateTableStatement(ctx *sql.Context, tab

// TODO: The columns that are rendered in defaults should be backticked
if col.Default != nil {
stmt = fmt.Sprintf("%s DEFAULT %s", stmt, col.Default.String())
def := col.Default.String()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know how the double-quotes get stored? I noticed in one of the test cases that the input statement used single quotes, but was output wrapped in double-quotes.

Copy link
Contributor Author

@jennifersp jennifersp Jun 7, 2022

Choose a reason for hiding this comment

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

I think go stores string value like val = "example", and when this gets used with String() method of literal expressions, we use %q for fmt.Sprintf method for string type, which double quotes the double quotes, which makes it "\"example\"".
https://github.com/dolthub/go-mysql-server/blob/main/sql/expression/literal.go#L68

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there something we should do at this level to prevent double quotes from being stored? I haven't dug through this code yet to understand it deeply, but I'm curious what you think. Do you see a good reason we're storing the double quotes?

Overall, I think this fix is okay, but just wanted to understand the rest of the code. I'm specifically wondering if there are other problems from storing the double quotes that we could fix if we looked a little deeper here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One issue I've worked on before that is related to this double quoted literal expression was for displaying column default value in information_schema.columns table. Everytime the literal expression .String() method was called, it was adding a pair of double quotes. We use this method in FromDoltSchema method as well as in information_schema.columns table iter, which is why the result in the columns table has at least two pairs of double quotes. To resolve this, Vinai added code where the columns table will go through analyzer and the column default values are resolved accordingly which gets rid of extra double quotes, but still ends up with one extra pair of double quotes that gets trimmed in trimColumnDefaultOutput method in columns_table.go

enginetest/queries/create_table_queries.go Show resolved Hide resolved
Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for digging deeper into the literal value quoting behavior and finding a cleaner solution for this. Awesome to tidy up information_schema.columns a little more along the way, too. 😄

It would be good to go ahead and run the dolt tests against this GMS code to see if there are any dolt tests to patch up.

@jennifersp
Copy link
Contributor Author

Added changes for requiring parentheses around default function expressions. To do this,

  • we catch some cases of functions expressions without enclosing parentheses in parser such as GEOMETRY DEFAULT POINT(1,2) is returns false for ParenExpr interface check but has children which means it's functions expression.
  • the other set of cases are caught during analyzing the query as we need more information on column type, and some exceptions such as allowing NOW() and CURRENT_TIMESTAMP() without enclosing parentheses for datetime and timestamp column types only are validated in analyzer.

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Code changes look good. Just two questions to make sure we've got good test coverage and two really minor suggestions.

enginetest/queries/queries.go Show resolved Hide resolved
sql/plan/show_create_table_test.go Show resolved Hide resolved
sql/information_schema/columns_table.go Outdated Show resolved Hide resolved
sql/parse/parse.go Outdated Show resolved Hide resolved
@jennifersp jennifersp merged commit 51dc04e into main Jun 14, 2022
@jennifersp jennifersp deleted the jennifer/default branch June 14, 2022 16:30
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.

dolt dump has incorrect default values for create table statements
2 participants