Skip to content

Conversation

@joshwd36
Copy link
Contributor

@joshwd36 joshwd36 commented Apr 6, 2021

This adds ON UPDATE and ON DELETE actions to sqlparser::ast::TableConstraint::ForeignKey, as described in table constraint and references specification.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 723771861

  • 35 of 41 (85.37%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 90.092%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 10 11 90.91%
tests/sqlparser_common.rs 14 15 93.33%
src/ast/ddl.rs 11 15 73.33%
Totals Coverage Status
Change from base Build 676410191: 0.04%
Covered Lines: 5492
Relevant Lines: 6096

💛 - Coveralls

@alamb
Copy link
Contributor

alamb commented Aug 20, 2021

Hi @joshwd36 -- sorry for the delay in review. I am going to help out now with this repo and we are working to clear the backlog. Is this PR still something you would like to work on to help contribute?

@joshwd36
Copy link
Contributor Author

Hi @joshwd36 -- sorry for the delay in review. I am going to help out now with this repo and we are working to clear the backlog. Is this PR still something you would like to work on to help contribute?

No worries at all. I do currently have a project that relies on my fork of this so it would be good to get it merged. Other than rebasing is there any other work that needs to be done on it?

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks great -- thank you very much @joshwd36

I think the code looks correct 👍 . Other than rebasing, the only thing I suggest for this PR is possibly add tests for the following cases:

  1. Alternate clause order ON UPDATE ... ON DELETE (I only see a test for ON DELETE .. ON UPDATE..)
  2. Error: duplicated ON DELETE clause e.g. ... ON DELETE CASCADE ON DELETE CASCADE

Thoughts @Dandandan / @andygrove ?

@joshwd36
Copy link
Contributor Author

Looks great -- thank you very much @joshwd36

I think the code looks correct 👍 . Other than rebasing, the only thing I suggest for this PR is possibly add tests for the following cases:

1. Alternate clause order `ON UPDATE ... ON DELETE` (I only see a test for `ON DELETE .. ON UPDATE..`)

2. Error: duplicated ON DELETE clause e.g. `... ON DELETE CASCADE ON DELETE CASCADE`

Thoughts @Dandandan / @andygrove ?

Sounds good. I'll see if I get a chance to do this in the next few days.

joshwd36 and others added 2 commits August 25, 2021 12:55
…qlparser-rs into referential_constriant_action

# Conflicts:
#	tests/sqlparser_common.rs
@joshwd36 joshwd36 changed the base branch from main to assert_remove_postgres August 25, 2021 12:07
@joshwd36 joshwd36 changed the base branch from assert_remove_postgres to main August 25, 2021 12:07
@coveralls
Copy link

coveralls commented Aug 25, 2021

Pull Request Test Coverage Report for Build 1166487596

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 50 of 55 (90.91%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 90.181%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 10 11 90.91%
src/ast/ddl.rs 11 15 73.33%
Totals Coverage Status
Change from base Build 1160013514: 0.1%
Covered Lines: 5621
Relevant Lines: 6233

💛 - Coveralls

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks @joshwd36

@alamb alamb merged commit a95c81f into apache:main Aug 25, 2021
@joshwd36 joshwd36 deleted the referential_constriant_action branch August 25, 2021 16:30
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