iffyio commented on code in PR #2185:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2185#discussion_r2746608179
##########
tests/sqlparser_common.rs:
##########
@@ -12785,6 +12785,61 @@ fn parse_connect_by() {
"prior"
)))]
);
+
+ // no START WITH and NOCYCLE
+ let connect_by_5 = "SELECT child, parent FROM t CONNECT BY NOCYCLE parent
= PRIOR child";
+ assert_eq!(
+ dialects.verified_only_select(connect_by_5),
+ Select {
+ select_token: AttachedToken::empty(),
+ optimizer_hint: None,
+ distinct: None,
+ top: None,
+ top_before_distinct: false,
+ projection: vec![
+ SelectItem::UnnamedExpr(Expr::Identifier(Ident::new("child"))),
+
SelectItem::UnnamedExpr(Expr::Identifier(Ident::new("parent"))),
+ ],
+ exclude: None,
+ from: vec![TableWithJoins {
+ relation:
table_from_name(ObjectName::from(vec![Ident::new("t")])),
+ joins: vec![],
+ }],
+ into: None,
+ lateral_views: vec![],
+ prewhere: None,
+ selection: None,
+ group_by: GroupByExpr::Expressions(vec![], vec![]),
+ cluster_by: vec![],
+ distribute_by: vec![],
+ sort_by: vec![],
+ having: None,
+ named_window: vec![],
+ qualify: None,
+ window_before_qualify: false,
+ value_table_mode: None,
+ connect_by: Some(ConnectBy {
+ condition: None,
+ relationships: vec![Expr::BinaryOp {
+ left: Expr::Identifier(Ident::new("parent")).into(),
+ op: BinaryOperator::Eq,
+ right:
Expr::Prior(Expr::Identifier(Ident::new("child")).into()).into(),
+ }],
+ nocycle: true,
+ }),
+ flavor: SelectFlavor::Standard,
+ }
+ );
+
+ // ~ CONNECT BY after WHERE and before GROUP BY
Review Comment:
```suggestion
// CONNECT BY after WHERE and before GROUP BY
```
##########
src/parser/mod.rs:
##########
@@ -14185,25 +14185,35 @@ impl<'a> Parser<'a> {
/// Parse a `CONNECT BY` clause (Oracle-style hierarchical query support).
pub fn parse_connect_by(&mut self) -> Result<ConnectBy, ParserError> {
- let (condition, relationships) = if
self.parse_keywords(&[Keyword::CONNECT, Keyword::BY]) {
- let relationships = self.with_state(ParserState::ConnectBy,
|parser| {
- parser.parse_comma_separated(Parser::parse_expr)
+ let (condition, relationships, nocycle) = if self
+ .parse_keywords(&[Keyword::CONNECT, Keyword::BY])
+ {
+ let (relationships, nocycle) =
self.with_state(ParserState::ConnectBy, |parser| {
Review Comment:
wondering a couple things around the current function impl
The current impl essentially duplicates the code in the if and else blocks.
Since the goal is solely to parse in arbitrary order, can we rewrite it to use
a loop instead? e.g.
```rust
loop {
if self.parse_keywords(STARTS, WITH) {
} else if self.parse_keywords(CONNECT, BY) {
} else {
break
}
}
```
I think the function would be much easier to follow as a result.
Also, can we instead of having three separate but fields `relationships` and
`nocycle` and `condition`, can we use a representation like e.g.
```rust
enum ConnectByKind {
ConnectBy{
relationships: Vec<Expr>,
nocycle: bool,
},
StartsWith(Expr)
}
connect_by: Vec<ConnectByKind>
```
and the order in which the clauses show up in the query will be preserved as
a result too (and we skip the consideration of whether snowflake supports
specific ordering)
##########
src/parser/mod.rs:
##########
@@ -14000,6 +14000,17 @@ impl<'a> Parser<'a> {
None
};
+ let connect_by = if self.dialect.supports_connect_by()
+ && self
+ .parse_one_of_keywords(&[Keyword::START, Keyword::CONNECT])
+ .is_some()
+ {
+ self.prev_token();
Review Comment:
```suggestion
&& self
.peek_one_of_keywords(&[Keyword::START, Keyword::CONNECT])
.is_some()
{
```
would this be equivalent?
##########
src/ast/query.rs:
##########
@@ -374,6 +374,8 @@ pub struct Select {
pub prewhere: Option<Expr>,
/// WHERE
pub selection: Option<Expr>,
+ /// [START WITH ..] CONNECT BY ..
+ pub connect_by: Option<ConnectBy>,
Review Comment:
Can we add a link to the docs where this syntax comes from?
##########
src/ast/query.rs:
##########
@@ -1094,23 +1094,34 @@ impl fmt::Display for TableWithJoins {
/// Joins a table to itself to process hierarchical data in the table.
///
/// See <https://docs.snowflake.com/en/sql-reference/constructs/connect-by>.
+/// See
<https://docs.oracle.com/en/database/oracle/oracle-database/21/sqlrf/Hierarchical-Queries.html>
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
pub struct ConnectBy {
/// START WITH
- pub condition: Expr,
+ pub condition: Option<Expr>,
/// CONNECT BY
pub relationships: Vec<Expr>,
+ /// [CONNECT BY] NOCYCLE
+ pub nocycle: bool,
Review Comment:
Similarly, can we add a link to the docs for this syntax?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
