-
Notifications
You must be signed in to change notification settings - Fork 558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for parsing MsSql alias with equals #1467
Conversation
src/parser/mod.rs
Outdated
/// 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 | ||
/// <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> | ||
fn parse_mssql_alias_with_equal(&mut self, expr: &Expr) -> Option<SelectItem> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn parse_mssql_alias_with_equal(&mut self, expr: &Expr) -> Option<SelectItem> { | |
fn maybe_unpack_alias_assignment(expr: &Expr) -> Option<SelectItem> { |
Thinking since this is only a helper function that it doesn't get confused for a parser/parsing method
src/parser/mod.rs
Outdated
/// 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 | ||
/// <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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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 | |
/// <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> | |
/// 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 |
src/parser/mod.rs
Outdated
if let Expr::BinaryOp { | ||
left, op, right, .. | ||
} = expr | ||
{ | ||
if op == &BinaryOperator::Eq { | ||
if let Expr::Identifier(ref alias) = **left { | ||
return Some(SelectItem::ExprWithAlias { | ||
expr: *right.clone(), | ||
alias: alias.clone(), | ||
}); | ||
} | ||
} | ||
} | ||
|
||
None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if let Expr::BinaryOp { | |
left, op, right, .. | |
} = expr | |
{ | |
if op == &BinaryOperator::Eq { | |
if let Expr::Identifier(ref alias) = **left { | |
return Some(SelectItem::ExprWithAlias { | |
expr: *right.clone(), | |
alias: alias.clone(), | |
}); | |
} | |
} | |
} | |
None | |
if let Expr::BinaryOp { | |
left: Expr::Identifier(alias), op: BinaryOperator::Eq, right, | |
} = expr { | |
SelectItem::ExprWithAlias { | |
expr: right, | |
alias: Expr::Identifier(alias), | |
} | |
} else { | |
expr | |
} |
Could be simplified to the following? I think no need to pass in a reference and return an option, we can move the value and return the same value if untouched?
Also we can skip the ..
operator when deconstructing in this case, so that if the field set changes, we would be notified to ensure the logic remains correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this would make the function return two different types, as SelectItem is not an Expr variant, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah that's correct, actually I'm thinking in that case we can inline the function since its small enough?
383226c
to
cb7eeed
Compare
@iffyio I tried inlining the function, it's not as short as I wish it would, due to pattern matching the boxed fields. But maybe I'm missing something... Would love your feedback. |
src/dialect/mod.rs
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// For example: SELECT col_alias = col FROM tbl | |
/// 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
Outdated
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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 |
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
src/parser/mod.rs
Outdated
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), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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), | |
}) | |
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you can match to Expr::Identifier
in L11182 because left
is a boxed field. This further complicates the logic of the function because you now need another if let
condition to check the value of left
. The complexity ends up similar to what I suggested.
I suggest going back to the helper function design, it cleanly encapsulates this rather edgy case of MsSql without complicating the main flow of parsing the common syntax for aliasing a column.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works as its own match clause for example:
Expr::BinaryOp {
left,
op: BinaryOperator::Eq,
right,
} if self.dialect.supports_eq_alias_assigment() && matches!(left.as_ref(), Expr::Identifier(_)) => {
let Expr::Identifier(alias) = *left else {
return parser_err!(
"BUG: expected identifier expression as alias",
self.peek_token().location
);
};
Ok(SelectItem::ExprWithAlias {
expr: *right,
alias,
})
}
I think the least desirable option involves the expression cloning, i.e. the right.clone()
I think we want to avoid in any case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive! Adopted in the next commit
tests/sqlparser_mssql.rs
Outdated
@@ -1024,6 +1024,17 @@ fn parse_create_table_with_identity_column() { | |||
} | |||
} | |||
|
|||
#[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? 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?
@iffyio Fixed all comments but went back to a helper function design as the elegance of the |
Pull Request Test Coverage Report for Build 11334438657Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
92ea0f4
to
e9de85d
Compare
@iffyio I adopted your change to inline the function in the latest commit |
|
||
#[test] | ||
fn test_alias_equal_expr() { | ||
let dialects = all_dialects_where(|d| d.supports_eq_alias_assigment()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yoavcloud just one comment on the test from here if we can include a similar case for the other dialects verifying that the some_alias = some_column
gets parsed as an actual expression vs an alias?
Beyond that I think this should be good to go, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! cc @alamb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @yoavcloud and @iffyio
This PR addresses the MsSql syntax for select item alias using the
=
operator, for example: