-
Notifications
You must be signed in to change notification settings - Fork 652
Added support for MATCH syntax and unified column option ForeignKey #2062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Added support for MATCH syntax and unified column option ForeignKey #2062
Conversation
src/parser/mod.rs
Outdated
if self.parse_keyword(Keyword::CONSTRAINT) { | ||
let name = Some(self.parse_identifier()?); | ||
if let Some(option) = self.parse_optional_column_option()? { | ||
if let Some(option) = self.parse_optional_column_option(&col_name)? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what this change implies, it looks like we'd be storing the column name twice in the AST node which doesn't seem ideal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is needed to standardize ForeignKeyConstraint
and avoid having a duplicated struct to represent the same type of concept. I could possibly leave the columns
vector of the ForeignKeyConstraint
empty, but when I use that struct in code that works on top of the AST it is quite useful to have the column ident defined there, and not have to handle the case of a special ForeignKeyConstraint
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will note that it is the same change needed for PR #2064, which I did separately but the goal is identical - to standardize the structs used to represent constraints in the columns and table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, the goal of the AST is to represent the syntax as closely as possible so that I don't think that storing the column name in another node in the tree than where it was found seems reasonable. haven't looked closely at the different foreign key constraint variants but I would assume that if both variants show up in different parts of the syntax then its not unreasonable that they are represented differently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with that, but this philosophy of using the most strictly compatible AST nodes in the different parts of the tree has already been broken several times to reduce code duplication.
Here I am quite unsure whether the most appropriate solution would be to:
- use the table constraint struct and duplicate the column ident
- use the table constraint struct and leave its columns attribute empty
- use a new struct to strictly represent the column option for this constraint variant (for the same reasons of having structs instead of struct variants)
I would personally be inclined with 1 or 2, but I understand the duplication concerns of 1. Btw, is there any particular reason for the current clone-based approach for idents instead of a copy and lifetime based one?
Lmk your opinion regarding how do you feel best proceeding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went for solution number (2), let me know whether you agree with it.
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
This PR adds support for the SQL standard
MATCH [FULL | PARTIAL | SIMPLE]
syntax for foreign key constraints.Changes
PARTIAL
andSIMPLE
MatchKind
enum withFull
,Partial
, andSimple
variantsMATCH
syntax in both column-level and table-level foreign key constraintsColumnOption::ForeignKey
to useForeignKeyConstraint
struct, eliminating code duplicationExamples
Closes issue #2061 and works towards closing issue #2059