Fix create statement for mysql #602

RainJoe · 2022-09-05T04:54:16Z

Add ON UPDATE CURRENT_TIMESTAMP support.
Add KEY/INDEX support

AugustoFKL · 2022-09-26T17:02:32Z

src/ast/ddl.rs

@@ -256,11 +263,38 @@ impl fmt::Display for TableConstraint {
                name,
                columns,
                is_primary,
+                is_mysql_unique_key,


This is a bad design that can turn this code huge when considering other dialects.

If you really need the dialect info here, maybe you could have the Dialect itself, not that level of specificity here.

Since there are 3 possible options, couldn't we use an enum variant? Like:

pub enum UniqueType { PrimaryKey, Key, Simple }

(ignore the horrible naming, but you got the idea hehe)

Use enum variant would not work, i think using specific dialect is a good solution, but when I add dialect trait in TableConstrait struct, the compiler says: the trait bound dyn dialect::Dialect: Clone is not satisfied。
There may not have explict constrait key word in mysql create statement, constraint keyword is optional, like:

CREATE TABLE table_name( column_name1 column_definition, column_name2 column_definition, ..., [CONSTRAINT constraint_name] UNIQUE(column_name1,column_name2) );

so use the origin display_constraint_name method not work，This is why I use is_mysql_unique_key to distinguish mysql dialect specifically.

I haven't been able to find an elegant way to solve it yet, any good idea?

Use enum variant would not work

Why would this not work?

If I remove is_mysql_unique_key and use the original display method, then run the parse_table_colum_option_key test, it would panic, the painc message show the difference:

thread parse_table_colum_option_key panicked at assertion failed: (left == right)
left: "CREATE TABLE foo (modification_time DATETIME ON UPDATE, UNIQUE KEY u_m_t (modification_time))",
right: "CREATE TABLE foo (modification_time DATETIME ON UPDATE, CONSTRAINT u_m_t UNIQUE (modification_time))", src/test_utils.rs:95:13

@RainJoe but you can still use an enum or something like that. Using a single boolean makes the expansion difficult, and doesn't make that much sense as it is really specific information added to the general AST. Using an enum there allows the upper levels to match and handle as desired when, for example, they're using the PostgreSQL dialect.

@AugustoFKL Thanks for your suggestion，I will make some modifications and create a now PR later.

alamb

Thank you for your contribution @RainJoe

I suggest following the idea of @AugustoFKL and using an enum variant to describe the different possible parsed versions of the CONSTRAINTs keyword.

I am not familiar with the differences in syntax -- perhaps if you could summarize the differences in the description of this PR we could offer some more helpful feedback

alamb · 2022-09-27T13:12:01Z

src/ast/ddl.rs

@@ -247,6 +249,11 @@ pub enum TableConstraint {
        name: Option<Ident>,
        expr: Box<Expr>,
    },
+    Key {


Can you please add comments here about what this field means / how it maps back to SQL?

@RainJoe please LMK if I'm wrong, but UNIQUE KEY isn't the same as a UNIQUE constraint, while only KEY refers to the same as CREATE INDEX?

Wouldn't be better to keep UNIQUE [KEY | INDEX] as a UNIQUE table constraint?

Yes, it's not same. I have fixed it, PTAL @AugustoFKL

alamb · 2022-09-27T13:12:08Z

src/ast/ddl.rs

@@ -228,6 +229,7 @@ pub enum TableConstraint {
        columns: Vec<Ident>,
        /// Whether this is a `PRIMARY KEY` or just a `UNIQUE` constraint
        is_primary: bool,
+        is_mysql_unique_key: bool,


Please add comments to this field explaining what it means (aka how is this different)

alamb · 2022-09-27T13:19:53Z

src/ast/ddl.rs

@@ -256,11 +263,38 @@ impl fmt::Display for TableConstraint {
                name,
                columns,
                is_primary,
+                is_mysql_unique_key,


Use enum variant would not work

Why would this not work?

coveralls · 2022-09-27T13:23:05Z

Pull Request Test Coverage Report for Build 3169255008

156 of 167 (93.41%) changed or added relevant lines in 3 files are covered.
845 unchanged lines in 12 files lost coverage.
Overall coverage increased (+0.1%) to 85.687%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser.rs	20	23	86.96%
src/ast/ddl.rs	20	28	71.43%

Files with Coverage Reduction	New Missed Lines	%
src/ast/ddl.rs	1	80.0%
src/dialect/hive.rs	1	94.12%
src/dialect/mod.rs	1	91.67%
src/test_utils.rs	1	83.58%
src/dialect/postgresql.rs	2	93.75%
tests/sqlparser_postgres.rs	5	97.72%
src/ast/data_type.rs	7	91.03%
src/ast/value.rs	8	88.24%
src/tokenizer.rs	46	89.42%
tests/sqlparser_common.rs	81	97.08%

Totals
Change from base Build 2957846519:	0.1%
Covered Lines:	10339
Relevant Lines:	12066

💛 - Coveralls

RainJoe · 2022-10-02T06:31:05Z

@AugustoFKL PTAL
I made some modifications at your suggestion.

AugustoFKL · 2022-10-02T15:09:15Z

src/ast/ddl.rs

@@ -247,6 +249,11 @@ pub enum TableConstraint {
        name: Option<Ident>,
        expr: Box<Expr>,
    },
+    Key {


@RainJoe please LMK if I'm wrong, but UNIQUE KEY isn't the same as a UNIQUE constraint, while only KEY refers to the same as CREATE INDEX?

Wouldn't be better to keep UNIQUE [KEY | INDEX] as a UNIQUE table constraint?

AugustoFKL · 2022-10-02T15:11:38Z

src/parser.rs

            Token::Word(w) if w.keyword == Keyword::PRIMARY || w.keyword == Keyword::UNIQUE => {
                let is_primary = w.keyword == Keyword::PRIMARY;
                if is_primary {
                    self.expect_keyword(Keyword::KEY)?;
+                } else if dialect_of!(self is MySqlDialect) {


Please add the GenericDialect here. The idea of that, as long as I understood by this comment, is to support everything possible.

It's hard to cover all dialects now. so using the specific dialect to avoid affecting other dialects.

@RainJoe THeGenericDialect is the one that contains literally EVERY single possible AST structure. Therefore, it should contemplate any possible query

dialect_of!(self is GenericDialect | MySqlDialect) should be enough

AugustoFKL · 2022-10-02T15:12:52Z

src/parser.rs

+                        columns,
+                        format: KeyFormat::Index,
+                    })),
+                    _ => unreachable!(),


Please replace this with an error. I left a possible approach for cases like that here

There would be only two cases, and both of them are covered, so it's actually unreachable.

Fine, but can't you return an error here?

It's better to have an error (like the self.expected(...)) than having a unreachable

AugustoFKL · 2022-10-02T15:13:33Z

src/parser.rs

-            Ok(Some(ColumnOption::DialectSpecific(vec![
-                Token::make_keyword("ON UPDATE"),
-            ])))
+            if self.parse_keyword(Keyword::CURRENT_TIMESTAMP) {


Why did it change?

It's not related to your PR, so shouldn't have changed at all

I mentioned it in my first comment.

AugustoFKL · 2022-10-02T15:14:19Z

src/ast/ddl.rs

@@ -428,6 +474,19 @@ fn display_constraint_name(name: &'_ Option<Ident>) -> impl fmt::Display + '_ {
    ConstraintName(name)
 }

+fn display_key_name(name: &'_ Option<Ident>) -> impl fmt::Display + '_ {


Too much complex, this isn't necessary at all...

Please can you try to make it simple? LMK if you need any help

It's similar to display_constraint_name methed, the name dose not implement display method, so i think it's necessary to do this.

AugustoFKL · 2022-10-02T15:15:29Z

src/ast/ddl.rs

+/// For mysql index, here may be there format for index, there patterns are similar。
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+pub enum KeyFormat {


As I stated in another comment, UNIQUE is a constraint. Please don't change the current handle structure for it, as this doesn't make sense. You're changing the AST to match only one dialect.

If you don't do this. this libarary would never support for mysql create statement. I think we should make it work first, and if someone have a better solution，we can apply it later.

@RainJoe would you mind if I tried a different approach and you took a look in a day or two?

Sorry for asking that, I know that this has been your work, don't want to look like throwing that away

Of course not. waitting for your approach.

@RainJoe @alamb could you please leave your opinion? #665

Sorry the delay, busy week at work + college

@AugustoFKL Thanks for your work, but it didn't solve my problem, maybe I would just use my fork repository. It's seems hard to consider all possible scenarios.

@RainJoe can you give examples of what you intend about this task?

The index usage itself is being parsed... What is the syntax you need?

alamb · 2022-10-19T21:22:32Z

Marking as draft as it seems to be waiting on feedback. Please mark as ready for review if/when it is

alamb · 2022-12-28T13:59:48Z

@RainJoe can you see if #685 works for you?

RainJoe added 9 commits September 3, 2022 17:53

Fix on update current_timestamp for mysql

f86ed2f

Add test for mysql on update current_timestamp

cb1d2ab

Add index for mysql create statement

311a771

Add index for mysql create statement

bca276e

Add index test for mysql create statement

3aaaf24

Fix create statement by using mysql specific dialect

36be43a

Fix unique key for mysql

7521106

Fix unique key for mysql

033d183

Add test for mysql unique key

b0d1159

AugustoFKL reviewed Sep 26, 2022

View reviewed changes

alamb reviewed Sep 27, 2022

View reviewed changes

Add mysql key/index suuport in create table statement

7afaedf

AugustoFKL reviewed Oct 2, 2022

View reviewed changes

Fix table constraint unique case for mysql create statement

72703ce

alamb marked this pull request as draft October 19, 2022 21:22

This was referenced Dec 28, 2022

fix: mysql ON UPDATE not taking an expression #586

Closed

Support CREATE TABLE ON UPDATE <expr> Function #685

Merged

alamb closed this Dec 28, 2022

Fix create statement for mysql #602

Fix create statement for mysql #602

Uh oh!

Conversation

RainJoe commented Sep 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RainJoe Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3169255008

💛 - Coveralls

Uh oh!

RainJoe commented Oct 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RainJoe Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RainJoe Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RainJoe Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AugustoFKL Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RainJoe Oct 2, 2022 •

edited

Loading

coveralls commented Sep 27, 2022 •

edited

Loading

RainJoe Oct 2, 2022 •

edited

Loading

RainJoe Oct 2, 2022 •

edited

Loading

RainJoe Oct 2, 2022 •

edited

Loading

AugustoFKL Oct 10, 2022 •

edited

Loading