Build aggregate schema in Aggregate::try_new #3739
Conversation
| let grouping_expr: Vec<Expr> = grouping_set_to_exprlist(group_expr.as_slice())?; | ||
|
|
||
| let all_expr = grouping_expr.iter().chain(aggr_expr.iter()); | ||
| validate_unique_names("Aggregations", all_expr.clone())?; | ||
| let aggr_schema = DFSchema::new_with_metadata( | ||
| exprlist_to_fields(all_expr, &self.plan)?, | ||
| self.plan.schema().metadata().clone(), | ||
| )?; |
There was a problem hiding this comment.
This code is moved into Aggregate::try_new
| let grouping_expr: Vec<Expr> = grouping_set_to_exprlist(group_expr.as_slice())?; | ||
| let all_expr = grouping_expr.iter().chain(aggr_expr.iter()); | ||
| validate_unique_names("Aggregations", all_expr.clone())?; | ||
| let schema = DFSchema::new_with_metadata( | ||
| exprlist_to_fields(all_expr, &input)?, | ||
| input.schema().metadata().clone(), | ||
| )?; |
There was a problem hiding this comment.
This is the code moved from LogicalPlanBuilder::aggregate
| let new_schema = DFSchema::new_with_metadata( | ||
| schema | ||
| .fields() | ||
| .iter() | ||
| .filter(|x| new_required_columns.contains(&x.qualified_column())) | ||
| .cloned() | ||
| .collect(), | ||
| schema.metadata().clone(), | ||
| )?; |
There was a problem hiding this comment.
This code was problematic and is now replaced with the code used elsewhere
There was a problem hiding this comment.
yes I agree having the caller have to specify (correctly) the aggregate schema is a recipe for disaster
| let new_schema = DFSchema::new_with_metadata( | ||
| schema | ||
| .fields() | ||
| .iter() | ||
| .filter(|x| new_required_columns.contains(&x.qualified_column())) | ||
| .cloned() | ||
| .collect(), | ||
| schema.metadata().clone(), | ||
| )?; |
There was a problem hiding this comment.
yes I agree having the caller have to specify (correctly) the aggregate schema is a recipe for disaster
|
CI failure appears unrelated: #3743 |
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
|
Benchmark runs are scheduled for baseline = 8dcef91 and contender = 38cf2eb. 38cf2eb is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3738
Rationale for this change
We had duplicate code for building an aggregate schema. This code is now inside
Aggregate::try_newso we don't have to duplicate it everwhere,What changes are included in this PR?
Refactoring to eliminate duplicate code for building aggregate schema
Are there any user-facing changes?