Skip to content
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

Feature/on delete #197

Merged
merged 3 commits into from
Oct 3, 2023
Merged

Conversation

antoineB
Copy link
Contributor

No description provided.

Copy link
Collaborator

@matthias-Q matthias-Q left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I made some remarks that need to be addressed.

@@ -75,6 +75,7 @@ module.exports = grammar({
keyword_cross: _ => make_keyword("cross"),
keyword_join: _ => make_keyword("join"),
keyword_lateral: _ => make_keyword("lateral"),
keyword_natural: _ => make_keyword("natural"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is already part of #196 right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I used my previous MR to avoid conflicts.

grammar.js Outdated
@@ -2669,6 +2690,11 @@ module.exports = grammar({
)
),

natural_join: $ => seq(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already part of #196

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

grammar.js Outdated
@@ -2581,6 +2601,7 @@ module.exports = grammar({
repeat(
choice(
$.join,
$.natural_join,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already part of #196

grammar.js Outdated
@@ -2273,13 +2275,31 @@ module.exports = grammar({
seq(
$.keyword_on,
$.keyword_delete,
$.keyword_cascade,
$._referential_action,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could be combined in one optional block with a choice between keyword_delete and keyword_update. I see no difference between the other parts of the sequence.

grammar.js Show resolved Hide resolved
Copy link
Collaborator

@matthias-Q matthias-Q left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I double checked the statement in the Postgres documentation and it looks like a small part is missing. Would be nice if you could add this.

$.keyword_cascade,
seq(
$.keyword_set,
choice($.keyword_null, $.keyword_default),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think set null and set default (this one is missing), can optionally be followed by a column name. In our parser the column name is represented by the object_reference node. (paren_list() of object_reference

https://www.postgresql.org/docs/current/sql-altertable.html (under referential_action)

Copy link
Contributor Author

@antoineB antoineB Oct 2, 2023

Choose a reason for hiding this comment

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

You cannot put an object_reference just an identifier:

toto=# create table test1(id integer references test0(id) on delete set null(test.id));
ERROR:  syntax error at or near "."
LINE 1: ...d integer references test0(id) on delete set null(test.id));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also line 2273 $.ordered_columns should be paren_list($.identifier) at least on postgres.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This worked for me on postgres 15.3

CREATE TABLE table1 (
    id serial PRIMARY KEY,
    column_name1 integer,
    column_name2 integer
);

CREATE TABLE table2 (
    id serial PRIMARY KEY,
    table1_id integer REFERENCES table1(id) ON DELETE CASCADE
);

ALTER TABLE table1
DROP CONSTRAINT IF EXISTS table1_column_name1_fkey;

ALTER TABLE table1
ADD CONSTRAINT table1_column_name1_fkey
FOREIGN KEY (column_name1)
REFERENCES table2(id)
ON DELETE SET NULL (column_name1);

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right about object_reference. This won't work. identifier does work however.

@matthias-Q
Copy link
Collaborator

@antoineB Thanks for the MR. I have two general remarks:
Would be nice if you would not force push changes that are requested in a review. It makes it easier to review. We can always squash commits before merging. It would also be nice if you could add a small query that is failing or not yet supported and also provide the link to the SQL Dialect documentation. This makes it a lot easier for us.

However, we should definitely think about some contribution rules

@matthias-Q matthias-Q merged commit b27337b into DerekStride:main Oct 3, 2023
@antoineB antoineB deleted the feature/on_delete branch October 8, 2023 14:25
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