Skip to content

feat: allow multiple actions in one ALTER TABLE statement #960

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

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

ForbesLindesay
Copy link
Contributor

@ForbesLindesay ForbesLindesay commented Aug 29, 2023

Fixes #675

I primarily focused on ensuring this follows the Postgres syntax: https://www.postgresql.org/docs/15/sql-altertable.html

This should largely work for MySql too though: https://dev.mysql.com/doc/refman/8.0/en/alter-table.html

One possible thing that could be worth considering is that MySql supports a list of "partition_options" after the "alter_options" and while the "alter_options" are comma-separated just like Postgres, the "partition_options" are separated by a space. I'm not sure how best to cleanly represent this in our current AST.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6014479206

  • 189 of 202 (93.56%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 87.2%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/test_utils.rs 12 13 92.31%
src/ast/mod.rs 11 14 78.57%
src/parser/mod.rs 86 95 90.53%
Totals Coverage Status
Change from base Build 5978153931: -0.02%
Covered Lines: 16214
Relevant Lines: 18594

💛 - Coveralls

@alamb
Copy link
Contributor

alamb commented Sep 7, 2023

the "partition_options" are separated by a space. I'm not sure how best to cleanly represent this in our current AST

Could they be represented by a Vec<Partition> or something similar?

Interestingly #959 is also related to mysql partition syntax

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.

This looks great -- thank you @ForbesLindesay 🦾

Sorry for the delay in reviewing -- I was away last week.

assert_eq!("foo", column_def.name.to_string());
assert_eq!("TEXT", column_def.data_type.to_string());
}
_ => unreachable!(),
};

let rename_table = "ALTER TABLE tab RENAME TO new_tab";
match verified_stmt(rename_table) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice refactoring of the tests -- thank you

@@ -588,6 +584,48 @@ fn parse_alter_table_alter_column() {
}
}

#[test]
fn parse_alter_table_add_columns() {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

self.peek_token(),
);
};
let operations = self.parse_comma_separated(Parser::parse_alter_table_operation)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@alamb alamb changed the title feat: allow multiple actions in one alter-table statement feat: allow multiple actions in one ALTER TABLE statement Sep 7, 2023
@alamb alamb merged commit 25e037c into apache:main Sep 7, 2023
@ForbesLindesay ForbesLindesay deleted the feat/fl/alter-table branch September 20, 2023 14:26
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
Development

Successfully merging this pull request may close these issues.

adding multiple columns in a single alter table statement doesn't work
3 participants