Add CREATE TRIGGER support for SQL Server

[aharpervc]

Adjacent to: #1808 with similar considerations, and temporarily rebased on it (eg, this branch should probably wait for that branch to merge).

---

This PR introduces support for parsing CREATE TRIGGER for SQL Server.

The main concern is that for the existing dialects, there was an expectation of an EXECUTE keyword (PG: https://www.postgresql.org/docs/current/sql-createtrigger.html). However, SQL Server doesn't use this syntax and instead supports multi statement blocks (like a stored procedure).

The difficulty here in the codebase is what to do about CreateTrigger.exec_body & TriggerExecBody. In this iteration I made the property an Option, which seemed like the least impact on existing code. TriggerExecBody, etc can be left alone.

However in the future I think this could be improved with a more broad refactoring, such as a TriggerBody enum, which can consolidate TriggerExecBodyType's options & use Vec<Statements> for a MultiStatement variant and FunctionDesc for Function & Procedure variants. That'd basically remove the TriggerExecBody struct, too. Overall I think that'd be a cleaner approach for the CreateTrigger struct.

---

That's all speculative followup work, for now I just want to be able to parse most SQL Server triggers. Other SQL Server specific things like particular trigger options, DDL trigger stuff, etc can come later as needed.

[aharpervc]

This fails to parse on this branch, but should:

CREATE TRIGGER some_trigger ON some_table FOR INSERT
AS
BEGIN
    IF 1=1
    BEGIN
        RAISERROR('Trigger fired', 10, 1);
    END

    RETURN;
END

Fixed, never mind

[aharpervc]

FYI for reviewers I rebased this on the create function branch, due to the return statement logic here: #1808 (comment)

[aharpervc]

Rebased again now that #1808 has been merged. Should be ready for review.

[iffyio]

maybe this could be parsed via self.dialect.parse_statement() for the mssql version? since the dialect_of macro we try to stay away for new code

[aharpervc]

That seems tricky. Can you help me understand what that code would look like? parse_create_trigger is happening inside a complicated branch of parse_create, so it seems like that logic from src/parser/mod.rs would need to be copy/pasted to src/dialect/mssql.rs which seems unfortunate.

Otherwise I don't know how to have the dialect's parse_statement implement custom logic only for "create trigger" but not for all the other "create xxx"'s

[iffyio]

ah yeah it would need to manually parse the alter, replace for the trigger, I agree that its not ideal but its probably the lesser evil, see snowflake here for an example

[aharpervc]

Okay, thanks for the reference. The patterns in this library are difficult to interpret sometimes (what's best practice vs what's deprecated). I've moved the trigger parsing logic into the dialect. Doing it this way also means we need to be able to rewind the parser if it's some form of "create" not handled particularly in the dialect, so I also added a parser method to reset the index in that case accordingly.

[iffyio]

I think instead of manually tracking index we can rely on the parser.maybe_parse()? that will automatically handle the indexing

[aharpervc]

I've added a commit to the branch for this approach.

I'm not sure it's strictly better because it seems more difficult to extend if/when additional CREATE things are moved into dialects, if this is a general pattern. Perhaps you can clear up what I'm concerned about here -- with maybe_parse if parsing fails inside the dialect, it'll rewind the parser & report None up the chain. Then, parsing will continue as if it was common code. However that might not be what we prefer -- if we're already into parse_create_trigger and get a parsing error, we actually do want it to fail right there rather than rewinding. In the former approach that seemed clear. With this approach it seems like the dialect specific parser failure will get ignored (and you will probably get another parser error from the general code instead).

Thoughts?

[iffyio]

Ah yes you're right, if failure occurs while parsing the trigger body for example the ideal behavior in this case would be to report that error instead of returning None. In which case maybe_parse isn't ideal.

I think the main goal is avoiding a custom implementation to track indexing here, looking at the new diff it seems the sqlserver version is simpler so maybe something like this suffices already?

if self.parse_keywords(CREATE, TRIGGER) {
  Some(self.parse_create_trigger(self, parser, false))
} else if self.parse_keywords(CREATE, OR, ALTER, TRIGGER) {
  Some(self.parse_create_trigger(self, parser, true))
} else {
  None
}

[aharpervc]

Great, I'll drop the maybe_parse commit & update the implementation to this two branch approach. I think that will meet both our expectations for the time being.

A pro/con on that iteration will be that the dialect implementation of parse_statement is mixing peek & parse strategies in that function, which seems unfortunate. However further refactoring it to consolidate strategies seems outside the scope of the branch.

[aharpervc]

Done 👍

[aharpervc]

Similar approach in the other PR for stored procedures that may/not have begin/end tokens https://github.com/apache/datafusion-sqlparser-rs/pull/1834/files#r2069458988

[aharpervc]

I also updated the related stored procedure PR accordingly: #1834 (comment)

[iffyio]

Oh I think instead of representing the begin optionality using tokens, we can use an enum, something similar to ConditionalStatements where we have an explicit BeginEnd enum variant

[aharpervc]

Gotcha, yeah I could see it either way. I will update this to use the enum pattern

[aharpervc]

Done 👍