-
Notifications
You must be signed in to change notification settings - Fork 605
Add support for table valued functions for SQL Server #1839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
3686b1b
to
db1c9b2
Compare
CREATE FUNCTION some_inline_tvf(@foo INT, @bar VARCHAR(256)) \ | ||
RETURNS TABLE \ | ||
AS \ | ||
RETURN (SELECT 1 AS col_1)\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parentheses are optional for inline tvf return
queries, although I think the subquery expr expects/requires them currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added support for that syntax & added a new test case example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
src/parser/mod.rs
Outdated
if self.peek_keyword(Keyword::AS) { | ||
self.expect_keyword_is(Keyword::AS)?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if self.peek_keyword(Keyword::AS) { | |
self.expect_keyword_is(Keyword::AS)?; | |
} | |
self.parse_keyword(Keyword::AS); |
this looks like it could be simplified as above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
src/ast/data_type.rs
Outdated
if fields.is_empty() { | ||
return write!(f, "TABLE"); | ||
} | ||
write!(f, "TABLE({})", display_comma_separated(fields)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
let return_type = if return_table.is_some() { | ||
return_table | ||
} else { | ||
Some(self.parse_data_type()?) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah indeed, in this case its one or the other, so should be fine to leave as is.
- rename `AsReturn` to `AsReturnSubquery` for clarity between these two variants
1b92ede
to
999964c
Compare
Done 👍 |
NamedTable( | ||
/// Table name. | ||
ObjectName, | ||
/// Table columns. | ||
Vec<ColumnDef>, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NamedTable( | |
/// Table name. | |
ObjectName, | |
/// Table columns. | |
Vec<ColumnDef>, | |
), | |
NamedTable { | |
/// Table name. | |
table: ObjectName, | |
/// Table columns. | |
columns: Vec<ColumnDef>, | |
}, |
we can use an anonymous struct in this manner?
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(...). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a link to the docs that support NamedTable
variant?
/// RETURNS TABLE | ||
/// AS RETURN SELECT a + b AS sum; | ||
/// ``` | ||
AsReturnSelect(Select), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can also include a reference doc link here?
})); | ||
Ok(DataType::NamedTable( | ||
ObjectName(vec![ObjectNamePart::Identifier(return_table_name)]), | ||
table_column_defs.clone().unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we return an error instead of the unwrap here?
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can replace the if/else statement here with self.expect_keyword(Keyword::TABLE)
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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm is the clone()
here necessary?
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
DataType::Table(Some(t)) if !t.is_empty() => t
})); | ||
Ok(DataType::NamedTable( | ||
ObjectName(vec![ObjectNamePart::Identifier(return_table_name)]), | ||
table_column_defs.clone().unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly here, is the clone
necessary?
let return_type = if return_table.is_some() { | ||
return_table | ||
} else { | ||
Some(self.parse_data_type()?) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah indeed, in this case its one or the other, so should be fine to leave as is.
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 newCreateFunctionBody::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.