You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "romanb (via GitHub)" <gi...@apache.org> on 2025/04/02 09:46:19 UTC
[PR] Add support for MSSQL IF/ELSE statements. [datafusion-sqlparser-rs]
romanb opened a new pull request, #1791:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1791
This PR is a follow-up to https://github.com/apache/datafusion-sqlparser-rs/pull/1741. In particular, it addresses the parsing of IF/ELSE statements for MSSQL. These are syntactically quite different from the already supported IF ... THEN ... ELSEIF ... END IF statements.
The following are the main changes in this PR:
* `IfStatement` is now an enum with two variants.
* Statement parsing is overridden for the MSSQL dialect in order to parse `IF` statements differently for MSSQL.
* AST nodes for IF/CASE statements are slightly refactored to produce proper spans that include start/end tokens, if present.
* Tests are updated and extended.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for MSSQL IF/ELSE statements. [datafusion-sqlparser-rs]
Posted by "romanb (via GitHub)" <gi...@apache.org>.
romanb commented on PR #1791:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1791#issuecomment-2784202219
@aharpervc I see your difficulty. While working on this PR I did notice that a more general custom parsing for begin/end for the MSSQL dialect is probably necessary as well (e.g. by overriding `parse_begin` in the MSSQL dialect), but I did not want to expand the scope of this PR unnecessarily.
If your example statement for the stored procedure does not parse, I would suggest to open an issue as a starting point. The problem of parsing multiple top-level statements with optional semicolons for MSSQL may also warrant another issue - I'm personally just sticking to the code I mentioned above to work around that problem for now, not least because even in SQL server, the use of semicolons is very much encouraged (quoting from [the docs](https://learn.microsoft.com/en-us/sql/t-sql/language-elements/transact-sql-syntax-conventions-transact-sql?view=sql-server-ver16&tabs=code): `Although the semicolon isn't required for most statements in this version of SQL Server, it will be required in a future version.`)
Realistically though, I doubt the enforcement of semicolons will ever happen in SQL server and it is definitely desirable for this library to be fully capable of parsing with optional semicolons when using the MSSQL dialect out-of-the-box. I can only suggest to open new issues (or even PRs, if possible) for the remaining problems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for MSSQL IF/ELSE statements. [datafusion-sqlparser-rs]
Posted by "romanb (via GitHub)" <gi...@apache.org>.
romanb commented on code in PR #1791:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1791#discussion_r2029938472
##########
src/ast/spans.rs:
##########
@@ -739,19 +740,12 @@ impl Spanned for CreateIndex {
impl Spanned for CaseStatement {
fn span(&self) -> Span {
let CaseStatement {
- match_expr,
- when_blocks,
- else_block,
- has_end_case: _,
+ case_token: AttachedToken(start),
+ end_case_token: AttachedToken(end),
+ ..
Review Comment:
Sounds good, I did that! Thanks again for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for MSSQL IF/ELSE statements. [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1791:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1791#discussion_r2025392312
##########
src/ast/mod.rs:
##########
@@ -2145,116 +2149,189 @@ impl fmt::Display for CaseStatement {
}
if let Some(else_block) = else_block {
- write!(f, " ELSE ")?;
- format_statement_list(f, else_block)?;
+ write!(f, " {else_block}")?;
}
write!(f, " END")?;
- if *has_end_case {
- write!(f, " CASE")?;
+
+ if let Token::Word(w) = &end.token {
+ if w.keyword == Keyword::CASE {
+ write!(f, " CASE")?;
+ }
}
Ok(())
}
}
/// An `IF` statement.
-///
-/// Examples:
-/// ```sql
-/// IF TRUE THEN
-/// SELECT 1;
-/// SELECT 2;
-/// ELSEIF TRUE THEN
-/// SELECT 3;
-/// ELSE
-/// SELECT 4;
-/// END IF
-/// ```
-///
-/// [BigQuery](https://cloud.google.com/bigquery/docs/reference/standard-sql/procedural-language#if)
-/// [Snowflake](https://docs.snowflake.com/en/sql-reference/snowflake-scripting/if)
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
-pub struct IfStatement {
- pub if_block: ConditionalStatements,
- pub elseif_blocks: Vec<ConditionalStatements>,
- pub else_block: Option<Vec<Statement>>,
+pub enum IfStatement {
+ /// An `IF ... THEN [ELSE[IF] ...] END IF` statement.
+ ///
+ /// Example:
+ /// ```sql
+ /// IF TRUE THEN
+ /// SELECT 1;
+ /// SELECT 2;
+ /// ELSEIF TRUE THEN
+ /// SELECT 3;
+ /// ELSE
+ /// SELECT 4;
+ /// END IF
+ /// ```
+ ///
+ /// [BigQuery](https://cloud.google.com/bigquery/docs/reference/standard-sql/procedural-language#if)
+ /// [Snowflake](https://docs.snowflake.com/en/sql-reference/snowflake-scripting/if)
+ IfThenElseEnd {
+ /// The `IF` token that starts the statement.
+ if_token: AttachedToken,
+ if_block: ConditionalStatements,
+ elseif_blocks: Vec<ConditionalStatements>,
+ else_block: Option<ConditionalStatements>,
+ /// The `IF` token that ends the statement.
+ end_if_token: AttachedToken,
+ },
+ /// An MSSQL `IF ... ELSE ...` statement.
+ ///
+ /// Example:
+ /// ```sql
+ /// IF 1=1 SELECT 1 ELSE SELECT 2
+ /// ```
+ ///
+ /// [MSSQL](https://learn.microsoft.com/en-us/sql/t-sql/language-elements/if-else-transact-sql?view=sql-server-ver16)
+ MsSqlIfElse {
Review Comment:
Would it be possible to merge the representation to be shared by both? Something like this roughly I'm thinking
```rust
enum StatementBlock {
Expr(Expr),
Statements(Vec<Statements>),
Begin(Vec<Statements>)
}
pub struct IfStatement {
pub condition: Expr,
pub if_block: StatementBlock,
pub elseif_blocks: Vec<StatementBlock>,
pub else_block: Option<StatementBlock>,
}
```
Ideally we avoid introducing dialect specific nodes in the AST
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for MSSQL IF/ELSE statements. [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio merged PR #1791:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1791
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for MSSQL IF/ELSE statements. [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1791:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1791#discussion_r2029718008
##########
src/ast/spans.rs:
##########
@@ -739,19 +740,12 @@ impl Spanned for CreateIndex {
impl Spanned for CaseStatement {
fn span(&self) -> Span {
let CaseStatement {
- match_expr,
- when_blocks,
- else_block,
- has_end_case: _,
+ case_token: AttachedToken(start),
+ end_case_token: AttachedToken(end),
+ ..
Review Comment:
For this and `ConditionalStatement` Spanned impl could we explicitly list the remaining fields so that the `let` match is exhaustive? So that we're forced to revisit this part of the code if new fields are added/modified
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for MSSQL IF/ELSE statements. [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on PR #1791:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1791#issuecomment-2784381755
> If your example statement for the stored procedure does not parse, I would suggest to open an issue as a starting point.
Done: https://github.com/apache/datafusion-sqlparser-rs/issues/1800
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for MSSQL IF/ELSE statements. [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on PR #1791:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1791#issuecomment-2783695881
This is a great improvement! However, I noticed a difficulty with my code. This SQL fails to parse:
```
declare @x bit = 1
if @x = 1
begin
select 1
end
```
It works if you add a semi-colon for the declare statement, though. Either syntax executes as expected in SQL Server directly.
What would it take to make it parse okay even without the semi-colon?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for MSSQL IF/ELSE statements. [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on PR #1791:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1791#issuecomment-2784031574
Interesting. Of course, I should have also shared my Rust code. I'm doing `Parser::parse_sql(&dialect, sql_text)` where the dialect is SQL Server & the sql_text is loaded from a file.
Also, I was trying to simplify the example SQL to a minimum reproducible example, but my original scenario is this kind of code inside of a SP.
For example:
```mssql
create or alter procedure test()
as
begin
declare @x bit = 1
if @x = 1
begin
select 1
end
end
```
This also demonstrates the difficulty. I will adjust my Rust logic to the approach you suggested and see if I can make it work -- thank you!
I still view this as a very surprising library behavior, though, especially since SQL Server itself accepts/parses/executes the identical file without issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for MSSQL IF/ELSE statements. [datafusion-sqlparser-rs]
Posted by "romanb (via GitHub)" <gi...@apache.org>.
romanb commented on code in PR #1791:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1791#discussion_r2029344741
##########
src/ast/mod.rs:
##########
@@ -2145,116 +2149,189 @@ impl fmt::Display for CaseStatement {
}
if let Some(else_block) = else_block {
- write!(f, " ELSE ")?;
- format_statement_list(f, else_block)?;
+ write!(f, " {else_block}")?;
}
write!(f, " END")?;
- if *has_end_case {
- write!(f, " CASE")?;
+
+ if let Token::Word(w) = &end.token {
+ if w.keyword == Keyword::CASE {
+ write!(f, " CASE")?;
+ }
}
Ok(())
}
}
/// An `IF` statement.
-///
-/// Examples:
-/// ```sql
-/// IF TRUE THEN
-/// SELECT 1;
-/// SELECT 2;
-/// ELSEIF TRUE THEN
-/// SELECT 3;
-/// ELSE
-/// SELECT 4;
-/// END IF
-/// ```
-///
-/// [BigQuery](https://cloud.google.com/bigquery/docs/reference/standard-sql/procedural-language#if)
-/// [Snowflake](https://docs.snowflake.com/en/sql-reference/snowflake-scripting/if)
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
-pub struct IfStatement {
- pub if_block: ConditionalStatements,
- pub elseif_blocks: Vec<ConditionalStatements>,
- pub else_block: Option<Vec<Statement>>,
+pub enum IfStatement {
+ /// An `IF ... THEN [ELSE[IF] ...] END IF` statement.
+ ///
+ /// Example:
+ /// ```sql
+ /// IF TRUE THEN
+ /// SELECT 1;
+ /// SELECT 2;
+ /// ELSEIF TRUE THEN
+ /// SELECT 3;
+ /// ELSE
+ /// SELECT 4;
+ /// END IF
+ /// ```
+ ///
+ /// [BigQuery](https://cloud.google.com/bigquery/docs/reference/standard-sql/procedural-language#if)
+ /// [Snowflake](https://docs.snowflake.com/en/sql-reference/snowflake-scripting/if)
+ IfThenElseEnd {
+ /// The `IF` token that starts the statement.
+ if_token: AttachedToken,
+ if_block: ConditionalStatements,
+ elseif_blocks: Vec<ConditionalStatements>,
+ else_block: Option<ConditionalStatements>,
+ /// The `IF` token that ends the statement.
+ end_if_token: AttachedToken,
+ },
+ /// An MSSQL `IF ... ELSE ...` statement.
+ ///
+ /// Example:
+ /// ```sql
+ /// IF 1=1 SELECT 1 ELSE SELECT 2
+ /// ```
+ ///
+ /// [MSSQL](https://learn.microsoft.com/en-us/sql/t-sql/language-elements/if-else-transact-sql?view=sql-server-ver16)
+ MsSqlIfElse {
Review Comment:
Thank you for your feedback. I pushed further changes to avoid the dialect-specific AST nodes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for MSSQL IF/ELSE statements. [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1791:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1791#issuecomment-2783255779
Amazing -- thank you @romanb and @iffyio
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for MSSQL IF/ELSE statements. [datafusion-sqlparser-rs]
Posted by "romanb (via GitHub)" <gi...@apache.org>.
romanb commented on PR #1791:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1791#issuecomment-2783825394
> This is a great improvement! However, I noticed a difficulty with my code. This SQL fails to parse:
>
> ```
> declare @x bit = 1
>
> if @x = 1
> begin
> select 1
> end
> ```
>
> It works if you add a semi-colon for the declare statement, though. Either syntax executes as expected in SQL Server directly.
>
> What would it take to make it parse okay even without the semi-colon?
To parse multiple top-level statements for MSSQL with optional semicolons, you can use the `Parser` API as follows:
```rust
let mut stmts = Vec::new();
loop {
if let Token::EOF = parser.peek_token_ref().token {
break;
}
stmts.push(parser.parse_statement()?);
while let Token::SemiColon = parser.peek_token_ref().token {
parser.advance_token();
}
}
```
I also did it similarly in this PR when parsing the statements within `BEGIN` and `END` of a conditional for MSSQL. In contrast, `Parser#parse_statements()` always expects semicolon-separated statements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org