Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
2 changes: 2 additions & 0 deletions src/ast/ddl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2361,6 +2361,8 @@ pub struct CreateIndex {
pub name: Option<ObjectName>,
#[cfg_attr(feature = "visitor", visit(with = "visit_relation"))]
pub table_name: ObjectName,
/// Index type used in the statement. Can also be found inside [`CreateIndex::index_options`]
/// depending on the position of the option within the statement.
pub using: Option<IndexType>,
pub columns: Vec<IndexColumn>,
pub unique: bool,
Expand Down
24 changes: 19 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 Expand Up @@ -7118,6 +7123,15 @@ impl<'a> Parser<'a> {
// parse it anyway (as we do inside `ALTER TABLE` and `CREATE TABLE` parsing).
let index_options = self.parse_index_options()?;

// Only keep the latest USING, we don't take the IndexOption out of the vec and place the
// value in the `using` var because we try to keep the order of the options as they appear.
if index_options
.iter()
.any(|opt| matches!(opt, IndexOption::Using(_)))
{
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.

USING can be used in 3 different positions across the other dialects. This makes sure that only the latest USING is recognized.

// MySQL allows `ALGORITHM` and `LOCK` options. Unlike in `ALTER TABLE`, they need not be comma separated.
let mut alter_options = Vec::new();
while self
Expand Down
43 changes: 43 additions & 0 deletions tests/sqlparser_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17246,3 +17246,46 @@ fn parse_invisible_column() {
_ => panic!("Unexpected statement {stmt}"),
}
}

#[test]
fn parse_create_index_different_using_positions() {
let sql = "CREATE INDEX idx_name USING BTREE ON table_name (col1)";
let expected = "CREATE INDEX idx_name ON table_name USING BTREE (col1)";
match all_dialects().one_statement_parses_to(sql, expected) {
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!(),
}

let sql = "CREATE INDEX idx_name USING BTREE ON table_name (col1) USING HASH";
let expected = "CREATE INDEX idx_name ON table_name(col1) USING HASH";
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I followed the intent, why do we drop the first clause vs keeping both as in the input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but it's just super confusing because there is the USING field and another IndexOption::IndexType in the index_options field.

The point here is to mimic mysql behaviour of only keeping the second, not preserve the input.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm what's confusing do you mean with the first clause going into the using field and the second clause going into the index_options field?
It feels like it'd be unusual for the parser to swallow some of its input, and in general we avoid mimicking server behavior in the parser, so its currently unclear to me what the rationale is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can keep both then, but it will be confusing for users having two index types.

And while I don't like how we have two separate fields for that, I really don't want to hold this PR any longer

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

match all_dialects().one_statement_parses_to(sql, expected) {
Statement::CreateIndex(CreateIndex {
name,
table_name,
columns,
index_options,
..
}) => {
assert_eq!(name.unwrap().to_string(), "idx_name");
assert_eq!(table_name.to_string(), "table_name");
assert_eq!(columns.len(), 1);
assert!(index_options
.iter()
.any(|o| o == &IndexOption::Using(IndexType::Hash)));
}
_ => unreachable!(),
}
}