Improve support for cursors for SQL Server #1831

aharpervc · 2025-04-28T17:39:44Z

This PR is a followup to #1821 to address several difficulties I had parsing real world SQL files.

There are 3 related enhancements here:

Introduce support for parsing OPEN statements, eg: OPEN my_cursor
Expand existing FETCH statement parsing support the FROM keyword. Eg, FETCH NEXT FROM my_cursor. The logic formerly only supported "FETCH NEXT IN" syntax
Introduce support for parsing WHILE statements, which is commonly used in conjunction with cursors. Eg WHILE @@fetch_status = 0.... This is a conditional statement block, much like IF & CASE, so that code has been structured similarly and placed adjacent to those statements.

(4th thing -- a test helper introduced in this PR was brought over here, because it simplifies validating a test case. If the other PR merges first that commit can be dropped out of this branch).

The effect of these changes is that this cursor example from the SQL Server FETCH documentation page now successfully parses:

DECLARE Employee_Cursor CURSOR FOR
SELECT LastName, FirstName
FROM AdventureWorks2022.HumanResources.vEmployee
WHERE LastName LIKE 'B%';

OPEN Employee_Cursor;
FETCH NEXT FROM Employee_Cursor;

WHILE @@FETCH_STATUS = 0
BEGIN
    FETCH NEXT FROM Employee_Cursor;
END;

CLOSE Employee_Cursor;
DEALLOCATE Employee_Cursor

- parse `OPEN cursor_name` statements - enable `FETCH` statements to parse `FROM cursor_name`, in addition to the existing `IN` parsing

- it's a conditional block alongside IF & CASE

aharpervc · 2025-04-28T17:41:09Z

src/ast/mod.rs

+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub struct WhileStatement {
+    pub while_block: ConditionalStatementBlock,


We don't absolutely need a WhileStatement struct; we could be doing Statement::While(ConditionalStatementBlock) instead. I'm following the example of CASE & IF, which also do it this way.

aharpervc · 2025-04-28T17:43:53Z

src/ast/mod.rs

+    /// OPEN cursor_name
+    /// ```
+    /// Opens a cursor.
+    Open {


I placed this next to CLOSE because they're semantically paired, rather than alphabetical. Not sure what's preferred on this project

aharpervc · 2025-04-28T17:44:16Z

src/ast/mod.rs

+        /// Differentiate between dialects that fetch `FROM` vs fetch `IN`
+        ///
+        /// [MsSql](https://learn.microsoft.com/en-us/sql/t-sql/language-elements/fetch-transact-sql)
+        from_or_in: AttachedToken,


Not sure what's best here, it could also be two separate Optional fields

Could we represent it with an explicit enum?
e.g

enum FetchPosition { From In }

- this is useful since opening a cursor typically happens immediately after declaring the cursor's query

iffyio · 2025-04-29T18:54:34Z

src/ast/mod.rs

+    /// OPEN cursor_name
+    /// ```
+    /// Opens a cursor.
+    Open {


Could we wrap this new statement in a named struct?

Done 👍. I didn't do that originally so as to more closely mimic the existing code

iffyio · 2025-04-29T19:02:57Z

src/ast/mod.rs

+        /// Differentiate between dialects that fetch `FROM` vs fetch `IN`
+        ///
+        /// [MsSql](https://learn.microsoft.com/en-us/sql/t-sql/language-elements/fetch-transact-sql)
+        from_or_in: AttachedToken,


Could we represent it with an explicit enum?
e.g

enum FetchPosition { From In }

src/keywords.rs

iffyio · 2025-04-29T19:17:04Z

tests/sqlparser_mssql.rs

+
+#[test]
+fn test_mssql_while_statement() {
+    let while_single_statement = "WHILE 1 = 0 PRINT 'Hello World';";


Can we include a test case with multiple statements in the while block?

Also since this introduces a new statement, can we include a test case that asserts the returned AST?

Can we include a test case with multiple statements in the while block?

This is covered by the additional subsequent examples

Also since this introduces a new statement, can we include a test case that asserts the returned AST?

Yes, I'll do that here for the initial example case

iffyio · 2025-04-29T19:19:07Z

src/test_utils.rs

+        canonical: &str,
+    ) -> Vec<Statement> {
+        let statements = self.parse_sql_statements(sql).expect(sql);
+        assert_eq!(statements.len(), statement_count);


this assertion seems to already be covered by the if/else below? so that we can skip the statement_count argument requirement?

Hm, I don't fully understand. Without this line you can't guarantee that the string you feed in has exactly the number of statements you intend it to parse. Also, one_statement_parses_to has this same assertion before the if/else.

Ah so I meant that in both cases when asserting :

in this case we're already explicitly check that both statements lists are identical

assert_eq!(self.parse_sql_statements(canonical).unwrap(), statements);

Then in this case, we're doing so implicitly, reconstructing the input sql based off the returned statement

assert_eq!( sql, statements .iter() .map(|s| s.to_string()) .collect::<Vec<_>>() .join("; ") );

So that i imagine it shouldn't be possible for the count assertion to fail and either of the subsequent assertion to pass?

one_statement_parse_to uses the count internally and there it makes sense to sanity check that since the expected count is always one, in this case we're exposing the count as a function argument which makes for suboptimal API that the user has to manually increment/decrement a counter when the sql changes. So that if the count argument isn't strictly necessary we would want to skip it

Then in this case, we're doing so implicitly, reconstructing the input sql based off the returned statement ... So that i imagine it shouldn't be possible for the count assertion to fail and either of the subsequent assertion to pass?

I will remove the assertion here to get this branch merged. However, I think removing it removes a level of safety that is beneficial. Part of my thinking here is motivated by my upcoming branch on making semi colon statement delimiters optional. So any code that is making assumptions about "number of statements" becomes even more useful.

But perhaps that branch can re-introduce that assertion if necessary.

BTW, this helper was introduced over on the GO branch, does your opinion change at all seeing the usage over there?

iffyio · 2025-04-29T19:19:54Z

src/parser/mod.rs

@@ -8735,6 +8779,14 @@ impl<'a> Parser<'a> {
        })
    }

+    /// Parse [Statement::Open]


maybe I missed it, we seem to be lacking test cases for the open statement feature?

It's part of test_mssql_cursor, but I'll make a separate test function just for OPEN for clarity

iffyio · 2025-04-29T19:21:57Z

src/parser/mod.rs

+            if let Token::EOF = self.peek_nth_token_ref(0).token {
+                break;
+            }


can we collapse this into above to use a match statement?

match &self.peek_nth_token_ref(0).token { Token::Word(n) if ... Token::Eof }

Done 👍. I was probably trying to minimize the diff for review here

Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>

aharpervc · 2025-04-29T22:03:46Z

src/ast/mod.rs

-                write!(f, "FETCH {direction} ")?;
-
-                write!(f, "IN {name}")?;
+                write!(f, "FETCH {direction} {position} {name}")?;


This could probably be write!(f, "{position} {name}")?;, not sure what the pro/con is on that

tests/sqlparser_mssql.rs

iffyio

LGTM! Thanks @aharpervc!
cc @alamb

iffyio · 2025-05-02T03:23:54Z

src/parser/mod.rs

+            let begin_token = self.expect_keyword(Keyword::BEGIN)?;
+            let statements = self.parse_statement_list(terminal_keywords)?;
+            let end_token = self.expect_keyword(Keyword::END)?;


We seem to have this pattern upcoming in a few places, like #1810 maybe it would be good to pull it out into a method and reuse it both here and the preexisting usage here? We can probably do so in the former PR instead

aharpervc added 3 commits April 28, 2025 13:21

Add additional cursor parsing support for SQL Server

ac298a6

- parse `OPEN cursor_name` statements - enable `FETCH` statements to parse `FROM cursor_name`, in addition to the existing `IN` parsing

Add statements_parse_to helper

f1e8ac7

Add support for parsing WHILE statements

5ec1463

- it's a conditional block alongside IF & CASE

aharpervc commented Apr 28, 2025

View reviewed changes

aharpervc marked this pull request as ready for review April 28, 2025 17:49

Make OPEN reserved for table aliases

a72e1c1

- this is useful since opening a cursor typically happens immediately after declaring the cursor's query

iffyio reviewed Apr 29, 2025

View reviewed changes

aharpervc and others added 4 commits April 29, 2025 16:36

Introduce FetchPosition to simplify FROM vs IN

01d85a0

Remove unnecessary comment

9965777

Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>

Introduce a OpenStatement struct for the OPEN statement

648c024

Expand test example to assert AST

bf0036a

aharpervc force-pushed the mssql-cursor-usage branch from e0b484e to bf0036a Compare April 29, 2025 21:35

aharpervc added 2 commits April 29, 2025 17:37

Add test for OPEN statement

2941291

Merge conditions into single match

3d2001f

aharpervc commented Apr 29, 2025

View reviewed changes

aharpervc requested a review from iffyio April 29, 2025 22:03

aharpervc commented Apr 29, 2025

View reviewed changes

tests/sqlparser_mssql.rs Outdated Show resolved Hide resolved

aharpervc added 2 commits April 29, 2025 18:08

Fix incorrect trailing comma

dbf7606

Remove statement count assertion from statements_parse_to

3608d8c

aharpervc mentioned this pull request Apr 30, 2025

Allow stored procedures to be defined without BEGIN/END #1834

Open

iffyio approved these changes May 2, 2025

View reviewed changes

iffyio merged commit a464f8e into apache:main May 2, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve support for cursors for SQL Server #1831

Improve support for cursors for SQL Server #1831

aharpervc commented Apr 28, 2025

aharpervc Apr 28, 2025

aharpervc Apr 28, 2025

aharpervc Apr 28, 2025

iffyio Apr 29, 2025

aharpervc Apr 29, 2025

iffyio Apr 29, 2025

aharpervc Apr 29, 2025

iffyio Apr 29, 2025

iffyio Apr 29, 2025

aharpervc Apr 29, 2025

aharpervc Apr 29, 2025

iffyio Apr 29, 2025

aharpervc Apr 29, 2025

iffyio Apr 30, 2025

aharpervc Apr 30, 2025

aharpervc Apr 30, 2025

iffyio Apr 29, 2025

aharpervc Apr 29, 2025

aharpervc Apr 29, 2025

iffyio Apr 29, 2025

aharpervc Apr 29, 2025

aharpervc Apr 29, 2025

iffyio left a comment

iffyio May 2, 2025

Improve support for cursors for SQL Server #1831

Improve support for cursors for SQL Server #1831

Conversation

aharpervc commented Apr 28, 2025

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