Re: [PR] Support for projection item prefix operator (CONNECT_BY_ROOT) [datafusion-sqlparser-rs]

2025-04-28 Thread via GitHub


tomershaniii commented on PR #1780:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1780#issuecomment-2836104032

   Hi @iffyio , please see latest commit.


-- 
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



Re: [PR] Support for projection item prefix operator (CONNECT_BY_ROOT) [datafusion-sqlparser-rs]

2025-04-27 Thread via GitHub


iffyio commented on code in PR #1780:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1780#discussion_r2062522304


##
src/parser/mod.rs:
##
@@ -15375,6 +15391,17 @@ impl<'a> Parser<'a> {
 }
 }
 
+fn prefixed_expr(expr: Expr, prefix: Option) -> Expr {

Review Comment:
   ```suggestion
   fn maybe_prefixed_expr(expr: Expr, prefix: Option) -> Expr {
   ```
   thinking in order to flag that the prefixing is optional



##
tests/sqlparser_snowflake.rs:
##
@@ -3983,3 +3983,38 @@ fn test_nested_join_without_parentheses() {
 }],
 );
 }
+
+#[test]
+fn parse_connect_by_root_operator() {
+let sql = "SELECT CONNECT_BY_ROOT name AS root_name FROM Tbl1";
+
+match snowflake().verified_stmt(sql) {
+Statement::Query(query) => {
+assert_eq!(
+query.body.as_select().unwrap().projection[0],
+SelectItem::ExprWithAlias {
+expr: Expr::Prefixed {
+prefix: Ident::new("CONNECT_BY_ROOT"),
+value: Box::new(Expr::Identifier(Ident::new("name")))
+},
+alias: Ident::new("root_name"),
+}
+);
+}
+_ => unreachable!(),
+}
+
+let sql = "SELECT CONNECT_BY_ROOT name FROM Tbl2";
+match snowflake().verified_stmt(sql) {
+Statement::Query(query) => {
+assert_eq!(
+query.body.as_select().unwrap().projection[0],
+SelectItem::UnnamedExpr(Expr::Prefixed {
+prefix: Ident::new("CONNECT_BY_ROOT"),
+value: Box::new(Expr::Identifier(Ident::new("name")))
+})
+);
+}
+_ => unreachable!(),

Review Comment:
   can we add a test case for e.g. `SELECT connect_by_root FROM T`? i.e. one 
that uses the keyword as the projected column name



-- 
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



Re: [PR] Support for projection item prefix operator (CONNECT_BY_ROOT) [datafusion-sqlparser-rs]

2025-04-26 Thread via GitHub


tomershaniii commented on PR #1780:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1780#issuecomment-2832681512

   Hi @iffyio , Modified the PR per you suggestion, please see if this is 
aligned to what you have in mind.


-- 
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



Re: [PR] Support for projection item prefix operator (CONNECT_BY_ROOT) [datafusion-sqlparser-rs]

2025-04-09 Thread via GitHub


iffyio commented on PR #1780:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1780#issuecomment-2791591778

   Marking as draft in the meantime as this PR is no longer pending review. 
@tomershaniii please feel free to undraft and ping when ready!


-- 
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



Re: [PR] Support for projection item prefix operator (CONNECT_BY_ROOT) [datafusion-sqlparser-rs]

2025-04-05 Thread via GitHub


tomershaniii commented on code in PR #1780:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1780#discussion_r2024616388


##
tests/sqlparser_postgres.rs:
##
@@ -4911,17 +4965,17 @@ fn parse_array_agg() {
 let sql = r#"SELECT GREATEST(sections_tbl.*) AS sections FROM 
sections_tbl"#;
 pg().verified_stmt(sql);
 
-// follows special-case array_agg code path
-let sql2 = "SELECT ARRAY_AGG(sections_tbl.*) AS sections FROM 
sections_tbl";
-pg().verified_stmt(sql2);
+// // follows special-case array_agg code path
+// let sql2 = "SELECT ARRAY_AGG(sections_tbl.*) AS sections FROM 
sections_tbl";
+// pg().verified_stmt(sql2);
 
-// handles multi-part identifier with general code path
-let sql3 = "SELECT GREATEST(my_schema.sections_tbl.*) AS sections FROM 
sections_tbl";
-pg().verified_stmt(sql3);
+// // handles multi-part identifier with general code path
+// let sql3 = "SELECT GREATEST(my_schema.sections_tbl.*) AS sections FROM 
sections_tbl";
+// pg().verified_stmt(sql3);
 
-// handles multi-part identifier with array_agg code path
-let sql4 = "SELECT ARRAY_AGG(my_schema.sections_tbl.*) AS sections FROM 
sections_tbl";
-pg().verified_stmt(sql4);
+// // handles multi-part identifier with array_agg code path
+// let sql4 = "SELECT ARRAY_AGG(my_schema.sections_tbl.*) AS sections FROM 
sections_tbl";
+// pg().verified_stmt(sql4);

Review Comment:
   yes, missed that



-- 
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



Re: [PR] Support for projection item prefix operator (CONNECT_BY_ROOT) [datafusion-sqlparser-rs]

2025-04-01 Thread via GitHub


iffyio commented on code in PR #1780:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1780#discussion_r2018413958


##
src/ast/spans.rs:
##
@@ -1722,8 +1722,20 @@ impl Spanned for SelectItemQualifiedWildcardKind {
 impl Spanned for SelectItem {
 fn span(&self) -> Span {
 match self {
-SelectItem::UnnamedExpr(expr) => expr.span(),
-SelectItem::ExprWithAlias { expr, alias } => 
expr.span().union(&alias.span),
+SelectItem::UnnamedExpr { expr, prefix } => expr
+.span()
+.union_opt(&prefix.as_ref().map(|expr| expr.span())),
+
+SelectItem::ExprWithAlias {
+expr,
+alias,
+prefix,
+} => {
+// let x = &prefix.map(|i| i.span());

Review Comment:
   ```suggestion
   ```



##
src/parser/mod.rs:
##
@@ -1237,6 +1237,23 @@ impl<'a> Parser<'a> {
 }
 }
 
+//Select item operators

Review Comment:
   ```suggestion
   /// Select item operators
   ```
   for the doc comment can we describe what the function does?



##
tests/sqlparser_postgres.rs:
##
@@ -4911,17 +4965,17 @@ fn parse_array_agg() {
 let sql = r#"SELECT GREATEST(sections_tbl.*) AS sections FROM 
sections_tbl"#;
 pg().verified_stmt(sql);
 
-// follows special-case array_agg code path
-let sql2 = "SELECT ARRAY_AGG(sections_tbl.*) AS sections FROM 
sections_tbl";
-pg().verified_stmt(sql2);
+// // follows special-case array_agg code path
+// let sql2 = "SELECT ARRAY_AGG(sections_tbl.*) AS sections FROM 
sections_tbl";
+// pg().verified_stmt(sql2);
 
-// handles multi-part identifier with general code path
-let sql3 = "SELECT GREATEST(my_schema.sections_tbl.*) AS sections FROM 
sections_tbl";
-pg().verified_stmt(sql3);
+// // handles multi-part identifier with general code path
+// let sql3 = "SELECT GREATEST(my_schema.sections_tbl.*) AS sections FROM 
sections_tbl";
+// pg().verified_stmt(sql3);
 
-// handles multi-part identifier with array_agg code path
-let sql4 = "SELECT ARRAY_AGG(my_schema.sections_tbl.*) AS sections FROM 
sections_tbl";
-pg().verified_stmt(sql4);
+// // handles multi-part identifier with array_agg code path
+// let sql4 = "SELECT ARRAY_AGG(my_schema.sections_tbl.*) AS sections FROM 
sections_tbl";
+// pg().verified_stmt(sql4);

Review Comment:
   should these be uncommented?



##
src/dialect/mod.rs:
##
@@ -888,6 +888,12 @@ pub trait Dialect: Debug + Any {
 keywords::RESERVED_FOR_TABLE_FACTOR
 }
 
+/// Returns reserved keywords for projection item prefix operator
+/// e.g. SELECT CONNECT_BY_ROOT name FROM Tbl2 (Snowflake)
+fn get_reserved_keywords_for_select_item_operator(&self) -> &[Keyword] {

Review Comment:
   ```suggestion
   fn get_reserved_keywords_for_select_item(&self) -> &[Keyword] {
   ```



##
src/parser/mod.rs:
##
@@ -13732,6 +13749,10 @@ impl<'a> Parser<'a> {
 
 /// Parse a comma-delimited list of projections after SELECT
 pub fn parse_select_item(&mut self) -> Result {
+let prefix = self
+.maybe_parse(|parser| 
parser.parse_select_item_prefix_by_reserved_word())?
+.flatten();
+
 match self.parse_wildcard_expr()? {

Review Comment:
   Thinking introducing the prefix field into the select item is a bit more 
invasive and would be nice to avoid if it makes sense. I noticed this use case 
is similar in representation to 
[IntroducedString](https://github.com/apache/datafusion-sqlparser-rs/blob/main/src/ast/mod.rs#L929-L934),
 maybe we can repurpose that one to be generic.
   
   I'm thinking something like this?
   
   changing that enum variant into
   ```rust
   Expr::Prefixed { prefix: Ident, expr: Expr }
   ```
   
   Then impl wise here we could wrap the `self.parse_wildcard_expr()` call with 
the logic to optionally parse the prefix
   
   ```rust
   fn parse_select_item_expr() {
 let prefix = 
self.parse_one_of_keywords(self.dialect.get_reserved_keywords_for_select_item());
   
 let expr = self.parse_wildcard_expr()?
   
 if let Some(prefix) = prefix {
   Expr::Prefixed {
 prefix: Ident::new(prefix),
 expr,
   }
 } else {
   expr
 }
   }
   ```
   



##
src/dialect/snowflake.rs:
##
@@ -346,6 +346,13 @@ impl Dialect for SnowflakeDialect {
 fn supports_group_by_expr(&self) -> bool {
 true
 }
+
+/// See: 

+fn get_reserved_keywords_for_select_item_operator(&self) -> &[Keyword] {
+const RESERVED_KEYWORDS_FOR_SELECT_ITEM_OPERATOR: [Keyword; 1] = 
[Keyword::CONNECT_BY_ROOT];

Review Comment:
   could we move this to the top of the file maybe? thinking it would be more 
visible there and if the list happens to get long it doesnt add to the