iffyio commented on code in PR #1790:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1790#discussion_r2025317055
##########
src/ast/mod.rs:
##########
@@ -7900,6 +7909,36 @@ impl Display for MergeClause {
}
}
+/// A Output in the end of a 'MERGE' Statement
+///
+/// Example:
+/// OUTPUT $action, deleted.* INTO dbo.temp_products;
+///
[mssql](https://learn.microsoft.com/en-us/sql/t-sql/queries/output-clause-transact-sql)
+
Review Comment:
```suggestion
```
##########
tests/sqlparser_mssql.rs:
##########
@@ -1921,3 +1921,9 @@ fn ms() -> TestedDialects {
fn ms_and_generic() -> TestedDialects {
TestedDialects::new(vec![Box::new(MsSqlDialect {}),
Box::new(GenericDialect {})])
}
+
+#[test]
+fn parse_mssql_merge_with_output() {
+ let stmt = "MERGE dso.products AS t USING dsi.products AS s ON s.ProductID
= t.ProductID WHEN MATCHED AND NOT (t.ProductName = s.ProductName OR
(ISNULL(t.ProductName, s.ProductName) IS NULL)) THEN UPDATE SET t.ProductName =
s.ProductName WHEN NOT MATCHED BY TARGET THEN INSERT (ProductID, ProductName)
VALUES (s.ProductID, s.ProductName) WHEN NOT MATCHED BY SOURCE THEN DELETE
OUTPUT $action, deleted.ProductID INTO dsi.temp_products";
Review Comment:
to help with readability could we use line breaks for formatting here
similar to the `test_merge_with_output` test case in?
##########
src/ast/mod.rs:
##########
@@ -5407,14 +5410,20 @@ impl fmt::Display for Statement {
source,
on,
clauses,
+ output,
} => {
write!(
f,
"MERGE{int} {table} USING {source} ",
int = if *into { " INTO" } else { "" }
)?;
write!(f, "ON {on} ")?;
- write!(f, "{}", display_separated(clauses, " "))
+ write!(f, "{}", display_separated(clauses, " "))?;
+ if output.is_some() {
+ let out = output.clone().unwrap();
+ write!(f, " {out}")?;
+ }
Review Comment:
```suggestion
if let Some(output) = output {
write!(f, " {output}")?;
}
```
we could rewrite it to use existing convention in order to replace the unwrap
##########
src/parser/mod.rs:
##########
@@ -14586,6 +14589,29 @@ impl<'a> Parser<'a> {
Ok(clauses)
}
+ pub fn parse_output(&mut self) -> Result<Output, ParserError> {
+ self.expect_keyword_is(Keyword::OUTPUT)?;
+ let select_items = self.parse_projection()?;
+ self.expect_keyword_is(Keyword::INTO)?;
+ let temporary = self
+ .parse_one_of_keywords(&[Keyword::TEMP, Keyword::TEMPORARY])
+ .is_some();
+ let unlogged = self.parse_keyword(Keyword::UNLOGGED);
+ let table = self.parse_keyword(Keyword::TABLE);
+ let name = self.parse_object_name(false)?;
+ let into_table = SelectInto {
+ temporary,
+ unlogged,
+ table,
+ name,
+ };
Review Comment:
This looks to be the same code as in `parse_select`? could we factor that
into a function like `parse_into_clause()` that could be reused in both places?
##########
src/parser/mod.rs:
##########
@@ -14586,6 +14589,29 @@ impl<'a> Parser<'a> {
Ok(clauses)
}
+ pub fn parse_output(&mut self) -> Result<Output, ParserError> {
Review Comment:
```suggestion
fn parse_output(&mut self) -> Result<Output, ParserError> {
```
##########
src/parser/mod.rs:
##########
@@ -14489,7 +14489,10 @@ impl<'a> Parser<'a> {
pub fn parse_merge_clauses(&mut self) -> Result<Vec<MergeClause>,
ParserError> {
let mut clauses = vec![];
loop {
- if self.peek_token() == Token::EOF || self.peek_token() ==
Token::SemiColon {
+ if self.peek_token() == Token::EOF
+ || self.peek_token() == Token::SemiColon
+ || self.peek_keyword(Keyword::OUTPUT)
Review Comment:
hmm I wonder in order to avoid accumulating conditions here over time, would
it make sense to change to something like this? replacing the EOF, Semicolon
and OUTPUT conditions
```rust
loop {
if !self.parse_keyword(Keyword::WHEN) {
break
}
// ...
}
```
##########
src/ast/mod.rs:
##########
@@ -3828,6 +3829,8 @@ pub enum Statement {
on: Box<Expr>,
/// Specifies the actions to perform when values match or do not match.
clauses: Vec<MergeClause>,
+ // Speccifies the output to save changes in MSSQL
Review Comment:
```suggestion
// Specifies the output to save changes in MSSQL
```
##########
src/ast/mod.rs:
##########
@@ -7900,6 +7909,36 @@ impl Display for MergeClause {
}
}
+/// A Output in the end of a 'MERGE' Statement
+///
+/// Example:
+/// OUTPUT $action, deleted.* INTO dbo.temp_products;
+///
[mssql](https://learn.microsoft.com/en-us/sql/t-sql/queries/output-clause-transact-sql)
+
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub struct Output {
Review Comment:
```suggestion
pub struct OutputClause {
```
thinking maybe calling it `OutputClause` per their docs would be better?
since `Output` would be a bit too generic out without context
--
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
