You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "lovasoa (via GitHub)" <gi...@apache.org> on 2025/03/10 21:55:56 UTC
[PR] add support for with clauses in delete statements [datafusion-sqlparser-rs]
lovasoa opened a new pull request, #1764:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1764
fixes https://github.com/apache/datafusion-sqlparser-rs/issues/1763
--
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 `with` clauses (CTEs) in `delete` statements [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1764:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1764#discussion_r1988453837
##########
src/parser/mod.rs:
##########
@@ -10202,19 +10209,25 @@ impl<'a> Parser<'a> {
}
}
+ /// Parse a `WITH` clause, i.e. a `WITH` keyword followed by a `RECURSIVE` keyword
+ /// and a comma-separated list of CTE declarations.
+ fn parse_with_clause(&mut self) -> Result<With, ParserError> {
+ let with_token = self.get_current_token();
+ Ok(With {
+ with_token: with_token.clone().into(),
+ recursive: self.parse_keyword(Keyword::RECURSIVE),
+ cte_tables: self.parse_comma_separated(Parser::parse_cte)?,
+ })
+ }
Review Comment:
oh was there a requirement to not have the logic inlined as before?
--
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 `with` clauses (CTEs) in `delete` statements [datafusion-sqlparser-rs]
Posted by "lovasoa (via GitHub)" <gi...@apache.org>.
lovasoa commented on code in PR #1764:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1764#discussion_r1990001185
##########
src/parser/mod.rs:
##########
@@ -10202,19 +10209,25 @@ impl<'a> Parser<'a> {
}
}
+ /// Parse a `WITH` clause, i.e. a `WITH` keyword followed by a `RECURSIVE` keyword
+ /// and a comma-separated list of CTE declarations.
+ fn parse_with_clause(&mut self) -> Result<With, ParserError> {
+ let with_token = self.get_current_token();
+ Ok(With {
+ with_token: with_token.clone().into(),
+ recursive: self.parse_keyword(Keyword::RECURSIVE),
+ cte_tables: self.parse_comma_separated(Parser::parse_cte)?,
+ })
+ }
Review Comment:
i inlined it back
--
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 `with` clauses (CTEs) in `delete` statements [datafusion-sqlparser-rs]
Posted by "lovasoa (via GitHub)" <gi...@apache.org>.
lovasoa commented on code in PR #1764:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1764#discussion_r1988500551
##########
src/parser/mod.rs:
##########
@@ -10202,19 +10209,25 @@ impl<'a> Parser<'a> {
}
}
+ /// Parse a `WITH` clause, i.e. a `WITH` keyword followed by a `RECURSIVE` keyword
+ /// and a comma-separated list of CTE declarations.
+ fn parse_with_clause(&mut self) -> Result<With, ParserError> {
+ let with_token = self.get_current_token();
+ Ok(With {
+ with_token: with_token.clone().into(),
+ recursive: self.parse_keyword(Keyword::RECURSIVE),
+ cte_tables: self.parse_comma_separated(Parser::parse_cte)?,
+ })
+ }
Review Comment:
No such requirement other than readability and consistency. Do you want me to re-inline it ?
--
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 `with` clauses (CTEs) in `delete` statements [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio merged PR #1764:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1764
--
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 `with` clauses (CTEs) in `delete` statements [datafusion-sqlparser-rs]
Posted by "lovasoa (via GitHub)" <gi...@apache.org>.
lovasoa commented on code in PR #1764:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1764#discussion_r1988500551
##########
src/parser/mod.rs:
##########
@@ -10202,19 +10209,25 @@ impl<'a> Parser<'a> {
}
}
+ /// Parse a `WITH` clause, i.e. a `WITH` keyword followed by a `RECURSIVE` keyword
+ /// and a comma-separated list of CTE declarations.
+ fn parse_with_clause(&mut self) -> Result<With, ParserError> {
+ let with_token = self.get_current_token();
+ Ok(With {
+ with_token: with_token.clone().into(),
+ recursive: self.parse_keyword(Keyword::RECURSIVE),
+ cte_tables: self.parse_comma_separated(Parser::parse_cte)?,
+ })
+ }
Review Comment:
No such requirement (other than readability and consistency). Do you want me to re-inline it ?
--
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 `with` clauses (CTEs) in `delete` statements [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1764:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1764#discussion_r1989848061
##########
src/parser/mod.rs:
##########
@@ -10202,19 +10209,25 @@ impl<'a> Parser<'a> {
}
}
+ /// Parse a `WITH` clause, i.e. a `WITH` keyword followed by a `RECURSIVE` keyword
+ /// and a comma-separated list of CTE declarations.
+ fn parse_with_clause(&mut self) -> Result<With, ParserError> {
+ let with_token = self.get_current_token();
+ Ok(With {
+ with_token: with_token.clone().into(),
+ recursive: self.parse_keyword(Keyword::RECURSIVE),
+ cte_tables: self.parse_comma_separated(Parser::parse_cte)?,
+ })
+ }
Review Comment:
Ah I see, was initially wondering in the diff if i missed something and figured to double check. yeah I think it we can have it inlined instead, the new function partially contains the impl since the `WITH` part is parsed elsewhere which might be a bit confusing, might as well leave it inline i figure
--
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