iffyio commented on code in PR #1809:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1809#discussion_r2051416091
##########
src/parser/mod.rs:
##########
@@ -4055,6 +4070,44 @@ impl<'a> Parser<'a> {
)
}
+ /// Look backwards in the token stream and expect that there was only
whitespace tokens until the previous newline
+ pub fn expect_previously_only_whitespace_until_newline(&mut self) ->
Result<(), ParserError> {
Review Comment:
```suggestion
pub(crate) fn expect_previously_only_whitespace_until_newline(&mut self)
-> Result<(), ParserError> {
```
##########
src/parser/mod.rs:
##########
@@ -15064,6 +15117,38 @@ impl<'a> Parser<'a> {
}))
}
+ /// Parse [Statement::Go]
+ fn parse_go(&mut self) -> Result<Statement, ParserError> {
+ self.expect_previously_only_whitespace_until_newline()?;
Review Comment:
hmm this call look equivalent to the loop below that peeks forward to skip
newlines? unclear why we're peeking backwards here right after we parsed the GO
keyword (so that this will always be infallible?)
##########
src/parser/mod.rs:
##########
@@ -15064,6 +15117,38 @@ impl<'a> Parser<'a> {
}))
}
+ /// Parse [Statement::Go]
+ fn parse_go(&mut self) -> Result<Statement, ParserError> {
+ self.expect_previously_only_whitespace_until_newline()?;
+
+ let count = loop {
+ // using this peek function because we want to halt this statement
parsing upon newline
Review Comment:
Can we include the example you had in the comment earlier, explicitly
highlighting why this statement is special? I think otherwise it would not be
obvious to folks that come across the code later on why the current code is a
special case
##########
tests/sqlparser_mssql.rs:
##########
@@ -2053,3 +2054,171 @@ fn parse_drop_trigger() {
}
);
}
+
+#[test]
+fn parse_mssql_go_keyword() {
+ let single_go_keyword = "USE some_database;\nGO";
Review Comment:
Ah yeah a new helper sounds reasonable to me thanks!
##########
src/parser/mod.rs:
##########
@@ -618,6 +632,7 @@ 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::PRINT => self.parse_print(),
+ Keyword::GO => self.parse_go(),
Review Comment:
can we rewind with `self.prev_token()` here before calling parse_go? so that
the `parse_go` function is self contained and can parse a full GO statement. (I
also realise now we missed that for print)
##########
src/dialect/mssql.rs:
##########
@@ -116,7 +116,17 @@ impl Dialect for MsSqlDialect {
true
}
- fn is_column_alias(&self, kw: &Keyword, _parser: &mut Parser) -> bool {
+ fn is_column_alias(&self, kw: &Keyword, parser: &mut Parser) -> bool {
+ // if we find maybe whitespace then a newline looking backward, then
`GO` ISN'T a column alias
+ // if we can't find a newline then we assume that `GO` IS a column
alias
+ if kw == &Keyword::GO
+ && parser
+ .expect_previously_only_whitespace_until_newline()
+ .is_ok()
Review Comment:
if the intent is for the condition to return a bool, then we can update this
function to return a bool instead of a result (where the latter is flagging an
error to be handled)
##########
src/test_utils.rs:
##########
@@ -166,6 +168,32 @@ impl TestedDialects {
only_statement
}
+ /// The same as [`one_statement_parses_to`] but it works for a multiple
statements
+ pub fn multiple_statements_parse_to(
Review Comment:
```suggestion
pub fn statements_parse_to(
```
##########
src/parser/mod.rs:
##########
@@ -4055,6 +4070,44 @@ impl<'a> Parser<'a> {
)
}
+ /// Look backwards in the token stream and expect that there was only
whitespace tokens until the previous newline
+ pub fn expect_previously_only_whitespace_until_newline(&mut self) ->
Result<(), ParserError> {
Review Comment:
hmm so i think it would be good to write this function in a reusable manner
if possible since it feels similar to the `peek_*`, `prev_*` helper functions
that we have. Would it be possible to introduce a generic
`prev_nth_token_no_skip(n: usize)` that can be called by parser and dialect
methods?
--
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
