iffyio commented on code in PR #1809:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1809#discussion_r2040595052
##########
src/parser/mod.rs:
##########
@@ -617,6 +623,9 @@ impl<'a> Parser<'a> {
}
// `COMMENT` is snowflake specific
https://docs.snowflake.com/en/sql-reference/sql/comment
Keyword::COMMENT if self.dialect.supports_comment_on() =>
self.parse_comment(),
+ Keyword::GO if dialect_of!(self is MsSqlDialect) => {
Review Comment:
```suggestion
Keyword::GO => {
```
I think it should be fine to let the parser always accept the statement if
it shows up. Technically if we wanted to gate it behind a feature then we could
use a `self.dialect.supports` flag since the `dialect_of` macro is deprecated,
but I think it makes more sense to let the parser accept unconditionally
##########
src/parser/mod.rs:
##########
@@ -475,6 +475,12 @@ impl<'a> Parser<'a> {
if expecting_statement_delimiter && word.keyword ==
Keyword::END {
break;
}
+ // Treat batch delimiter as an end of statement
+ if expecting_statement_delimiter && dialect_of!(self is
MsSqlDialect) {
+ if let Some(Statement::Go { count: _ }) = stmts.last()
{
+ expecting_statement_delimiter = false;
+ }
+ }
Review Comment:
not sure I followed this part, what was the intention?
##########
src/parser/mod.rs:
##########
@@ -15017,6 +15026,48 @@ impl<'a> Parser<'a> {
}
}
+ fn parse_go(&mut self) -> Result<Statement, ParserError> {
Review Comment:
One thing we could do, in the `parse_stmt()` function could we call
`self.prev_token()` to rewind the `Go` keyword so that this function becomes
self contained in being able to parse a full `Go` statement?
##########
src/parser/mod.rs:
##########
@@ -15017,6 +15026,48 @@ impl<'a> Parser<'a> {
}
}
+ fn parse_go(&mut self) -> Result<Statement, ParserError> {
+ // previous token should be a newline (skipping non-newline whitespace)
Review Comment:
hmm I didn't look too closely but not sure I followed the function body on a
high level, from the docs and the representation in the parser the Go statement
only contains an optional int, so that I imagined all the function needs to do
would be to `maybe_parse(|parser| parser.parse_number_value())` or similar? The
function is currently managing whitespace and semicolon but not super clear to
me why that is required
##########
src/ast/mod.rs:
##########
@@ -4050,6 +4050,14 @@ pub enum Statement {
arguments: Vec<Expr>,
options: Vec<RaisErrorOption>,
},
+ /// Go (SQL Server)
+ ///
+ /// GO is not a Transact-SQL statement; it is a command recognized by
various tools as a batch delimiter
+ ///
+ /// See:
https://learn.microsoft.com/en-us/sql/t-sql/language-elements/sql-server-utilities-statements-go
+ Go {
+ count: Option<u64>,
+ },
Review Comment:
Repr wise can we wrap the statement body in explicit structs? we're[
transitioning
away](https://github.com/apache/datafusion-sqlparser-rs/issues/1204) from
anonymous structs in order to make the API more ergonomic. Something like
```rust
struct GoStatement {
count: Option<u64>
}
Statement::Go(GoStatement)
```
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
