Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
15 changes: 10 additions & 5 deletions src/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7061,19 +7061,24 @@ impl<'a> Parser<'a> {
pub fn parse_create_index(&mut self, unique: bool) -> Result<Statement, ParserError> {
let concurrently = self.parse_keyword(Keyword::CONCURRENTLY);
let if_not_exists = self.parse_keywords(&[Keyword::IF, Keyword::NOT, Keyword::EXISTS]);

let mut using = None;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a fan of this mut but also not a fan of returning a tuple of (index_name, using) at line 7067.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems okay IMO.


let index_name = if if_not_exists || !self.parse_keyword(Keyword::ON) {
let index_name = self.parse_object_name(false)?;
// MySQL allows `USING index_type` either before or after `ON table_name`
using = self.parse_optional_using_then_index_type()?;
self.expect_keyword_is(Keyword::ON)?;
Some(index_name)
} else {
None
};

let table_name = self.parse_object_name(false)?;
let using = if self.parse_keyword(Keyword::USING) {
Some(self.parse_index_type()?)
} else {
None
};

// MySQL allows having two `USING` clauses.
// In that case, the second clause overwrites the first.
using = self.parse_optional_using_then_index_type()?.or(using);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be something like

using = using.or_else(|| self.parse_optional_using_then_index_type()).transpose()?;

in that we should only look to parse again if we didn't parse a value already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As Michael pointed out earlier, MySQL does allow having two "USING" clauses. In that case, the second one overwrites the first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth a comment on this line, because at first glance (or if you haven't tried it on MySQL yourself) it does seem backwards 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, double checking do the tests include the scenario where two USING clauses are specified? If not we can probably add those

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Added a second test with two USING clauses. Force-pushed to resolve conflicts and squashed the commits.


let columns = self.parse_parenthesized_index_column_list()?;

Expand Down
44 changes: 44 additions & 0 deletions tests/sqlparser_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17246,3 +17246,47 @@ fn parse_invisible_column() {
_ => panic!("Unexpected statement {stmt}"),
}
}

#[test]
fn parse_create_index_using_before_on() {
let sql = "CREATE INDEX idx_name USING BTREE ON table_name (col1)";
// Can't use `verified_stmt` here as the USING will be placed after the `ON` clause
match all_dialects().parse_sql_statements(sql).unwrap()[0].clone() {
Copy link
Contributor

Choose a reason for hiding this comment

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

for the tests, I think we can use one_statement_parses_to() instead so that the test covers the display impl as well?
Also can we merge both test functions into the same parse_create_index_using() or similar function - since they cover the same feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated tests and one other small change for the third position of USING.

Statement::CreateIndex(CreateIndex {
name,
table_name,
using,
columns,
unique,
..
}) => {
assert_eq!(name.unwrap().to_string(), "idx_name");
assert_eq!(table_name.to_string(), "table_name");
assert_eq!(using, Some(IndexType::BTree));
assert_eq!(columns.len(), 1);
assert!(!unique);
}
_ => unreachable!(),
}
}

#[test]
fn parse_create_index_using_multiple_clauses() {
let sql = "CREATE INDEX idx_name USING BTREE ON table_name USING HASH (col1)";
// Can't use `verified_stmt` here as the first USING will be ignored
match all_dialects().parse_sql_statements(sql).unwrap()[0].clone() {
Statement::CreateIndex(CreateIndex {
name,
table_name,
using,
columns,
..
}) => {
assert_eq!(name.unwrap().to_string(), "idx_name");
assert_eq!(table_name.to_string(), "table_name");
assert_eq!(using, Some(IndexType::Hash));
assert_eq!(columns.len(), 1);
}
_ => unreachable!(),
}
}