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

Support MySQL UNIQUE table constraint #1164

Merged
merged 9 commits into from
Apr 9, 2024

Conversation

Nikita-str
Copy link
Contributor

Fix to #1149 and some more.

Allow to parse [CONSTRAINT [name]] UNIQUE [INDEX | KEY] [index_name] [index_type] (<columns>) [index_option]s ... as it say in the specification.

Also unallow CONSTRAINT bar INDEX ind (foo) for table constraint; as it was said INDEX ind (foo) form of table constraint was found only in MySQL, but in MySQL allowed only INDEX ind (foo) without CONSTRAINT bar before it.

Some remarks:

  • Maybe it have sense to split TableConstraint::Unique into TableConstraint::Unique & TableConstraint::Primary?
  • In this PR was created IndexOption struct, and maybe it can be unite with some another structure, I unsure.
  • In MySQL CONSTRAINT a UNIQUE b (x) will set b and for index_name(in MySQL you can obtain it by SELECT index_name FROM INFORMATION_SCHEMA.STATISTICS) and for constraint_name(in MySQL you can obtain it by SELECT constraint_name FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS) so I don't sure if names really needed to be splitted.

Also
* use different names for constraint name and index name.
In MySQL they can be obtained by `SELECT index_name FROM INFORMATION_SCHEMA.STATISTICS` and  `SELECT constraint_name FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS`. But in MySQL `CONSTRAINT `a` UNIQUE `b` (x)` will set `b` and for `index_name` and for `constraint_name` so I don't sure it's it really needed to split them or not.
* Allow use `INDEX` after `UNIQUE`

Currently allowed index options:
* `USING {HASH | BTREE}`
* `COMMENT 'string'`
As it said `INDEX ind (foo)` form of constraint was found only in MySQL, but in MySQL allowed only `INDEX ind (foo)` without `CONSTRAINT bar` so if there was `name` then unallow `INDEX` constraint.
@coveralls
Copy link

coveralls commented Mar 11, 2024

Pull Request Test Coverage Report for Build 8621622983

Details

  • 186 of 198 (93.94%) changed or added relevant lines in 3 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 88.034%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ast/ddl.rs 44 49 89.8%
src/parser/mod.rs 52 59 88.14%
Files with Coverage Reduction New Missed Lines %
src/parser/mod.rs 3 82.88%
Totals Coverage Status
Change from base Build 8588850954: 0.03%
Covered Lines: 20939
Relevant Lines: 23785

💛 - Coveralls

@alamb
Copy link
Contributor

alamb commented Apr 7, 2024

Maybe it have sense to split TableConstraint::Unique into TableConstraint::Unique & TableConstraint::Primary?

Yes, I agree this would make more sense than trying to combine UNIQUE and PRIMARY KEY syntaxes together

Are you willing to make this change? I am really sorry for the delay in reviewing

@Nikita-str
Copy link
Contributor Author

I do the split, but now I'm unsure that it was good idea, because MySql specification say that there no [index_name] in PRIMARY KEY variant but actually compiler take CREATE TABLE table_name (x INT, CONSTRAINT name PRIMARY KEY index_name (x)); as valid query. So PRIMARY KEY and UNIQUE have less differences than I expected.

Their similarity is visible in tests during creation and during assertion of inner structure.

But still maybe when they are separate it's more expected by an user, idk.

@Nikita-str
Copy link
Contributor Author

Also during last changes I add some functions like display_constraint_name but for Option<T>.

For example instead next

if let Some(exclude) = &self.opt_exclude {
    write!(f, " {exclude}")?;
}
if let Some(except) = &self.opt_except {
    write!(f, " {except}")?;
}
if let Some(rename) = &self.opt_rename {
    write!(f, " {rename}")?;
}
if let Some(replace) = &self.opt_replace {
    write!(f, " {replace}")?;
}

you can write

write!(
    f, 
    "{}{}{}{}",
    display_option_spaced(&self.opt_exclude),
    display_option_spaced(&self.opt_except),
    display_option_spaced(&self.opt_rename),
    display_option_spaced(&self.opt_replace),
)?;

I think it will be pretty convenient for fmt

@alamb
Copy link
Contributor

alamb commented Apr 9, 2024

But still maybe when they are separate it's more expected by an user, idk.

I agree -- the sqlparser's crate aim is to preserve syntax and it is up downstream consumers to decide how to handle the semantics (e.g if they want to treat UNIQUE and PRIMARY the same)

@alamb alamb changed the title MySQL UNIQUE table constraint fix Support MySQL UNIQUE table constraint Apr 9, 2024
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you very much @Nikita-str -- I pushed a few small comment improvements and then I'll merge this PR in.

Thanks again and sorry for the delay in reviewing

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

/// If `option` is
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I agree this is very nice

@alamb alamb merged commit 8f67d1a into apache:main Apr 9, 2024
10 checks passed
@alamb
Copy link
Contributor

alamb commented Apr 9, 2024

Thanks again @Nikita-str

JichaoS pushed a commit to luabase/sqlparser-rs that referenced this pull request May 7, 2024
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.

3 participants