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<SelectItem, ParserError> {
+ 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:
<https://docs.snowflake.com/en/sql-reference/constructs/connect-by>
+ 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
function's length
##########
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)
Review Comment:
```suggestion
/// e.g. `SELECT CONNECT_BY_ROOT name FROM Tbl2` (Snowflake)
```
##########
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
Review Comment:
```suggestion
/// Returns reserved keywords that may prefix a select item expression
```
--
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]
