-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix group by aliased expression in LogicalPLanBuilder::aggregate #8629
Conversation
/// | ||
/// This allows MySQL style selects like | ||
/// `SELECT col FROM t WHERE pk = 5` if col is unique | ||
fn add_group_by_exprs_from_dependencies( |
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 pulled the logic into its own function so I could document it better
let expr = | ||
Expr::Column(Column::new(field.qualifier().cloned(), field.name())); | ||
let expr_name = expr.display_name()?; | ||
if !group_by_field_names.contains(&expr_name) { |
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 is the change -- rather than comparing the Expr
s it compares their display_name
(the fields they will create)
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.
Thanks @alamb LGTM!
Thank you for the review @mustafasrepo |
Which issue does this PR close?
Closes #8628
Rationale for this change
Fixed regression introduced in #8356 as explained on #8628
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
Fix bug