Skip to content

Conversation

LucaCappelletti94
Copy link
Contributor

@LucaCappelletti94 LucaCappelletti94 commented Sep 18, 2025

This PR:

  • EnablesCREATE TRIGGER for SQLite dialect, which was early blocked by a dialect guard
  • Adds support for TEMPORARY triggers - at this time, I have not added a dialect guard on the use of the TEMPORARY keyword, which may be desirable if we want to raise errors in other dialect if TEMP triggers are provided.
  • Adds support for optional FOR EACH ROW in triggers, which is optional in SQLite
  • Adds test suite for several CREATE TRIGGER cases in SQLite.
  • Enables DROP TRIGGER for SQLite dialect, which was early blocked by a dialect guard
  • Adds test suite for a DROP TRIGGER case in SQLite.

Closes issue #2023

Comment on lines +5624 to +5626
if !dialect_of!(self is SQLiteDialect ) {
self.expect_keyword_is(Keyword::FOR)?;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if !dialect_of!(self is SQLiteDialect ) {
self.expect_keyword_is(Keyword::FOR)?;
}
self.expect_keyword_is(Keyword::FOR)?;

Can we skip the dialect guard here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems this comment is unresolved? ideally we want to skip dialect_of checks where possible

@LucaCappelletti94
Copy link
Contributor Author

Regarding commit c893bda, it should be noted that the MsSQL parser for triggers is incorrect, as it does not include much of their syntax, and currently always adds in a FOR STATEMENT, which is incorrect. This is clearly way outside the current PR objectives though. I have patched the parse trigger so that it would be compatible with the MsSQL tests, but I am completely unfamiliar with that dialect so I would leave a more refined extension to somebody more familiar with it.

@LucaCappelletti94
Copy link
Contributor Author

Is there anything else needed to be done for this PR? @iffyio

@iffyio
Copy link
Contributor

iffyio commented Oct 4, 2025

@LucaCappelletti94 could you take a look at this comment?

@LucaCappelletti94
Copy link
Contributor Author

Hi @iffyio, I replied to it here: #2037 (comment)

@iffyio
Copy link
Contributor

iffyio commented Oct 8, 2025

@LucaCappelletti94 the link takes me to the top of the page not sure why but I'm unable to find the comment in this case

@LucaCappelletti94
Copy link
Contributor Author

LucaCappelletti94 commented Oct 8, 2025

@LucaCappelletti94 the link takes me to the top of the page not sure why but I'm unable to find the comment in this case

@iffyio Here follows the comment:

So, if you mean to refactor this into:

} else {
    None
};

would simply make it more permissive and allow for parsing stuff that does not make sense in some dialects.

Conversely, the following:

} else {
    self.expect_keyword_is(Keyword::FOR)?;

    None
};

would crash for valid SQLite statements.

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