Skip to content

Conversation

mvzink
Copy link
Contributor

@mvzink mvzink commented Jun 20, 2025

Index column lists generally allow for more flexibility than just column names: i.e. ASC/DESC modifiers, Postgres opclasses, MySQL column prefix length, and generic expressions/"functional key parts".

This change uses the existing support for these constructs added in #1707 for CREATE INDEX and changes the AST for ALTER TABLE <table> ADD KEY (and associated variants), and constraints present in CREATE TABLE statements to support these constructs in those location.

Note that, as was already the case with existing CREATE INDEX support, there is no special representation for MySQL column prefix length (INDEX (textcol(10))), nor do we require the enclosing parentheses for MySQL functional key parts (INDEX ((col1 + col2)); for Postgres, this is optional if it doesn't introduce ambiguity). Instead, these are parsed generically as expressions, so the former is parsed as a function call and the latter is wrapped in Expr::Nested.

Also note that, as far as I can tell, no dialect supports these more general column expressions in the case of FOREIGN KEY constraints, so the parsing and AST for foreign keys are left unchanged.

Index column lists generally allow for more flexibility than just column
names: i.e. `ASC`/`DESC` modifiers, Postgres opclasses, MySQL column
prefix length, and generic expressions/"functional key parts".

This change uses the existing support for these constructs added in
apache#1707 for `CREATE INDEX` and changes the AST for `ALTER TABLE <table>
ADD KEY` (and associated variants), and constraints present in `CREATE
TABLE` statements to support these constructs in those location.

Note that, as was already the case with existing `CREATE INDEX` support,
there is no special representation for MySQL column prefix length
(`INDEX (textcol(10))`), nor do we require the enclosing parentheses for
MySQL functional key parts (`INDEX ((col1 + col2))`; for Postgres, this
is optional if it doesn't introduce ambiguity). Instead, these are
parsed generically as expressions, so the former is parsed as a function
call and the latter is wrapped in `Expr::Nested`.

Also note that, as far as I can tell, no dialect supports these more
general column expressions in the case of `FOREIGN KEY` constraints, so
the parsing and AST for foreign keys are left unchanged.
@mvzink mvzink force-pushed the push-zwmpustnnkzo branch from 69888cc to f9f0f56 Compare June 23, 2025 17:46
@mvzink
Copy link
Contributor Author

mvzink commented Jun 23, 2025

Having started using this, I noticed that it's actually fairly inconvenient to have the TableConstraint::ForeignKey variant have a different type for columns (e.g. if you want to implement get_columns(constraint: &TableConstraint) -> &[???]). Thoughts on using IndexColumn for TableConstraint::ForeignKey as well, even though it should aways only be plain column names?

@iffyio
Copy link
Contributor

iffyio commented Jun 24, 2025

Ah yeah that's not ideal when consuming the AST, but I think we could leave the foreign key representation as Ident since it maps to what's expected at that level - it would be one less thing for downstream crates to validate in the AST

Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @mvzink!
cc @alamb

@iffyio iffyio merged commit 5d63663 into apache:main Jun 24, 2025
10 checks passed
@mvzink mvzink deleted the push-zwmpustnnkzo branch August 5, 2025 17:54
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.

2 participants