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

@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.

Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @LucaCappelletti94!

@iffyio iffyio added this pull request to the merge queue Oct 11, 2025
Merged via the queue into apache:main with commit 4490c8c Oct 11, 2025
10 checks passed
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