MINOR: Partial fix for SQL aggregate queries with aliases #2464
Conversation
| async fn csv_query_group_by_with_aliases() -> Result<()> { | ||
| let ctx = SessionContext::new(); | ||
| register_aggregate_csv(&ctx).await?; | ||
| let sql = "SELECT c1 AS c12, avg(c12) AS c1 FROM aggregate_test_100 GROUP BY c1"; |
There was a problem hiding this comment.
This originally failed with Aggregations require unique expression names but the expression \\\"AVG(#aggregate_test_100.c12)\\\" at position 0 and \\\"AVG(#aggregate_test_100.c12)\\\" at position 1 have the same name. Consider aliasing (\\\"AS\\\") one of them
| pub(crate) fn find_column_exprs(exprs: &[Expr]) -> Vec<Expr> { | ||
| find_exprs_in_exprs(exprs, &|nested_expr| matches!(nested_expr, Expr::Column(_))) |
There was a problem hiding this comment.
This code was only finding some expressions and was not recursing and finding them all
andygrove
changed the title
| let mut alias_map = alias_map.clone(); | ||
| for f in plan.schema().fields() { | ||
| alias_map.remove(f.name()); | ||
| } |
There was a problem hiding this comment.
This is the first part of the fix
|
Putting this back to draft for now - test is still failing |
andygrove
changed the title
andygrove
changed the title
|
This is ready for review now |
|
Thanks for the review @yjshen |
| @@ -251,6 +251,77 @@ pub enum Expr { | |||
| QualifiedWildcard { qualifier: String }, | |||
| } | |||
|
|
|||
| /// Recursively find all columns referenced by an expression | |||
| pub fn find_columns_referenced_by_expr(e: &Expr) -> Vec<Column> { | |||
There was a problem hiding this comment.
I think this code could use an ExprVisitor to avoid having to do the recursion itself (and potentially missing some case)
I took a crack at doing so #2471
Which issue does this PR close?
This is working towards a fix for #2430
Rationale for this change
See the added unit test for an example of a query that previously failed.
What changes are included in this PR?
Are there any user-facing changes?
No