Skip to content

Add support for "on delete cascade" column option - #170

Merged
nickolay merged 7 commits into
apache:masterfrom
c7hm4r:on_delete_option
May 27, 2020
Merged

Add support for "on delete cascade" column option#170
nickolay merged 7 commits into
apache:masterfrom
c7hm4r:on_delete_option

Conversation

@c7hm4r

@c7hm4rc7hm4r commented May 26, 2020

Copy link
Copy Markdown
Contributor

This introduces some breaking changes.

Is it a good idea to have a lifetime parameter in Parser?

nickolay
nickolay suggested changes May 26, 2020
View reviewed changes

@nickolaynickolay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this! I'd prefer to split c67d667 into a separate PR, so that this PR could be merged independently, and the discussion about evolving our dialect support would be more visible.

As for ON { UPDATE | DELETE } { RESTRICT | CASCADE | SET NULL | NO ACTION | SET DEFAULT }, please see my comments inline...

(Note: ANSI SQL calls a <referential triggered action>, and here are the Postgres docs.)

Comment thread.gitignore
Comment threadsrc/ast/ddl.rs Outdated
Comment threadsrc/ast/ddl.rs Outdated
Comment threadsrc/ast/ddl.rs Outdated
@c7hm4r
c7hm4rforce-pushed the on_delete_option branch 3 times, most recently from 2fbdaea to 79514b7CompareMay 26, 2020 20:11
@c7hm4r

c7hm4r commented May 26, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

I reworked the PR. I will create a new PR for the new dialect option.

@c7hm4r
c7hm4r requested a review from nickolayMay 26, 2020 20:20
nickolay
nickolay suggested changes May 26, 2020
View reviewed changes

@nickolaynickolay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Excellent, thank you! I have only a few nits.

Comment threadsrc/ast/ddl.rs Outdated
Comment threadsrc/ast/ddl.rs Outdated
Comment threadsrc/ast/ddl.rs Outdated
Comment threadsrc/parser.rs
Comment threadsrc/parser.rs Outdated
Comment threadsrc/parser.rs
@c7hm4r
c7hm4r marked this pull request as ready for review May 27, 2020 12:12
@c7hm4r
c7hm4r requested a review from nickolayMay 27, 2020 12:15
@nickolay
nickolay merged commit 98f97d0 into apache:masterMay 27, 2020
nickolay
nickolay reviewed May 27, 2020
View reviewed changes

@nickolaynickolay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perfect, thanks again!

@nickolaynickolay mentioned this pull request Jun 22, 2020
Closed
nickolay added a commit that referenced this pull request Aug 10, 2020
@eyalleshem@c7hm4r@nickolay
Enable dialect specific behaviours in the parser (#254)
1b46e82
* Change `Parser { ... }` to store the dialect used:
`Parser<'a> { ... dialect: &'a dyn Dialect }`
Thanks to @c7hm4r for the initial version of this submitted as
part of #170
* Introduce `dialect_of!(parser is SQLiteDialect | GenericDialect)` helper
to branch on the dialect's type
* Use the new functionality to make `AUTO_INCREMENT` and `AUTOINCREMENT`
parsing dialect-dependent.
Co-authored-by: Christoph Müller <pmzqxfmn@runbox.com>
Co-authored-by: Nickolay Ponomarev <asqueella@gmail.com>
@eyalleshemeyalleshem mentioned this pull request Aug 10, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@nickolaynickolaynickolay requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@c7hm4r@nickolay