MSSQL: Support standalone BEGIN...END blocks

[guan404ming]

Why

I'm currently building sql-parser npm pkg for js/ts named sqlparser-ts, which powered by this amazing repo via WebAssembly! When reconstruct all dialects test in typescript, I find some issue and decide to contribute back to upstream~

The MSSQL dialect previously had no way to parse standalone BEGIN...END blocks.
BEGIN SELECT 1; END would fail because parse_begin() only handles BEGIN TRANSACTION.

How

wires MSSQL into the same path like BigQuery and Snowflake

[iffyio]

would a condition like this do what we want?

if parse.peek_keywords(BEGIN, TRANSACTION) {
    None
} else if parse_keyword(BEGIN) {
    Some(parser.parse_begin_exception_end())
}

its not super clear to me why the current logic looks at WORK, TRY etc keywords?

[guan404ming]

Thanks for the review. These keywords are checked because parse_begin() handles more than just BEGIN TRANSACTION. It also handles WORK, TRY, CATCH, DEFERRED, IMMEDIATE, EXCLUSIVE (since MsSql's supports_start_transaction_modifier() returns true), as well as bare BEGIN;. If any of these are missed, they'd be incorrectly routed to parse_begin_exception_end() and fail to parse.

For example, BEGIN TRY ... END TRY is valid MSSQL syntax. If we only check for BEGIN TRANSACTION, then BEGIN TRY would fall through to parse_begin_exception_end(), which would try to parse TRY as a SQL statement and fail. because TRY is supposed to be handled as a

datafusion-sqlparser-rs/src/parser/mod.rs

Lines 17938 to 17939 in c8b7f7c

	} else if self.parse_keyword(Keyword::TRY) {
	Some(TransactionModifier::Try)

[guan404ming]

Feel free to let me know if there are any not clear, thanks!

[iffyio]

Oh I see, that makes sense! Can we do something like this to clarify the code?

we pull out the linked fallback logic into a function parse_transaction_modifier(). So that it can be reused.

Then here we can do e.g.

if without_modifier = self.maybe_parse(|parser| {
    if parser.parse_transaction_modifier()?.is_some() {
        parser_error!()
    } else {
        Some(())
    }
})?.is_some();

such that with that bool we know whether to defer to the main parser logic or continue with the begin/exception logic

[guan404ming]

Thanks for the really nice suggestion. I just updated it does more clean and work as well.

[iffyio]

Left some minor comments, otherwise the changes look good to me overall

[guan404ming]

Just updated, thanks for the suggestion!

[iffyio]

LGTM! Thanks @guan404ming!

[guan404ming]

Thanks @iffyio for your detailed reviews!