You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "aharpervc (via GitHub)" <gi...@apache.org> on 2025/05/06 19:22:59 UTC
[PR] Add support for table valued functions for SQL Serve [datafusion-sqlparser-rs]
aharpervc opened a new pull request, #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839
Note: temporarily rebased on https://github.com/apache/datafusion-sqlparser-rs/pull/1834, prior to that being merged; I will rebase once it's on main.
---
This PR adds support for table valued functions for SQL Server, both inline & multi statement functions. For reference, that's the B & C documentation here: https://learn.microsoft.com/en-us/sql/t-sql/statements/create-function-transact-sql?view=sql-server-ver16#b-create-an-inline-table-valued-function
Inline TVF's are defined with `AS RETURN`, so we have a new `CreateFunctionBody::AsReturn` variant accordingly. Functions using "AS RETURN" don't have BEGIN/END, so that part of the parsing logic is now conditional. Additionally, the data type parser now supports "RETURNS TABLE" without a table definition.
Multi statement TVF's use named table expressions, so a new `NamedTable` data type variant was added. I didn't see a great way to integrate this into the existing data type parser (especially without rewinding), so creating this data type happens inside the parse create function logic first by parsing the identifier, then parsing the table definition, then using those elements to produce a NamedTable.
I also added a new test example for each of these scenarios.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2078474669
##########
tests/sqlparser_mssql.rs:
##########
@@ -283,6 +294,50 @@ fn parse_create_function() {
END\
";
let _ = ms().verified_stmt(create_function_with_return_expression);
+
+ let create_inline_table_value_function = "\
+ CREATE FUNCTION some_inline_tvf(@foo INT, @bar VARCHAR(256)) \
+ RETURNS TABLE \
+ AS \
+ RETURN (SELECT 1 AS col_1)\
Review Comment:
`UNION` is also supported in this syntax but not in this current approach due to using parse_select, which is restricted. I'm comfortable leaving that for later
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2078461916
##########
tests/sqlparser_mssql.rs:
##########
@@ -283,6 +294,50 @@ fn parse_create_function() {
END\
";
let _ = ms().verified_stmt(create_function_with_return_expression);
+
+ let create_inline_table_value_function = "\
+ CREATE FUNCTION some_inline_tvf(@foo INT, @bar VARCHAR(256)) \
+ RETURNS TABLE \
+ AS \
+ RETURN (SELECT 1 AS col_1)\
Review Comment:
I added support for that syntax & added a new test case example
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2087207267
##########
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:
Oops, yes we can. Not sure why I did it that way. Done 👍
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2081901411
##########
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:
Hm, I don't see how that would be done. The use of maybe_parse is to get the table name & other parts, but if we don't have that then we should rewind and just parse a typical data type. Do you mean like having another maybe_parse inside the first maybe_parse?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2078030410
##########
tests/sqlparser_mssql.rs:
##########
@@ -283,6 +294,50 @@ fn parse_create_function() {
END\
";
let _ = ms().verified_stmt(create_function_with_return_expression);
+
+ let create_inline_table_value_function = "\
+ CREATE FUNCTION some_inline_tvf(@foo INT, @bar VARCHAR(256)) \
+ RETURNS TABLE \
+ AS \
+ RETURN (SELECT 1 AS col_1)\
Review Comment:
Parentheses are optional for inline tvf `return` queries, although I think the subquery expr expects/requires them currently.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio merged PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2098125359
##########
src/parser/mod.rs:
##########
@@ -5204,19 +5204,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()?;
+
+ if !p.peek_keyword(Keyword::TABLE) {
+ parser_err!(
+ "Expected TABLE keyword after return type",
+ p.peek_token().span.start
+ )?
+ }
+
+ let table_column_defs = match p.parse_data_type()? {
+ DataType::Table(maybe_table_column_defs) => match maybe_table_column_defs {
+ Some(table_column_defs) => {
+ if table_column_defs.is_empty() {
+ parser_err!(
+ "Expected table column definitions after TABLE keyword",
+ p.peek_token().span.start
+ )?
+ }
+
+ table_column_defs
+ }
+ None => parser_err!(
+ "Expected table column definitions after TABLE keyword",
+ p.peek_token().span.start
+ )?,
+ },
+ _ => parser_err!(
+ "Expected table data type after TABLE keyword",
+ p.peek_token().span.start
+ )?,
+ };
+
+ Ok(DataType::NamedTable {
+ name: ObjectName(vec![ObjectNamePart::Identifier(return_table_name)]),
+ columns: table_column_defs,
+ })
+ })?;
+
+ let return_type = if return_table.is_some() {
+ return_table
+ } else {
+ Some(self.parse_data_type()?)
+ };
- let begin_token = self.expect_keyword(Keyword::BEGIN)?;
- let statements = self.parse_statement_list(&[Keyword::END])?;
- let end_token = self.expect_keyword(Keyword::END)?;
+ let _ = self.parse_keyword(Keyword::AS);
- let function_body = Some(CreateFunctionBody::AsBeginEnd(BeginEndStatements {
- begin_token: AttachedToken(begin_token),
- statements,
- end_token: AttachedToken(end_token),
- }));
+ let function_body = if self.peek_keyword(Keyword::BEGIN) {
+ let begin_token = self.expect_keyword(Keyword::BEGIN)?;
+ let statements = self.parse_statement_list(&[Keyword::END])?;
+ let end_token = self.expect_keyword(Keyword::END)?;
+
+ Some(CreateFunctionBody::AsBeginEnd(BeginEndStatements {
+ begin_token: AttachedToken(begin_token),
+ statements,
+ end_token: AttachedToken(end_token),
+ }))
+ } else if self.parse_keyword(Keyword::RETURN) {
+ if self.peek_token() == Token::LParen {
+ Some(CreateFunctionBody::AsReturnExpr(self.parse_expr()?))
+ } else if self.peek_keyword(Keyword::SELECT) {
+ let select = self.parse_select()?;
+ Some(CreateFunctionBody::AsReturnSelect(select))
+ } else {
+ parser_err!(
+ "Expected a subquery (or bare SELECT statement) after RETURN",
+ self.peek_token().span.start
+ )?
+ }
Review Comment:
Done 👍
##########
src/parser/mod.rs:
##########
@@ -5204,19 +5204,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()?;
+
+ if !p.peek_keyword(Keyword::TABLE) {
+ parser_err!(
+ "Expected TABLE keyword after return type",
+ p.peek_token().span.start
+ )?
+ }
+
+ let table_column_defs = match p.parse_data_type()? {
+ DataType::Table(maybe_table_column_defs) => match maybe_table_column_defs {
+ Some(table_column_defs) => {
+ if table_column_defs.is_empty() {
+ parser_err!(
+ "Expected table column definitions after TABLE keyword",
+ p.peek_token().span.start
+ )?
+ }
+
+ table_column_defs
+ }
+ None => parser_err!(
+ "Expected table column definitions after TABLE keyword",
+ p.peek_token().span.start
+ )?,
+ },
+ _ => parser_err!(
+ "Expected table data type after TABLE keyword",
+ p.peek_token().span.start
+ )?,
+ };
Review Comment:
I consolidated this per your suggestion, and I added a test to assert a parser error for an incorrect table definition 👍
##########
src/parser/mod.rs:
##########
@@ -5204,19 +5204,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()?;
+
+ if !p.peek_keyword(Keyword::TABLE) {
+ parser_err!(
+ "Expected TABLE keyword after return type",
+ p.peek_token().span.start
+ )?
+ }
Review Comment:
Done 👍
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2098076680
##########
src/parser/mod.rs:
##########
@@ -5204,19 +5204,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()?;
+
+ if !p.peek_keyword(Keyword::TABLE) {
+ parser_err!(
+ "Expected TABLE keyword after return type",
+ p.peek_token().span.start
+ )?
+ }
Review Comment:
The reason I didn't do it this way is because expect_keyword (and expect_keyword_is) consume the token. That causes parse_data_type to break, because it uses the TABLE keyword to understand it should parse the table data type.
However, I suppose that I could just call `prev_token()` after expect to undo consuming the token. I will make this change.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2087211209
##########
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:
Done 👍
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2096898613
##########
src/parser/mod.rs:
##########
@@ -5204,19 +5204,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()?;
+
+ if !p.peek_keyword(Keyword::TABLE) {
+ parser_err!(
+ "Expected TABLE keyword after return type",
+ p.peek_token().span.start
+ )?
+ }
+
+ let table_column_defs = match p.parse_data_type()? {
+ DataType::Table(maybe_table_column_defs) => match maybe_table_column_defs {
+ Some(table_column_defs) => {
+ if table_column_defs.is_empty() {
+ parser_err!(
+ "Expected table column definitions after TABLE keyword",
+ p.peek_token().span.start
+ )?
+ }
+
+ table_column_defs
+ }
+ None => parser_err!(
+ "Expected table column definitions after TABLE keyword",
+ p.peek_token().span.start
+ )?,
+ },
+ _ => parser_err!(
+ "Expected table data type after TABLE keyword",
+ p.peek_token().span.start
+ )?,
+ };
+
+ Ok(DataType::NamedTable {
+ name: ObjectName(vec![ObjectNamePart::Identifier(return_table_name)]),
+ columns: table_column_defs,
+ })
+ })?;
+
+ let return_type = if return_table.is_some() {
+ return_table
+ } else {
+ Some(self.parse_data_type()?)
+ };
- let begin_token = self.expect_keyword(Keyword::BEGIN)?;
- let statements = self.parse_statement_list(&[Keyword::END])?;
- let end_token = self.expect_keyword(Keyword::END)?;
+ let _ = self.parse_keyword(Keyword::AS);
- let function_body = Some(CreateFunctionBody::AsBeginEnd(BeginEndStatements {
- begin_token: AttachedToken(begin_token),
- statements,
- end_token: AttachedToken(end_token),
- }));
+ let function_body = if self.peek_keyword(Keyword::BEGIN) {
+ let begin_token = self.expect_keyword(Keyword::BEGIN)?;
+ let statements = self.parse_statement_list(&[Keyword::END])?;
+ let end_token = self.expect_keyword(Keyword::END)?;
+
+ Some(CreateFunctionBody::AsBeginEnd(BeginEndStatements {
+ begin_token: AttachedToken(begin_token),
+ statements,
+ end_token: AttachedToken(end_token),
+ }))
+ } else if self.parse_keyword(Keyword::RETURN) {
+ if self.peek_token() == Token::LParen {
+ Some(CreateFunctionBody::AsReturnExpr(self.parse_expr()?))
+ } else if self.peek_keyword(Keyword::SELECT) {
+ let select = self.parse_select()?;
+ Some(CreateFunctionBody::AsReturnSelect(select))
+ } else {
+ parser_err!(
+ "Expected a subquery (or bare SELECT statement) after RETURN",
+ self.peek_token().span.start
+ )?
+ }
Review Comment:
can we add a test for this behavior? (e.g. one that passes a regular expression and the verifies that the parser rejects it)
##########
src/parser/mod.rs:
##########
@@ -5204,19 +5204,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()?;
+
+ if !p.peek_keyword(Keyword::TABLE) {
+ parser_err!(
+ "Expected TABLE keyword after return type",
+ p.peek_token().span.start
+ )?
+ }
+
+ let table_column_defs = match p.parse_data_type()? {
+ DataType::Table(maybe_table_column_defs) => match maybe_table_column_defs {
+ Some(table_column_defs) => {
+ if table_column_defs.is_empty() {
+ parser_err!(
+ "Expected table column definitions after TABLE keyword",
+ p.peek_token().span.start
+ )?
+ }
+
+ table_column_defs
+ }
+ None => parser_err!(
+ "Expected table column definitions after TABLE keyword",
+ p.peek_token().span.start
+ )?,
+ },
+ _ => parser_err!(
+ "Expected table data type after TABLE keyword",
+ p.peek_token().span.start
+ )?,
+ };
Review Comment:
```suggestion
DataType::Table(Some(table_column_defs)) !if table_column_defs.is_empty() => table_column_defs,
_ => parser_err!(
"Expected table data type after TABLE keyword",
p.peek_token().span.start
)?,
};
```
looks like this condition can be simplified?
Also could we add a negative test to assert the behavior on invalid data_types, noticed it seems to be lacking coverage in the PR tests
##########
src/parser/mod.rs:
##########
@@ -5204,19 +5204,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()?;
+
+ if !p.peek_keyword(Keyword::TABLE) {
+ parser_err!(
+ "Expected TABLE keyword after return type",
+ p.peek_token().span.start
+ )?
+ }
Review Comment:
```suggestion
p.expect_keyword(Keyword::TABLE)?;
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2080647857
##########
src/ast/data_type.rs:
##########
@@ -716,7 +724,15 @@ impl fmt::Display for DataType {
DataType::Unspecified => Ok(()),
DataType::Trigger => write!(f, "TRIGGER"),
DataType::AnyType => write!(f, "ANY TYPE"),
- DataType::Table(fields) => write!(f, "TABLE({})", display_comma_separated(fields)),
+ DataType::Table(fields) => {
+ if fields.is_empty() {
+ return write!(f, "TABLE");
+ }
+ write!(f, "TABLE({})", display_comma_separated(fields))
Review Comment:
I think instead of using the `is_empty` we can make the `fields` value an `Option` and skip the parenthesis only if `fields` is `None`. Otherwise we could run into issues if empty fields is allowed in the other variant of this datatype
##########
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()?)
+ };
- 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 self.peek_keyword(Keyword::AS) {
+ self.expect_keyword_is(Keyword::AS)?;
+ }
Review Comment:
```suggestion
self.parse_keyword(Keyword::AS);
```
this looks like it could be simplified as above?
##########
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:
Could this be included in the `maybe_parse()` logic above? such that the logic returns both the return table and the datatype. based on this condition, they seem to go hand in hand?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2083356065
##########
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(),
+ ))
+ })?;
+
+ let return_type = if return_table.is_some() {
+ return_table
+ } else {
+ Some(self.parse_data_type()?)
+ };
+
+ let _ = self.parse_keyword(Keyword::AS);
+
+ let function_body = if self.peek_keyword(Keyword::BEGIN) {
+ let begin_token = self.expect_keyword(Keyword::BEGIN)?;
+ let statements = self.parse_statement_list(&[Keyword::END])?;
+ let end_token = self.expect_keyword(Keyword::END)?;
+
+ Some(CreateFunctionBody::AsBeginEnd(BeginEndStatements {
+ begin_token: AttachedToken(begin_token),
+ statements,
+ end_token: AttachedToken(end_token),
+ }))
+ } else if self.parse_keyword(Keyword::RETURN) {
+ if self.peek_token() == Token::LParen {
+ let expr = self.parse_expr()?;
+ if !matches!(expr, Expr::Subquery(_)) {
+ parser_err!(
+ "Expected a subquery after RETURN",
+ self.peek_token().span.start
+ )?
+ }
+ Some(CreateFunctionBody::AsReturnSubquery(expr))
Review Comment:
```suggestion
Some(CreateFunctionBody::AsReturnExpr(expr))
```
thinking since a subquery is already an expr, we can be more permissive here?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#issuecomment-2892157468
It seems the CI failure for the lint job was addressed by https://github.com/apache/datafusion-sqlparser-rs/pull/1856. I've rebased again, all good 👍
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2078474669
##########
tests/sqlparser_mssql.rs:
##########
@@ -283,6 +294,50 @@ fn parse_create_function() {
END\
";
let _ = ms().verified_stmt(create_function_with_return_expression);
+
+ let create_inline_table_value_function = "\
+ CREATE FUNCTION some_inline_tvf(@foo INT, @bar VARCHAR(256)) \
+ RETURNS TABLE \
+ AS \
+ RETURN (SELECT 1 AS col_1)\
Review Comment:
`UNION` is also supported in this syntax, but I'm comfortable leaving that for later
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2081895986
##########
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()?)
+ };
- 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 self.peek_keyword(Keyword::AS) {
+ self.expect_keyword_is(Keyword::AS)?;
+ }
Review Comment:
Done 👍
##########
src/ast/data_type.rs:
##########
@@ -716,7 +724,15 @@ impl fmt::Display for DataType {
DataType::Unspecified => Ok(()),
DataType::Trigger => write!(f, "TRIGGER"),
DataType::AnyType => write!(f, "ANY TYPE"),
- DataType::Table(fields) => write!(f, "TABLE({})", display_comma_separated(fields)),
+ DataType::Table(fields) => {
+ if fields.is_empty() {
+ return write!(f, "TABLE");
+ }
+ write!(f, "TABLE({})", display_comma_separated(fields))
Review Comment:
Done 👍
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#issuecomment-2866937145
> took a quick look and left a couple comments, @aharpervc could you rebase on main now that the other PR has landed, in order to remove the extra diff?
Done 👍
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2087154451
##########
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(),
+ ))
+ })?;
+
+ let return_type = if return_table.is_some() {
+ return_table
+ } else {
+ Some(self.parse_data_type()?)
+ };
+
+ let _ = self.parse_keyword(Keyword::AS);
+
+ let function_body = if self.peek_keyword(Keyword::BEGIN) {
+ let begin_token = self.expect_keyword(Keyword::BEGIN)?;
+ let statements = self.parse_statement_list(&[Keyword::END])?;
+ let end_token = self.expect_keyword(Keyword::END)?;
+
+ Some(CreateFunctionBody::AsBeginEnd(BeginEndStatements {
+ begin_token: AttachedToken(begin_token),
+ statements,
+ end_token: AttachedToken(end_token),
+ }))
+ } else if self.parse_keyword(Keyword::RETURN) {
+ if self.peek_token() == Token::LParen {
+ let expr = self.parse_expr()?;
+ if !matches!(expr, Expr::Subquery(_)) {
+ parser_err!(
+ "Expected a subquery after RETURN",
+ self.peek_token().span.start
+ )?
+ }
+ Some(CreateFunctionBody::AsReturnSubquery(expr))
Review Comment:
I think this is a "could" vs "should" situation. The question here is what is the parser expecting? I don't think it makes sense to parse arbitrary expr's if they wouldn't otherwise be allowed by any sql engine... that seems better as a parse failure.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for table valued functions for SQL Server [datafusion-sqlparser-rs]
Posted by "aharpervc (via GitHub)" <gi...@apache.org>.
aharpervc commented on code in PR #1839:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1839#discussion_r2087214184
##########
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:
Done 👍
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org