Add support column prefix index for MySQL #1732
Conversation
src/ast/mod.rs
Outdated
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
| #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] | ||
| pub enum IndexExpr { | ||
| Column(Ident), | ||
| /// Mysql specific syntax | ||
| /// | ||
| /// See [Mysql documentation](https://dev.mysql.com/doc/refman/8.3/en/create-index.html) | ||
| /// for more details. | ||
| ColumnPrefix { | ||
| column: Ident, | ||
| length: u64, | ||
| }, | ||
| Functional(Box<Expr>), | ||
| } |
There was a problem hiding this comment.
would it be possible to replace this with an Expr instead? The docs seems to suggest that arbitrary expressions are allowed, from the docs
CREATE TABLE t1 (col1 INT, col2 INT, INDEX func_index ((ABS(col1))));
CREATE INDEX idx1 ON t1 ((col1 + col2));
CREATE INDEX idx2 ON t1 ((col1 + col2), (col1 - col2), col1);
ALTER TABLE t1 ADD INDEX ((col1 * 40) DESC);this would let us reuse existing code and representation, maybe even replace IndexField with OrderByExpr
There was a problem hiding this comment.
I agree that using OrderByExpr could be a good approach. However, I am somewhat concerned that users might find it challenging to distinguish between the different Expr when working with it.
There was a problem hiding this comment.
The different Expr I imagine are encoded by the Expr variants, I think it would be preferrable to using a custom repr here which looks to require duplicating a lot of Expr in order to be comprehensive
There was a problem hiding this comment.
That's right. I believe adding only ColumnPrefix will suffice. We can extend the Expr enum as follows:
enum Expr {
ColumnPrefix { column: Ident, length: u64 }
}There was a problem hiding this comment.
Hmm it looks like ColumnPrefix is equivalent to a function call with one argument? that can already be expressed by an Expr so that it doesn't seem like that should be necessary?
zzzdong
force-pushed
the
feature/column-prefix-index
branch
from
3ea8ed2 to
a362ed7
Compare
zzzdong
force-pushed
the
feature/column-prefix-index
branch
from
1613acf to
d2092d6
Compare
| pub fn parse_index_exprs(&mut self) -> Result<Vec<IndexExpr>, ParserError> { | ||
| self.parse_parenthesized(|p| p.parse_comma_separated(Parser::parse_index_expr)) | ||
| } | ||
|
|
||
| pub fn parse_index_expr(&mut self) -> Result<IndexExpr, ParserError> { |
There was a problem hiding this comment.
Since this is flagged as a public function can we add a short description mentioning what it does?
| let order_options = self.parse_order_by_options()?; | ||
| (None, order_options) | ||
| } else { | ||
| let operator_class = self.maybe_parse(|p| p.parse_expr())?; |
There was a problem hiding this comment.
hmm I couldn't see operator_class being used in the introduced tests or documented so its unclear what the syntax is?
|
|
||
| let (operator_class, order_options) = if self.peek_keyword(Keyword::ASC) | ||
| || self.peek_keyword(Keyword::DESC) | ||
| || self.peek_keyword(Keyword::NULLS) |
There was a problem hiding this comment.
can we introduce a test case that covers the ASC/DESC and nulls_first syntax?
| let (operator_class, order_options) = if self.peek_keyword(Keyword::ASC) | ||
| || self.peek_keyword(Keyword::DESC) | ||
| || self.peek_keyword(Keyword::NULLS) |
There was a problem hiding this comment.
Instead of using peek, we can probably use self.maybe_parse() so that we don't need to maintain this list of keywords in sync with the parse_order_by_options function e.g.
let (operator_class, order_options) = if let Some(order_options ) = self.maybe_parse(|p| p.parse_order_by_options())? {
(None, order_options)
} else {
let operator_class = self.maybe_parse(|p| p.parse_expr())?;
let order_options = self.parse_order_by_options()?;
(operator_class, order_options)
};| use std::vec; | ||
|
|
|
Considering the work on the index parsing in #1707, this will be closed. |
This pull request add support for column prefixe and functional indexes in MySQL's
CREATE TABLEandALTER TABLEstatements.May fix parts of #302 .