iffyio commented on code in PR #1808:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1808#discussion_r2043612190
##########
src/ast/mod.rs:
##########
@@ -9203,6 +9232,30 @@ pub enum CopyIntoSnowflakeKind {
Location,
}
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub struct ReturnStatement {
+ pub value: Option<ReturnStatementValue>,
+}
+
+impl fmt::Display for ReturnStatement {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match &self.value {
+ Some(ReturnStatementValue::Expr(expr)) => write!(f, "RETURN {}",
expr),
+ None => write!(f, "RETURN")
+ }
+ }
+}
+
+/// Variants of the Mssql `RETURN` statement
Review Comment:
```suggestion
/// Variants of a `RETURN` statement
```
since this statement isn't mysql specific
##########
src/parser/mod.rs:
##########
@@ -5135,6 +5142,69 @@ impl<'a> Parser<'a> {
}))
}
+ /// Parse `CREATE FUNCTION` for [MsSql]
+ ///
+ /// [MsSql]:
https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql
+ fn parse_mssql_create_function(
+ &mut self,
+ or_alter: bool,
+ or_replace: bool,
+ temporary: bool,
+ ) -> Result<Statement, ParserError> {
+ let name = self.parse_object_name(false)?;
+
+ let parse_function_param =
+ |parser: &mut Parser| -> Result<OperateFunctionArg, ParserError> {
+ let name = parser.parse_identifier()?;
+ let data_type = parser.parse_data_type()?;
+ Ok(OperateFunctionArg {
+ mode: None,
+ name: Some(name),
+ data_type,
+ default_expr: None,
+ })
+ };
+ self.expect_token(&Token::LParen)?;
+ let args = self.parse_comma_separated0(parse_function_param,
Token::RParen)?;
+ self.expect_token(&Token::RParen)?;
+
+ let return_type = if self.parse_keyword(Keyword::RETURNS) {
+ Some(self.parse_data_type()?)
+ } else {
+ None
+ };
Review Comment:
This part looks identical to the [bigquery
implementation](https://github.com/apache/datafusion-sqlparser-rs/blob/1b50dd9d0764b96d9a6b4e81f77d77e7cae84097/src/parser/mod.rs#L5064-L5085)
could we pull the logic out into a function to be reused by both?
##########
src/ast/mod.rs:
##########
@@ -4050,6 +4051,13 @@ pub enum Statement {
arguments: Vec<Expr>,
options: Vec<RaisErrorOption>,
},
+ /// Return (Mssql)
+ ///
+ /// for Functions:
+ /// RETURN scalar_expression
+ ///
+ /// See:
https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql
Review Comment:
Maybe we can move this to the `ReturnStatement` struct definition, and here
we can only point to the statement like `Parse a [ReturnStatement]`? Thinking
having it on the struct will be more visible as the struct can be
reused/reached from more places
##########
src/ast/mod.rs:
##########
@@ -8368,6 +8387,22 @@ pub enum CreateFunctionBody {
///
/// [BigQuery]:
https://cloud.google.com/bigquery/docs/reference/standard-sql/data-definition-language#syntax_11
AsAfterOptions(Expr),
+ /// Function body with statements before the `RETURN` keyword.
+ ///
+ /// Example:
+ /// ```sql
+ /// CREATE FUNCTION my_scalar_udf(a INT, b INT)
+ /// RETURNS INT
+ /// AS
+ /// BEGIN
+ /// DECLARE c INT;
+ /// SET c = a + b;
+ /// RETURN c;
+ /// END;
+ /// ```
+ ///
+ /// [SQL Server]:
https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql
+ MultiStatement(Vec<Statement>),
Review Comment:
Yeah I think we ideally have it in this PR since this adds a new API that
will otherwise need to be changed
##########
src/ast/mod.rs:
##########
@@ -4050,6 +4051,13 @@ pub enum Statement {
arguments: Vec<Expr>,
options: Vec<RaisErrorOption>,
},
+ /// Return (Mssql)
+ ///
+ /// for Functions:
+ /// RETURN scalar_expression
+ ///
+ /// See:
https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql
Review Comment:
```suggestion
/// Return
///
/// for Functions:
/// RETURN scalar_expression
///
/// [MsSql]:
https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql
```
since the statement is supported by other dialects
##########
src/parser/mod.rs:
##########
@@ -15017,6 +15075,13 @@ impl<'a> Parser<'a> {
}
}
+ fn parse_return(&mut self) -> Result<Statement, ParserError> {
+ let expr = self.parse_expr()?;
Review Comment:
Maybe we can remove the return statement bits from this PR and tackle it
separately as its own PR?
Otherwise indeed the expectation would be that this uses maybe_parse and we
have the test cases in place as part of this PR
##########
src/parser/mod.rs:
##########
@@ -5135,6 +5146,63 @@ impl<'a> Parser<'a> {
}))
}
+ /// Parse `CREATE FUNCTION` for [SQL Server]
+ ///
+ /// [SQL Server]:
https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql
+ fn parse_mssql_create_function(
+ &mut self,
+ or_alter: bool,
+ or_replace: bool,
+ temporary: bool,
+ ) -> Result<Statement, ParserError> {
+ let name = self.parse_object_name(false)?;
+
+ let parse_function_param =
+ |parser: &mut Parser| -> Result<OperateFunctionArg, ParserError> {
+ let name = parser.parse_identifier()?;
+ let data_type = parser.parse_data_type()?;
+ Ok(OperateFunctionArg {
+ mode: None,
+ name: Some(name),
+ data_type,
+ default_expr: None,
+ })
+ };
+ self.expect_token(&Token::LParen)?;
+ let args = self.parse_comma_separated0(parse_function_param,
Token::RParen)?;
+ self.expect_token(&Token::RParen)?;
+
+ let return_type = if self.parse_keyword(Keyword::RETURNS) {
+ Some(self.parse_data_type()?)
+ } else {
+ None
+ };
+
+ self.expect_keyword_is(Keyword::AS)?;
+ self.expect_keyword_is(Keyword::BEGIN)?;
+ let function_body =
Some(CreateFunctionBody::MultiStatement(self.parse_statements()?));
Review Comment:
> but I'm not yet sure how to actually implement that statement parsing logic
hmm not sure I followed what we mean here? parsing a semicolon-less
statement I imagine would follow from the above code snippet by calling
`self.parse_statements_inner(false)`
It would indeed be good to resolve in this PR, currently in the PR we seem
to have a workaround for this behavior
[here](https://github.com/apache/datafusion-sqlparser-rs/blob/1b50dd9d0764b96d9a6b4e81f77d77e7cae84097/src/parser/mod.rs#L487-L493)
and
[here](https://github.com/apache/datafusion-sqlparser-rs/blob/1b50dd9d0764b96d9a6b4e81f77d77e7cae84097/src/parser/mod.rs#L5180-L5185)
and another in the Go statement PR.
##########
src/parser/mod.rs:
##########
@@ -5135,6 +5146,63 @@ impl<'a> Parser<'a> {
}))
}
+ /// Parse `CREATE FUNCTION` for [SQL Server]
+ ///
+ /// [SQL Server]:
https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql
Review Comment:
Yeah the mssql name used in the library is unfortunate, but it wouldn't be
worth the breakage to rename the [dialect
itself](https://github.com/apache/datafusion-sqlparser-rs/blob/main/src/dialect/mssql.rs#L29-L31)
and the documentation in the code follows from there, so that I think it
would be preferable to remain consistent with the naming at this point even if
the name isn't optimal
##########
src/parser/mod.rs:
##########
@@ -5135,6 +5142,69 @@ impl<'a> Parser<'a> {
}))
}
+ /// Parse `CREATE FUNCTION` for [MsSql]
+ ///
+ /// [MsSql]:
https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql
+ fn parse_mssql_create_function(
+ &mut self,
+ or_alter: bool,
+ or_replace: bool,
+ temporary: bool,
+ ) -> Result<Statement, ParserError> {
+ let name = self.parse_object_name(false)?;
+
+ let parse_function_param =
+ |parser: &mut Parser| -> Result<OperateFunctionArg, ParserError> {
+ let name = parser.parse_identifier()?;
+ let data_type = parser.parse_data_type()?;
+ Ok(OperateFunctionArg {
+ mode: None,
+ name: Some(name),
+ data_type,
+ default_expr: None,
+ })
+ };
+ self.expect_token(&Token::LParen)?;
+ let args = self.parse_comma_separated0(parse_function_param,
Token::RParen)?;
+ self.expect_token(&Token::RParen)?;
+
+ let return_type = if self.parse_keyword(Keyword::RETURNS) {
+ Some(self.parse_data_type()?)
+ } else {
+ None
+ };
Review Comment:
Yeah it sounds reasonable in that case to expect the returns clause
--
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
