Skip to content

Fix create statement for mysql #602

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

Closed
wants to merge 11 commits into from
Closed

Fix create statement for mysql #602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f86ed2f
Fix on update current_timestamp for mysql
RainJoe Sep 3, 2022
cb1d2ab
Add test for mysql on update current_timestamp
RainJoe Sep 3, 2022
311a771
Add index for mysql create statement
RainJoe Sep 5, 2022
bca276e
Add index for mysql create statement
RainJoe Sep 5, 2022
3aaaf24
Add index test for mysql create statement
RainJoe Sep 5, 2022
36be43a
Fix create statement by using mysql specific dialect
RainJoe Sep 5, 2022
7521106
Fix unique key for mysql
RainJoe Sep 5, 2022
033d183
Fix unique key for mysql
RainJoe Sep 5, 2022
b0d1159
Add test for mysql unique key
RainJoe Sep 5, 2022
7afaedf
Add mysql key/index suuport in create table statement
RainJoe Oct 2, 2022
72703ce
Fix table constraint unique case for mysql create statement
RainJoe Oct 2, 2022
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
73 changes: 66 additions & 7 deletions src/ast/ddl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,14 @@ impl fmt::Display for AlterColumnOperation {
}
}

/// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

@RainJoe RainJoe Oct 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Author

Choose a reason for hiding this comment

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

Of course not. waitting for your approach.

Copy link
Contributor

@AugustoFKL AugustoFKL Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Sorry the delay, busy week at work + college

Copy link
Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Unique,
Key,
Index,
}
/// A table-level constraint, specified in a `CREATE TABLE` or an
/// `ALTER TABLE ADD <constraint>` statement.
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
Expand Down Expand Up @@ -247,6 +255,19 @@ pub enum TableConstraint {
name: Option<Ident>,
expr: Box<Expr>,
},
/// Index in constraint, for mysql specific dialect.
/// Deal with this pattern:
/// 1. {INDEX | KEY} [index_name] [index_type] (key_part,...)
/// 2. [CONSTRAINT [symbol]] UNIQUE [INDEX | KEY]
/// [index_name] [index_type] (key_part,...)
/// [index_option]
///
Key {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Author

@RainJoe RainJoe Oct 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

name: Option<Ident>,
columns: Vec<Ident>,
/// key format, eg: unique/key/index
format: KeyFormat,
},
}

impl fmt::Display for TableConstraint {
Expand All @@ -256,13 +277,38 @@ impl fmt::Display for TableConstraint {
name,
columns,
is_primary,
} => write!(
f,
"{}{} ({})",
display_constraint_name(name),
if *is_primary { "PRIMARY KEY" } else { "UNIQUE" },
display_comma_separated(columns)
),
} => {
write!(
f,
"{}{} ({})",
display_constraint_name(name),
if *is_primary { "PRIMARY KEY" } else { "UNIQUE" },
display_comma_separated(columns)
)
}
TableConstraint::Key {
name,
columns,
format,
} => {
match *format {
KeyFormat::Unique => {
write!(f, "UNIQUE ")?;
}
KeyFormat::Key => {
write!(f, "KEY ")?;
}
KeyFormat::Index => {
write!(f, "INDEX ")?;
}
}
write!(
f,
"{} ({})",
display_key_name(name),
display_comma_separated(columns)
)
}
TableConstraint::ForeignKey {
name,
columns,
Expand Down Expand Up @@ -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 + '_ {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Author

@RainJoe RainJoe Oct 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

struct KeyName<'a>(&'a Option<Ident>);
impl<'a> fmt::Display for KeyName<'a> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
if let Some(name) = self.0 {
write!(f, "{}", name)?;
}
Ok(())
}
}
KeyName(name)
}

/// `<referential_action> =
/// { RESTRICT | CASCADE | SET NULL | NO ACTION | SET DEFAULT }`
///
Expand Down
2 changes: 1 addition & 1 deletion src/ast/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use serde::{Deserialize, Serialize};

pub use self::data_type::DataType;
pub use self::ddl::{
AlterColumnOperation, AlterTableOperation, ColumnDef, ColumnOption, ColumnOptionDef,
AlterColumnOperation, AlterTableOperation, ColumnDef, ColumnOption, ColumnOptionDef, KeyFormat,
ReferentialAction, TableConstraint,
};
pub use self::operator::{BinaryOperator, UnaryOperator};
Expand Down
41 changes: 38 additions & 3 deletions src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2526,9 +2526,15 @@ impl<'a> Parser<'a> {
} else if self.parse_keywords(&[Keyword::ON, Keyword::UPDATE])
&& dialect_of!(self is MySqlDialect)
{
Ok(Some(ColumnOption::DialectSpecific(vec![
Token::make_keyword("ON UPDATE"),
])))
if self.parse_keyword(Keyword::CURRENT_TIMESTAMP) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did it change?

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

Copy link
Author

Choose a reason for hiding this comment

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

I mentioned it in my first comment.

Ok(Some(ColumnOption::DialectSpecific(vec![
Token::make_keyword("ON UPDATE CURRENT_TIMESTAMP"),
])))
} else {
Ok(Some(ColumnOption::DialectSpecific(vec![
Token::make_keyword("ON UPDATE"),
])))
}
} else {
Ok(None)
}
Expand Down Expand Up @@ -2562,10 +2568,39 @@ impl<'a> Parser<'a> {
None
};
match self.next_token() {
Token::Word(w)
if (w.keyword == Keyword::KEY || w.keyword == Keyword::INDEX)
&& dialect_of!(self is MySqlDialect) =>
{
let name = Some(self.parse_identifier()?);
let columns = self.parse_parenthesized_column_list(Mandatory)?;
match w.keyword {
Keyword::KEY => Ok(Some(TableConstraint::Key {
name,
columns,
format: KeyFormat::Key,
})),
Keyword::INDEX => Ok(Some(TableConstraint::Key {
name,
columns,
format: KeyFormat::Index,
})),
_ => unreachable!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

@RainJoe RainJoe Oct 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

}
}
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) && name == None {
//For mysql case: UNIQUE KEY (column_name), there is a little difference from CONSTRAINT constraint_name UNIQUE (column_name)
let name = Some(self.parse_identifier()?);
let columns = self.parse_parenthesized_column_list(Mandatory)?;
return Ok(Some(TableConstraint::Key {
name,
columns,
format: KeyFormat::Unique,
}));
}
let columns = self.parse_parenthesized_column_list(Mandatory)?;
Ok(Some(TableConstraint::Unique {
Expand Down
184 changes: 184 additions & 0 deletions tests/sqlparser_mysql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,6 +1016,190 @@ fn parse_table_colum_option_on_update() {
}
_ => unreachable!(),
}

let sql2 = "CREATE TABLE foo (`modification_time` DATETIME ON UPDATE CURRENT_TIMESTAMP)";
match mysql().verified_stmt(sql2) {
Statement::CreateTable { name, columns, .. } => {
assert_eq!(name.to_string(), "foo");
assert_eq!(
vec![ColumnDef {
name: Ident::with_quote('`', "modification_time"),
data_type: DataType::Datetime,
collation: None,
options: vec![ColumnOptionDef {
name: None,
option: ColumnOption::DialectSpecific(vec![Token::make_keyword(
"ON UPDATE CURRENT_TIMESTAMP"
)]),
},],
}],
columns
);
}
_ => unreachable!(),
}
}

#[test]
fn parse_table_create_constraint_option() {
let sql = "CREATE TABLE `foo` (`id` INT(11) NOT NULL, `node_id` INT(11) NOT NULL, `weight` INT(11) NOT NULL, `score` INT(11) NOT NULL, PRIMARY KEY (`id`), UNIQUE `node_id` (`node_id`), KEY `weight` (`weight`), INDEX `score` (`score`)) ENGINE=InnoDB";

match mysql().verified_stmt(sql) {
Statement::CreateTable {
name,
columns,
constraints,
..
} => {
assert_eq!(name.to_string(), "`foo`");
assert_eq!(
vec![
ColumnDef {
name: Ident::with_quote('`', "id"),
data_type: DataType::Int(Some(11)),
collation: None,
options: vec![ColumnOptionDef {
name: None,
option: ColumnOption::NotNull
},]
},
ColumnDef {
name: Ident::with_quote('`', "node_id"),
data_type: DataType::Int(Some(11)),
collation: None,
options: vec![ColumnOptionDef {
name: None,
option: ColumnOption::NotNull
}]
},
ColumnDef {
name: Ident::with_quote('`', "weight"),
data_type: DataType::Int(Some(11)),
collation: None,
options: vec![ColumnOptionDef {
name: None,
option: ColumnOption::NotNull
}]
},
ColumnDef {
name: Ident::with_quote('`', "score"),
data_type: DataType::Int(Some(11)),
collation: None,
options: vec![ColumnOptionDef {
name: None,
option: ColumnOption::NotNull
}]
}
],
columns
);
assert_eq!(
vec![
TableConstraint::Unique {
name: None,
columns: vec![Ident::with_quote('`', "id")],
is_primary: true
},
TableConstraint::Key {
name: Some(Ident::with_quote('`', "node_id")),
columns: vec![Ident::with_quote('`', "node_id")],
format: KeyFormat::Unique
},
TableConstraint::Key {
name: Some(Ident::with_quote('`', "weight")),
columns: vec![Ident::with_quote('`', "weight")],
format: KeyFormat::Key
},
TableConstraint::Key {
name: Some(Ident::with_quote('`', "score")),
columns: vec![Ident::with_quote('`', "score")],
format: KeyFormat::Index
},
],
constraints
);
}
_ => unreachable!(),
}

let sql2 = "CREATE TABLE `foo` (`id` INT(11) NOT NULL, `node_id` INT(11) NOT NULL, `weight` INT(11) NOT NULL, `score` INT(11) NOT NULL, PRIMARY KEY (`id`), CONSTRAINT `node_id` UNIQUE (`node_id`), KEY `weight` (`weight`), INDEX `score` (`score`)) ENGINE=InnoDB";
match mysql().verified_stmt(sql2) {
Statement::CreateTable {
name,
columns,
constraints,
..
} => {
assert_eq!(name.to_string(), "`foo`");
assert_eq!(
vec![
ColumnDef {
name: Ident::with_quote('`', "id"),
data_type: DataType::Int(Some(11)),
collation: None,
options: vec![ColumnOptionDef {
name: None,
option: ColumnOption::NotNull
},]
},
ColumnDef {
name: Ident::with_quote('`', "node_id"),
data_type: DataType::Int(Some(11)),
collation: None,
options: vec![ColumnOptionDef {
name: None,
option: ColumnOption::NotNull
}]
},
ColumnDef {
name: Ident::with_quote('`', "weight"),
data_type: DataType::Int(Some(11)),
collation: None,
options: vec![ColumnOptionDef {
name: None,
option: ColumnOption::NotNull
}]
},
ColumnDef {
name: Ident::with_quote('`', "score"),
data_type: DataType::Int(Some(11)),
collation: None,
options: vec![ColumnOptionDef {
name: None,
option: ColumnOption::NotNull
}]
}
],
columns
);
assert_eq!(
vec![
TableConstraint::Unique {
name: None,
columns: vec![Ident::with_quote('`', "id")],
is_primary: true
},
TableConstraint::Unique {
name: Some(Ident::with_quote('`', "node_id")),
columns: vec![Ident::with_quote('`', "node_id")],
is_primary: false,
},
TableConstraint::Key {
name: Some(Ident::with_quote('`', "weight")),
columns: vec![Ident::with_quote('`', "weight")],
format: KeyFormat::Key
},
TableConstraint::Key {
name: Some(Ident::with_quote('`', "score")),
columns: vec![Ident::with_quote('`', "score")],
format: KeyFormat::Index
},
],
constraints
);
}
_ => unreachable!(),
}
}

#[test]
Expand Down