iffyio commented on code in PR #1467:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1467#discussion_r1798869042
##########
src/dialect/mod.rs:
##########
@@ -561,6 +561,11 @@ pub trait Dialect: Debug + Any {
fn supports_asc_desc_in_column_definition(&self) -> bool {
false
}
+
+ /// For example: SELECT col_alias = col FROM tbl
Review Comment:
```suggestion
/// Returns true if this dialect supports treating the equals operator
`=` within a [`SelectItem`]
/// as an alias assignment operator, rather than a boolean expression.
/// For example: the following statements are equivalent for such a
dialect:
/// ```sql
/// SELECT col_alias = col FROM tbl;
/// SELECT col_alias AS col FROM tbl;
/// ```
```
Something like this to illustrate what the flag implies Im thinking?
##########
src/parser/mod.rs:
##########
@@ -11173,12 +11173,36 @@ impl<'a> Parser<'a> {
self.peek_token().location
)
}
- expr => self
- .parse_optional_alias(keywords::RESERVED_FOR_COLUMN_ALIAS)
- .map(|alias| match alias {
- Some(alias) => SelectItem::ExprWithAlias { expr, alias },
- None => SelectItem::UnnamedExpr(expr),
- }),
+ expr => {
+ // Parse a [`SelectItem`] based on an [MsSql] syntax that uses
the equal sign
+ // to denote an alias, for example: SELECT col_alias = col
FROM tbl
+ // [MsSql]:
https://learn.microsoft.com/en-us/sql/t-sql/queries/select-examples-transact-sql?view=sql-server-ver16#b-use-select-with-column-headings-and-calculations
+ let expr = if self.dialect.supports_eq_alias_assigment() {
+ if let Expr::BinaryOp {
+ ref left,
+ op: BinaryOperator::Eq,
+ ref right,
+ } = expr
+ {
+ if let Expr::Identifier(alias) = left.as_ref() {
+ return Ok(SelectItem::ExprWithAlias {
+ expr: *right.clone(),
+ alias: alias.clone(),
+ });
+ }
+ }
+ expr
+ } else {
+ expr
+ };
+
+ // Parse the common AS keyword for aliasing a column
+ self.parse_optional_alias(keywords::RESERVED_FOR_COLUMN_ALIAS)
+ .map(|alias| match alias {
+ Some(alias) => SelectItem::ExprWithAlias { expr, alias
},
+ None => SelectItem::UnnamedExpr(expr),
+ })
Review Comment:
```suggestion
let (expr, alias) = match expr {
Expr::BinaryOp {
left: Expr::Identifier(alias),
BinaryOperator::Eq,
right,
} if self.dialect.supports_eq_alias_assigment() => {
(expr, Some(alias))
}
expr => {
// Parse the common AS keyword for aliasing a column
self.parse_optional_alias(keywords::RESERVED_FOR_COLUMN_ALIAS)?
}
};
Ok(alias
.map(|alias| SelectItem::ExprWithAlias{ expr, alias })
.unwrap_or_else(|| SelectItem::UnnamedExpr(expr)))
```
The current inline looks reasonable, I think its mostly rustfmt that making
it look a like it has bit more code than it does, but I think we could simplify
the logic a bit as above to help with that and readability/intent as well?
hopefully?
##########
src/parser/mod.rs:
##########
@@ -11173,12 +11173,36 @@ impl<'a> Parser<'a> {
self.peek_token().location
)
}
- expr => self
- .parse_optional_alias(keywords::RESERVED_FOR_COLUMN_ALIAS)
- .map(|alias| match alias {
- Some(alias) => SelectItem::ExprWithAlias { expr, alias },
- None => SelectItem::UnnamedExpr(expr),
- }),
+ expr => {
+ // Parse a [`SelectItem`] based on an [MsSql] syntax that uses
the equal sign
+ // to denote an alias, for example: SELECT col_alias = col
FROM tbl
+ // [MsSql]:
https://learn.microsoft.com/en-us/sql/t-sql/queries/select-examples-transact-sql?view=sql-server-ver16#b-use-select-with-column-headings-and-calculations
Review Comment:
```suggestion
```
We can add the mssql link as part of the trait impl for the dialect instead?
Then here I think we would be able to skip the comment altogether since the
`self.dialect.supports_eq_alias_assignment()` along with the documentation on
the trait definition for the method would suffice. It'll let us keep the doc
for the feature centralized
##########
tests/sqlparser_mssql.rs:
##########
@@ -1024,6 +1024,17 @@ fn parse_create_table_with_identity_column() {
}
}
+#[test]
Review Comment:
Ah sorry I forgot to mention this part earlier, the test looks reasonable
but now with the method on the dialect, can we move the test to common.rs and
write it in a [similar style to
this](https://github.com/apache/datafusion-sqlparser-rs/blob/cb7eeed2aa7b4fdeb402e0df6e56a8b662633efb/tests/sqlparser_common.rs#L2781)?
that would allow any future dialects with the same support to be covered
automatically by the test.
Also for this feature, can we include an assertion that for the dialects
that don't support the flag, verifying that they indeed end up with an unnamed
select item?
--
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
