iffyio commented on code in PR #1839:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2082704398
##########
src/parser/mod.rs:
##########
@@ -5203,19 +5203,79 @@ impl<'a> Parser<'a> {
let (name, args) = self.parse_create_function_name_and_params()?;
self.expect_keyword(Keyword::RETURNS)?;
- let return_type = Some(self.parse_data_type()?);
- self.expect_keyword_is(Keyword::AS)?;
+ let return_table = self.maybe_parse(|p| {
+ let return_table_name = p.parse_identifier()?;
+ let table_column_defs = if p.peek_keyword(Keyword::TABLE) {
Review Comment:
I think we can replace the if/else statement here with
`self.expect_keyword(Keyword::TABLE)`
##########
src/ast/mod.rs:
##########
@@ -8660,6 +8660,28 @@ pub enum CreateFunctionBody {
///
/// [PostgreSQL]:
https://www.postgresql.org/docs/current/sql-createfunction.html
Return(Expr),
+
+ /// Function body expression using the 'AS RETURN' keywords
+ ///
+ /// Example:
+ /// ```sql
+ /// CREATE FUNCTION myfunc(a INT, b INT)
+ /// RETURNS TABLE
+ /// AS RETURN (SELECT a + b AS sum);
+ /// ```
+ ///
+ /// [MsSql]:
https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql
+ AsReturnSubquery(Expr),
+
+ /// Function body expression using the 'AS RETURN' keywords, with an
un-parenthesized SELECT query
+ ///
+ /// Example:
+ /// ```sql
+ /// CREATE FUNCTION myfunc(a INT, b INT)
+ /// RETURNS TABLE
+ /// AS RETURN SELECT a + b AS sum;
+ /// ```
+ AsReturnSelect(Select),
Review Comment:
maybe we can also include a reference doc link here?
##########
src/parser/mod.rs:
##########
@@ -5203,19 +5203,79 @@ impl<'a> Parser<'a> {
let (name, args) = self.parse_create_function_name_and_params()?;
self.expect_keyword(Keyword::RETURNS)?;
- let return_type = Some(self.parse_data_type()?);
- self.expect_keyword_is(Keyword::AS)?;
+ let return_table = self.maybe_parse(|p| {
+ let return_table_name = p.parse_identifier()?;
+ let table_column_defs = if p.peek_keyword(Keyword::TABLE) {
+ match p.parse_data_type()? {
+ DataType::Table(t) => t,
+ _ => parser_err!(
+ "Expected table data type after TABLE keyword",
+ p.peek_token().span.start
+ )?,
+ }
+ } else {
+ parser_err!(
+ "Expected TABLE keyword after return type",
+ p.peek_token().span.start
+ )?
+ };
- let begin_token = self.expect_keyword(Keyword::BEGIN)?;
- let statements = self.parse_statement_list(&[Keyword::END])?;
- let end_token = self.expect_keyword(Keyword::END)?;
+ if table_column_defs.is_none()
+ || table_column_defs.clone().is_some_and(|tcd| tcd.is_empty())
+ {
+ parser_err!(
+ "Expected table column definitions after TABLE keyword",
+ p.peek_token().span.start
+ )?
+ }
- let function_body =
Some(CreateFunctionBody::AsBeginEnd(BeginEndStatements {
- begin_token: AttachedToken(begin_token),
- statements,
- end_token: AttachedToken(end_token),
- }));
+ Ok(DataType::NamedTable(
+
ObjectName(vec![ObjectNamePart::Identifier(return_table_name)]),
+ table_column_defs.clone().unwrap(),
Review Comment:
can we return an error instead of the unwrap here?
##########
src/parser/mod.rs:
##########
@@ -5203,19 +5203,79 @@ impl<'a> Parser<'a> {
let (name, args) = self.parse_create_function_name_and_params()?;
self.expect_keyword(Keyword::RETURNS)?;
- let return_type = Some(self.parse_data_type()?);
- self.expect_keyword_is(Keyword::AS)?;
+ let return_table = self.maybe_parse(|p| {
+ let return_table_name = p.parse_identifier()?;
+ let table_column_defs = if p.peek_keyword(Keyword::TABLE) {
+ match p.parse_data_type()? {
+ DataType::Table(t) => t,
Review Comment:
I think already here we can check that the returned data type is none empty,
that would avoid the option invalid case propagating further below (where we
have the is_none and is_some checks). e.g.
```rust
DataType::Table(Some(t)) if !t.is_empty() => t
```
##########
src/parser/mod.rs:
##########
@@ -5203,19 +5203,73 @@ impl<'a> Parser<'a> {
let (name, args) = self.parse_create_function_name_and_params()?;
self.expect_keyword(Keyword::RETURNS)?;
- let return_type = Some(self.parse_data_type()?);
- self.expect_keyword_is(Keyword::AS)?;
+ let return_table = self.maybe_parse(|p| {
+ let return_table_name = p.parse_identifier()?;
+ let table_column_defs = if p.peek_keyword(Keyword::TABLE) {
+ match p.parse_data_type()? {
+ DataType::Table(t) => t,
+ _ => parser_err!(
+ "Expected table data type after TABLE keyword",
+ p.peek_token().span.start
+ )?,
+ }
+ } else {
+ parser_err!(
+ "Expected TABLE keyword after return type",
+ p.peek_token().span.start
+ )?
+ };
+ Ok(DataType::NamedTable(
+
ObjectName(vec![ObjectNamePart::Identifier(return_table_name)]),
+ table_column_defs.clone(),
+ ))
+ })?;
+
+ let return_type = if return_table.is_some() {
+ return_table
+ } else {
+ Some(self.parse_data_type()?)
+ };
Review Comment:
Ah indeed, in this case its one or the other, so should be fine to leave as
is.
##########
src/parser/mod.rs:
##########
@@ -5203,19 +5203,79 @@ impl<'a> Parser<'a> {
let (name, args) = self.parse_create_function_name_and_params()?;
self.expect_keyword(Keyword::RETURNS)?;
- let return_type = Some(self.parse_data_type()?);
- self.expect_keyword_is(Keyword::AS)?;
+ let return_table = self.maybe_parse(|p| {
+ let return_table_name = p.parse_identifier()?;
+ let table_column_defs = if p.peek_keyword(Keyword::TABLE) {
+ match p.parse_data_type()? {
+ DataType::Table(t) => t,
+ _ => parser_err!(
+ "Expected table data type after TABLE keyword",
+ p.peek_token().span.start
+ )?,
+ }
+ } else {
+ parser_err!(
+ "Expected TABLE keyword after return type",
+ p.peek_token().span.start
+ )?
+ };
- let begin_token = self.expect_keyword(Keyword::BEGIN)?;
- let statements = self.parse_statement_list(&[Keyword::END])?;
- let end_token = self.expect_keyword(Keyword::END)?;
+ if table_column_defs.is_none()
+ || table_column_defs.clone().is_some_and(|tcd| tcd.is_empty())
Review Comment:
hmm is the `clone()` here necessary?
##########
src/ast/data_type.rs:
##########
@@ -48,7 +48,15 @@ pub enum DataType {
/// Table type in [PostgreSQL], e.g. CREATE FUNCTION RETURNS TABLE(...).
///
/// [PostgreSQL]:
https://www.postgresql.org/docs/15/sql-createfunction.html
- Table(Vec<ColumnDef>),
+ /// [MsSQL]:
https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql?view=sql-server-ver16#c-create-a-multi-statement-table-valued-function
+ Table(Option<Vec<ColumnDef>>),
+ /// Table type with a name, e.g. CREATE FUNCTION RETURNS @result
TABLE(...).
Review Comment:
Could we add a link to the docs that support `NamedTable` variant?
##########
src/parser/mod.rs:
##########
@@ -5203,19 +5203,79 @@ impl<'a> Parser<'a> {
let (name, args) = self.parse_create_function_name_and_params()?;
self.expect_keyword(Keyword::RETURNS)?;
- let return_type = Some(self.parse_data_type()?);
- self.expect_keyword_is(Keyword::AS)?;
+ let return_table = self.maybe_parse(|p| {
+ let return_table_name = p.parse_identifier()?;
+ let table_column_defs = if p.peek_keyword(Keyword::TABLE) {
+ match p.parse_data_type()? {
+ DataType::Table(t) => t,
+ _ => parser_err!(
+ "Expected table data type after TABLE keyword",
+ p.peek_token().span.start
+ )?,
+ }
+ } else {
+ parser_err!(
+ "Expected TABLE keyword after return type",
+ p.peek_token().span.start
+ )?
+ };
- let begin_token = self.expect_keyword(Keyword::BEGIN)?;
- let statements = self.parse_statement_list(&[Keyword::END])?;
- let end_token = self.expect_keyword(Keyword::END)?;
+ if table_column_defs.is_none()
+ || table_column_defs.clone().is_some_and(|tcd| tcd.is_empty())
+ {
+ parser_err!(
+ "Expected table column definitions after TABLE keyword",
+ p.peek_token().span.start
+ )?
+ }
- let function_body =
Some(CreateFunctionBody::AsBeginEnd(BeginEndStatements {
- begin_token: AttachedToken(begin_token),
- statements,
- end_token: AttachedToken(end_token),
- }));
+ Ok(DataType::NamedTable(
+
ObjectName(vec![ObjectNamePart::Identifier(return_table_name)]),
+ table_column_defs.clone().unwrap(),
Review Comment:
similarly here, is the `clone` necessary?
##########
src/ast/data_type.rs:
##########
@@ -48,7 +48,15 @@ pub enum DataType {
/// Table type in [PostgreSQL], e.g. CREATE FUNCTION RETURNS TABLE(...).
///
/// [PostgreSQL]:
https://www.postgresql.org/docs/15/sql-createfunction.html
- Table(Vec<ColumnDef>),
+ /// [MsSQL]:
https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql?view=sql-server-ver16#c-create-a-multi-statement-table-valued-function
+ Table(Option<Vec<ColumnDef>>),
+ /// Table type with a name, e.g. CREATE FUNCTION RETURNS @result
TABLE(...).
+ NamedTable(
+ /// Table name.
+ ObjectName,
+ /// Table columns.
+ Vec<ColumnDef>,
+ ),
Review Comment:
```suggestion
NamedTable {
/// Table name.
table: ObjectName,
/// Table columns.
columns: Vec<ColumnDef>,
},
```
we can use an anonymous struct in this manner?
--
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
