alamb commented on code in PR #1949:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1949#discussion_r2219942253
##########
src/parser/mod.rs:
##########
@@ -266,6 +266,22 @@ impl ParserOptions {
self.unescape = unescape;
self
}
+
+ /// Set if semicolon statement delimiters are required.
Review Comment:
What is the usecase to have the same configuration information on the
`ParserOptions` and the `Dialect`?
It seems there are a few places in the Parser that inconsistently check the
options and/or the dialect flag. If we had only one way to specify the option,
there would not be any room for inconsistencies
Or maybe I misunderstand what is going on here
##########
src/dialect/mod.rs:
##########
@@ -1136,6 +1142,11 @@ pub trait Dialect: Debug + Any {
fn supports_notnull_operator(&self) -> bool {
false
}
+
+ /// Returns true if the dialect supports parsing statements without a
semicolon delimiter.
Review Comment:
Would it be possible to give an example here?
Something like
```suggestion
/// Returns true if the dialect supports parsing statements without a
semicolon delimiter.
///
/// If returns true, the following SQL will not parse. If returns
`false` the SQL will parse
///
/// ```sql
/// SELECT 1
/// SELECT 2
/// ```
```
##########
src/parser/mod.rs:
##########
@@ -16464,7 +16505,28 @@ impl<'a> Parser<'a> {
/// Parse [Statement::Return]
fn parse_return(&mut self) -> Result<Statement, ParserError> {
- match self.maybe_parse(|p| p.parse_expr())? {
+ let rs = self.maybe_parse(|p| {
+ let expr = p.parse_expr()?;
+
+ match &expr {
+ Expr::Value(_)
+ | Expr::Function(_)
+ | Expr::UnaryOp { .. }
+ | Expr::BinaryOp { .. }
+ | Expr::Case { .. }
+ | Expr::Cast { .. }
+ | Expr::Convert { .. }
+ | Expr::Subquery(_) => Ok(expr),
+ // todo: how to retstrict to variables?
Review Comment:
This seems more like a semantic check -- this crate is focused on syntax
parsing.
Read more hre:
https://github.com/apache/datafusion-sqlparser-rs?tab=readme-ov-file#syntax-vs-semantics
I think the error is inappropriate in this case
##########
src/test_utils.rs:
##########
@@ -186,6 +187,37 @@ impl TestedDialects {
statements
}
+ /// The same as [`statements_parse_to`] but it will strip semicolons from
the SQL text.
+ pub fn statements_without_semicolons_parse_to(
+ &self,
+ sql: &str,
+ canonical: &str,
+ ) -> Vec<Statement> {
+ let sql_without_semicolons = sql
+ .replace("; ", " ")
+ .replace(" ;", " ")
+ .replace(";\n", "\n")
+ .replace("\n;", "\n")
+ .replace(";", " ");
Review Comment:
wouldn't this last statement (replace `";"` with `" "` handle all the above?
Or is the idea that newlines are important ?
Can you please add comments explaining the rationale for these substitutions?
##########
tests/sqlparser_common.rs:
##########
@@ -272,20 +275,39 @@ fn parse_insert_default_values() {
"INSERT INTO test_table DEFAULT VALUES (some_column)";
assert_eq!(
ParserError::ParserError("Expected: end of statement, found:
(".to_string()),
-
parse_sql_statements(insert_with_default_values_and_hive_after_columns).unwrap_err()
+ all_dialects_requiring_semicolon_statement_delimiter()
Review Comment:
this query doesn't seem to have multiple statements. Why does the test need
to be changed?
##########
src/parser/mod.rs:
##########
@@ -4541,6 +4561,18 @@ impl<'a> Parser<'a> {
return Ok(vec![]);
}
+ if end_token == Token::SemiColon
+ && self
Review Comment:
do we also have to check the parser options here too?
##########
src/dialect/mssql.rs:
##########
@@ -123,6 +123,10 @@ impl Dialect for MsSqlDialect {
true
}
+ fn supports_statements_without_semicolon_delimiter(&self) -> bool {
Review Comment:
🤦 MS SQL!!!!
--
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
