Add `CREATE FUNCTION` support for SQL Server #1808

aharpervc · 2025-04-10T19:55:14Z

Partially related: #1800

For reference: https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql?view=sql-server-ver16

In SQL Server, functions look a lot like procedures, with begin/end, multiple statements, and a return. To accommodate that here in the parser, BeginEndStatement was extracted from the conditional logic to be reused here, along with a RETURN statement type

Formerly, this was a new struct, but was later consolidated in with BeginEndStatement. I'm open to suggestions about how this could be improve (now or later). For example, since it's a BEGIN (statements) END construct, should this (and procedures) be using the statement block logic introduced here? https://github.com//pull/1791

Then also while I was working on this, I noticed that only SQL Server (so far...?) supports OR ALTER, so I introduced a new struct field to track that. (Again, very similar to CREATE PROCEDURE logic).

I'm not aiming to enable parsing every function type here in this PR -- just trying to get something operational, and the additional capabilities like table expressions, etc, can be added in later.

To continue the recent discussion, I noticed that both these new multi statement functions & the existing procedure parsing fails unless the statements are semi-colon delimited. Semi-colons are optional in SQL Server, so for example, this code executes fine for real but fails to parse here in this library, unless that "SET" ends with a ;...

CREATE OR ALTER PROCEDURE test (@foo INT, @bar VARCHAR(256))
AS
BEGIN
    SET @foo = @foo + 1
    SELECT @foo
END

I think this is because the procedure body parsing logic is using parse_statements(), which requires it? I'm game to tackle the problem with some guidance on what's a good approach to make it parse 🤔

src/ast/mod.rs

tests/sqlparser_hive.rs

src/ast/ddl.rs

aharpervc · 2025-04-11T18:58:06Z

src/parser/mod.rs

+
+        self.expect_keyword_is(Keyword::AS)?;
+        self.expect_keyword_is(Keyword::BEGIN)?;
+        let function_body = Some(CreateFunctionBody::MultiStatement(self.parse_statements()?));


Again, this will suffer the same fate as other multi statement syntax currently in this parser, which is that semi-colons are required to parse.

Ah if the desired behavior is to have optional semicolon delimited statements for mssql, what we could do would be to make the behavior configurable when parsing the statements list. We could do something like this?

pub fn parse_statements(&mut self) -> Result<Vec<Statement>, ParserError> { self.parse_statements_inner(true) } fn parse_statements_inner(&mut self, require_semicolon_delimiter) -> Result<Vec<Statement>, ParserError> { // ... same as the body of `parse_statements()` on main // ... I _think_ except for this part to make the behavior configurable if require_semicolon_delimiter && expecting_statement_delimiter { return self.expected("end of statement", self.peek_token()); } // ... }

then the mssql code can call the inner function directly

I think I understand what you're getting at with regards to configuration, but I'm not yet sure how to actually implement that statement parsing logic.

Do you see the semicolon-less statement parsing question as a blocker for the PR? If not I'd prefer to get this in (per your other suggestions) and then tackle this separately

but I'm not yet sure how to actually implement that statement parsing logic

hmm not sure I followed what we mean here? parsing a semicolon-less statement I imagine would follow from the above code snippet by calling self.parse_statements_inner(false)

It would indeed be good to resolve in this PR, currently in the PR we seem to have a workaround for this behavior here and here and another in the Go statement PR.

@aharpervc just in case, note that this part of the PR is unresolved

Thanks for the reminder.

It would indeed be good to resolve in this PR, currently in the PR we seem to have a workaround for this behavior here and here and another in the Go statement PR.

I was able to refactor the latter, and the former we'll need regardless of the semi colon situation. Does the latest iteration resolve your concerns here?

does something like in this suggestion work just as well? its currently unclear to me why the semicolon handling is a special case of an IfCondition statement

I'm not deeply opposed to making an inner parse function, I guess, but it also doesn't seem required for this PR.

its currently unclear to me why the semicolon handling is a special case of an IfCondition statement

I reverted that change, I think either it was a problem while hacking on the branch or pollution from another branch 😬

we seem to have some of the related changes left in this codepath?

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

Lines 4458 to 4466 in 6d0ea64

let semi_colon_expected = match values.last() {

Some(Statement::If(if_statement)) => if_statement.end_token.is_some(),

Some(_) => true,

None => false,

};

if semi_colon_expected {

self.expect_token(&Token::SemiColon)?;

}

This is to address a separate issue. The situation is addressed by the "create_function_with_conditional" test case example, which runs verified_stmt on this sql:

CREATE FUNCTION some_scalar_udf() RETURNS INT AS BEGIN IF 1 = 2 BEGIN RETURN 1; END; RETURN 0; END

The difficulty is regarding the IF statement. The pre-existing parse_if_stmt logic advances the parse past semi-colons (if any) before looking for an else block. If there's no else block as in this example, the next token will be part of the next statement after END's semi-colon.

That conflicts with the pre-existing logic in parse_statement_list, which unconditionally required a semi-colon after each parsed statement.

Therefore, this example would incorrectly be a parser error. To work around that limitation I added this conditional logic to parse_statement_list to inspect the previous statement for an IF statement end block. If found, no semi colon is expected.

Thinking about it some more, I suppose it would be better to keep that concern inside the parse_if_stmt function rather than bleeding out into parse_statement_list. I've done that now on the branch, thoughts?

tests/sqlparser_mssql.rs

src/parser/mod.rs

src/ast/mod.rs

src/parser/mod.rs

src/ast/mod.rs

src/ast/ddl.rs

src/parser/mod.rs

iffyio · 2025-04-12T07:09:06Z

src/parser/mod.rs

+
+        self.expect_keyword_is(Keyword::AS)?;
+        self.expect_keyword_is(Keyword::BEGIN)?;
+        let function_body = Some(CreateFunctionBody::MultiStatement(self.parse_statements()?));


Ah if the desired behavior is to have optional semicolon delimited statements for mssql, what we could do would be to make the behavior configurable when parsing the statements list. We could do something like this?

pub fn parse_statements(&mut self) -> Result<Vec<Statement>, ParserError> { self.parse_statements_inner(true) } fn parse_statements_inner(&mut self, require_semicolon_delimiter) -> Result<Vec<Statement>, ParserError> { // ... same as the body of `parse_statements()` on main // ... I _think_ except for this part to make the behavior configurable if require_semicolon_delimiter && expecting_statement_delimiter { return self.expected("end of statement", self.peek_token()); } // ... }

then the mssql code can call the inner function directly

tests/sqlparser_mssql.rs

aharpervc · 2025-04-14T14:45:45Z

src/parser/mod.rs

@@ -15017,6 +15075,13 @@ impl<'a> Parser<'a> {
        }
    }

+    fn parse_return(&mut self) -> Result<Statement, ParserError> {
+        let expr = self.parse_expr()?;


Does this need to be a maybe_parse, like how parse_raise_stmt does it for Exprs? The approach I picked for the discussion of RETURN variants above was to only do what's needed for this PR & these tests, which always have an Expr after the return.

So I think perhaps what we'd need in a followup PR is:

test case for bare RETURN

update this to be use "maybe" logic to find an expr (perhaps we'd like maybe_expr to wrap up that pattern?)

Maybe we can remove the return statement bits from this PR and tackle it separately as its own PR?

Otherwise indeed the expectation would be that this uses maybe_parse and we have the test cases in place as part of this PR

Maybe we can remove the return statement bits from this PR and tackle it separately as its own PR?

I don't think this is possible, since SQL Server requires that functions have a return, so create function goes together with return. I'll take another pass on the PR though

Ah alright, in that case I think its only the test cases missing for the return statement we can add those to common.rs I imagine

I've added a test case for a bare RETURN to sqlparser_common.rs, although I'm unsure how useful it is. At least for SQL Server, a top level return must be bare (eg, can't return anything). Therefore, the more useful tests are where return is inside a function body, trigger body, etc. And we have those tests in their respective branches.

src/parser/mod.rs

src/ast/mod.rs

src/parser/mod.rs

iffyio · 2025-04-15T05:21:22Z

src/parser/mod.rs

@@ -15017,6 +15075,13 @@ impl<'a> Parser<'a> {
        }
    }

+    fn parse_return(&mut self) -> Result<Statement, ParserError> {
+        let expr = self.parse_expr()?;


Maybe we can remove the return statement bits from this PR and tackle it separately as its own PR?

Otherwise indeed the expectation would be that this uses maybe_parse and we have the test cases in place as part of this PR

src/parser/mod.rs

iffyio · 2025-04-15T05:37:51Z

src/parser/mod.rs

+
+        self.expect_keyword_is(Keyword::AS)?;
+        self.expect_keyword_is(Keyword::BEGIN)?;
+        let function_body = Some(CreateFunctionBody::MultiStatement(self.parse_statements()?));


but I'm not yet sure how to actually implement that statement parsing logic

hmm not sure I followed what we mean here? parsing a semicolon-less statement I imagine would follow from the above code snippet by calling self.parse_statements_inner(false)

It would indeed be good to resolve in this PR, currently in the PR we seem to have a workaround for this behavior here and here and another in the Go statement PR.

src/ast/ddl.rs

iffyio · 2025-04-16T05:12:48Z

src/ast/spans.rs

-                end_token: AttachedToken(end),
-            } => union_spans([start.span, end.span].into_iter()),
+            ConditionalStatements::BeginEnd(bes) => {
+                union_spans([bes.begin_token.0.span, bes.end_token.0.span].into_iter())


Can we impl Spanned for BeginEndStatements and then call bes.span() here instead? That would make the span impl reusable by nother nodes that embed the BeginEndStatements

I think I understand what you're asking for, I'll give the implementation a shot

src/ast/mod.rs

iffyio · 2025-04-16T05:20:23Z

src/ast/mod.rs

        }
    }
 }

+/// A shared representation of `BEGIN`, multiple statements, and `END` tokens.


Suggested change

/// A shared representation of `BEGIN`, multiple statements, and `END` tokens.

/// Represents a list of statements enclosed within `BEGIN` and `END` keywords.

/// Example:

/// ```sql

/// BEGIN

/// SELECT 1;

/// SELECT 2;

/// END

/// ```

tests/sqlparser_mssql.rs

iffyio · 2025-04-16T05:35:05Z

src/parser/mod.rs

@@ -15017,6 +15075,13 @@ impl<'a> Parser<'a> {
        }
    }

+    fn parse_return(&mut self) -> Result<Statement, ParserError> {
+        let expr = self.parse_expr()?;


Ah alright, in that case I think its only the test cases missing for the return statement we can add those to common.rs I imagine

src/ast/spans.rs

iffyio · 2025-04-18T06:52:21Z

tests/sqlparser_common.rs

+
+#[test]
+fn parse_return() {
+    all_dialects().verified_stmt("RETURN");


Can we have this check assert the AST? Also we can add another test case that includes a return statement having an expression

Can we have this check assert the AST?

Done 👍

Also we can add another test case that includes a return statement having an expression

Done, although this is invalid for SQL Server. I think having that test here would also mean we can't have a test that asserts an error when return with value is found as a top level statement. Thoughts? 🤔

hmm yeah given that its currently part of the parser behavior I think its reasonable to assert it. If the parser later stops accepting a top-level return value then we would change the test here to assert an error instead

tests/sqlparser_mssql.rs

iffyio · 2025-04-18T06:56:21Z

src/parser/mod.rs

+
+        self.expect_keyword_is(Keyword::AS)?;
+        self.expect_keyword_is(Keyword::BEGIN)?;
+        let function_body = Some(CreateFunctionBody::MultiStatement(self.parse_statements()?));


does something like in this suggestion work just as well? its currently unclear to me why the semicolon handling is a special case of an IfCondition statement

iffyio

Thanks @aharpervc! Left a couple minor comments, I think beyond the if condition semicolon special handling the changes look reasonable to me!

iffyio · 2025-04-19T06:45:30Z

src/parser/mod.rs

+        let return_type = if self.parse_keyword(Keyword::RETURNS) {
+            Some(self.parse_data_type()?)
+        } else {
+            return parser_err!("Expected RETURNS keyword", self.peek_token().span.start);
+        };


Suggested change

let return_type = if self.parse_keyword(Keyword::RETURNS) {

Some(self.parse_data_type()?)

} else {

return parser_err!("Expected RETURNS keyword", self.peek_token().span.start);

};

self.expect_keyword(Keyword::RETURNS)?;

let return_type = Some(self.parse_data_type()?);

iffyio · 2025-04-19T06:46:23Z

src/parser/mod.rs

@@ -15063,6 +15127,15 @@ impl<'a> Parser<'a> {
            message: Box::new(self.parse_expr()?),
        }))
    }
+    /// Parse [Statement::Return]


Suggested change

/// Parse [Statement::Return]

/// Parse [Statement::Return]

iffyio · 2025-04-19T06:51:21Z

src/parser/mod.rs

+
+        self.expect_keyword_is(Keyword::AS)?;
+        self.expect_keyword_is(Keyword::BEGIN)?;
+        let function_body = Some(CreateFunctionBody::MultiStatement(self.parse_statements()?));


we seem to have some of the related changes left in this codepath?

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

Lines 4458 to 4466 in 6d0ea64

let semi_colon_expected = match values.last() {

Some(Statement::If(if_statement)) => if_statement.end_token.is_some(),

Some(_) => true,

None => false,

};

if semi_colon_expected {

self.expect_token(&Token::SemiColon)?;

}

iffyio · 2025-04-19T06:53:13Z

tests/sqlparser_common.rs

+
+#[test]
+fn parse_return() {
+    all_dialects().verified_stmt("RETURN");


hmm yeah given that its currently part of the parser behavior I think its reasonable to assert it. If the parser later stops accepting a top-level return value then we would change the test here to assert an error instead

- in this dialect, functions can have statement(s) bodies like stored procedures (including `BEGIN`..`END`) - functions must end with `RETURN`, so a corresponding statement type is also introduced

…nctions - this lets us disacard the former (unfortunate, bespoke) multi statement parsing because we can just use `parse_statement_list` - however, `parse_statement_list` also needed a small change to allow subsequent statements to come after the final `END`

- no need for special IF logic to bleed over to another function

iffyio

LGTM! Thanks @aharpervc!
cc @alamb

aharpervc commented Apr 10, 2025

View reviewed changes

src/ast/mod.rs Outdated Show resolved Hide resolved

aharpervc commented Apr 10, 2025

View reviewed changes

tests/sqlparser_hive.rs Show resolved Hide resolved

aharpervc marked this pull request as ready for review April 10, 2025 20:06

aharpervc mentioned this pull request Apr 11, 2025

Add support for GO batch delimiter in SQL Server #1809

Open

aharpervc commented Apr 11, 2025

View reviewed changes

src/ast/ddl.rs Show resolved Hide resolved

aharpervc commented Apr 11, 2025

View reviewed changes

tests/sqlparser_mssql.rs Outdated Show resolved Hide resolved

aharpervc mentioned this pull request Apr 11, 2025

Add CREATE TRIGGER support for SQL Server #1810

Open

iffyio reviewed Apr 12, 2025

View reviewed changes

aharpervc force-pushed the mssql-create-function branch 2 times, most recently from f5857d9 to 618eb4d Compare April 14, 2025 14:37

aharpervc commented Apr 14, 2025

View reviewed changes

tests/sqlparser_mssql.rs Show resolved Hide resolved

aharpervc requested a review from iffyio April 14, 2025 14:39

aharpervc commented Apr 14, 2025

View reviewed changes

src/parser/mod.rs Outdated Show resolved Hide resolved

iffyio reviewed Apr 15, 2025

View reviewed changes

aharpervc force-pushed the mssql-create-function branch 3 times, most recently from 580114e to 9954416 Compare April 15, 2025 21:34

aharpervc requested a review from iffyio April 15, 2025 22:01

iffyio reviewed Apr 16, 2025

View reviewed changes

aharpervc force-pushed the mssql-create-function branch 2 times, most recently from 008d821 to 1e51121 Compare April 16, 2025 21:22

iffyio reviewed Apr 18, 2025

View reviewed changes

aharpervc force-pushed the mssql-create-function branch 3 times, most recently from 49f746c to 6d0ea64 Compare April 18, 2025 19:53

aharpervc requested a review from iffyio April 18, 2025 20:19

iffyio reviewed Apr 19, 2025

View reviewed changes

Add basic CREATE FUNCTION support for SQL Server

f3af75f

- in this dialect, functions can have statement(s) bodies like stored procedures (including `BEGIN`..`END`) - functions must end with `RETURN`, so a corresponding statement type is also introduced

aharpervc added 13 commits April 21, 2025 14:46

Add OR ALTER support for CREATE FUNCTION

02ecfe5

Add test for conditionals in multi statement functions

dff760f

Extract common implementation for function name & parameters

157aa60

Support bare RETURN without expression & add common test

930eaa8

Add RETURN test with value

e5116bc

Simplify test fixtures

8780462

Reword documentation comments to be more generalized for all dialects

fd104aa

Simplify writing AsBeginEnd body

a2c3bdc

Further simplify test cases to use verified_stmt

b44befa

Simplify parsing RETURN statements

f7993f3

Implement Spanned trait for BeginEndStatements

dbe1cc5

Create span from exhaustive match on struct fields

05d611c

aharpervc force-pushed the mssql-create-function branch from bfd767e to b451b2d Compare April 21, 2025 18:46

aharpervc added 2 commits April 21, 2025 14:52

Move IF statement semi-colon logic into parse_if_stmt

86781b6

- no need for special IF logic to bleed over to another function

Simplify parsing RETURNS

ca28413

aharpervc force-pushed the mssql-create-function branch from e04f4b7 to ca28413 Compare April 21, 2025 18:52

aharpervc requested a review from iffyio April 21, 2025 21:40

iffyio approved these changes Apr 23, 2025

View reviewed changes

iffyio merged commit 2eb1e7b into apache:main Apr 23, 2025
9 checks passed

	let semi_colon_expected = match values.last() {
	Some(Statement::If(if_statement)) => if_statement.end_token.is_some(),
	Some(_) => true,
	None => false,
	};

	if semi_colon_expected {
	self.expect_token(&Token::SemiColon)?;
	}

-/// A shared representation of `BEGIN`, multiple statements, and `END` tokens.
+/// Represents a list of statements enclosed within `BEGIN` and `END` keywords.
+/// Example:
+/// ```sql
+/// BEGIN
+///     SELECT 1;
+///     SELECT 2;
+/// END
+/// ```

Add CREATE FUNCTION support for SQL Server #1808

Add CREATE FUNCTION support for SQL Server #1808

Conversation

aharpervc commented Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

Add `CREATE FUNCTION` support for SQL Server #1808

Add `CREATE FUNCTION` support for SQL Server #1808

aharpervc commented Apr 10, 2025 •

edited

Loading