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

Add support for MySQL auto_increment offset #950

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

ehoeve
Copy link
Contributor

@ehoeve ehoeve commented Aug 17, 2023

This should also fix #920.

@ehoeve ehoeve changed the title This should also fix issue #920. Add support for MySQL auto_increment offset Aug 17, 2023
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.

Nice work, thank you @ehoeve

My only suggestion is to have the name more closely match the sql syntax, though I may have missed something.

Let me know and I'll merge this PR in!

Thanks again

@@ -66,6 +66,7 @@ pub struct CreateTableBuilder {
pub clone: Option<ObjectName>,
pub engine: Option<String>,
pub comment: Option<String>,
pub autoincrement_offset: Option<u32>,
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 any reason to use a different style than the original auto_increment? I wonder if you considered:

Suggested change
pub autoincrement_offset: Option<u32>,
pub auto_increment_offset: Option<u32>,

?

If so can you explain why you chose this way?

I was thinking that following the exisiting convention would make the relevant code / field easier to search for (aka you can search for auto_increment in the code to see how auto_increment in SQL would be parsed.

Copy link
Contributor Author

@ehoeve ehoeve Aug 21, 2023

Choose a reason for hiding this comment

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

@alamb Thanks for the feedback.
No specific reason, your suggestion of more closely following convention is a better option I think, so I updated the PR

@@ -3550,6 +3550,17 @@ impl<'a> Parser<'a> {
None
};

let autoincrement_offset = if self.parse_keyword(Keyword::AUTO_INCREMENT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

https://dev.mysql.com/doc/refman/8.0/en/create-table.html

I got super confused for a moment because AUTO_INCREMENT can occur either as part of the table options (as this PR correctly does)


table_options:
    table_option [[,] table_option] ...

table_option: {
    AUTOEXTEND_SIZE [=] value
  | AUTO_INCREMENT [=] value
  | AVG_ROW_LENGTH [=] value
...

Or also as part of the column definition


column_definition: {
    data_type [NOT NULL | NULL] [DEFAULT {literal | (expr)} ]
      [VISIBLE | INVISIBLE]
      [AUTO_INCREMENT] [UNIQUE [KEY]] [[PRIMARY] KEY]
...

Which SQLParser already handles
https://github.com/sqlparser-rs/sqlparser-rs/blob/41e47cc0136c705a3335b1504d50ae8b22711b86/src/ast/ddl.rs#L587

@alamb alamb merged commit 9500649 into apache:main Aug 21, 2023
9 checks passed
@alamb
Copy link
Contributor

alamb commented Aug 21, 2023

Thanks @ehoeve

serprex pushed a commit to serprex/sqlparser-rs that referenced this pull request Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants