Skip to content

Conversation

MohamedAbdeen21
Copy link
Contributor

MySQL allows specifying the index type USING index_type before the ON clause in CREATE INDEX statements.

This PR allows the CREATE INDEX parser to accept both positions of the USING clause, regardless of the dialect.

docs: https://dev.mysql.com/doc/refman/8.4/en/create-index.html

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 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

Choose a reason for hiding this comment

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

Seems okay IMO.

Copy link
Contributor

@mvzink mvzink left a comment

Choose a reason for hiding this comment

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

Great, thanks!

None
};

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.

@iffyio iffyio changed the title MySQL: allow USING clause before ON in CREATE INDEX MySQL: CREATE INDEx: allow USING clause before ON Sep 19, 2025
MySQL allows specifying the index type `USING index_type`
before the `ON` clause in `CREATE INDEX` statements.

This PR allows the `CREATE INDEX` parser to accept both
positions of the `USING` clause, regardless of the dialect.

docs: https://dev.mysql.com/doc/refman/8.4/en/create-index.html
Comment on lines 17253 to 17254
// 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.

{
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.

Comment on lines 17272 to 17273
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

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