alamb commented on a change in pull request #1428:
URL: https://github.com/apache/arrow-datafusion/pull/1428#discussion_r766026560
##########
File path: datafusion/src/logical_plan/builder.rs
##########
@@ -485,37 +485,30 @@ impl LogicalPlanBuilder {
{
let input_schema = input.schema();
- let mut new_expr = Vec::new();
- new_expr.extend(expr);
- missing_cols.iter().try_for_each::<_, Result<()>>(|c| {
- new_expr.push(normalize_col(Expr::Column(c.clone()),
&input)?);
+ let missing_exprs = missing_cols
+ .iter()
+ .map(|c| normalize_col(Expr::Column(c.clone()), &input))
+ .collect::<Result<Vec<_>>>()?;
- Ok(())
- })?;
+ expr.extend(missing_exprs);
- let new_inputs =
-
curr_plan.inputs().into_iter().cloned().collect::<Vec<_>>();
-
- let new_schema =
- DFSchema::new(exprlist_to_fields(&new_expr,
input_schema)?)?;
+ let new_schema = DFSchema::new(exprlist_to_fields(&expr,
input_schema)?)?;
Ok(LogicalPlan::Projection(Projection {
- expr: new_expr,
- input: Arc::new(new_inputs.get(0).unwrap().clone()),
+ expr,
+ input,
schema: DFSchemaRef::new(new_schema),
alias,
}))
}
_ => {
- let inputs = curr_plan.inputs();
- let mut new_inputs = vec![];
- inputs.iter().try_for_each::<_, Result<()>>(|input_plan| {
- let new_input =
- self.add_missing_columns((*input_plan).clone(),
missing_cols)?;
- new_inputs.push(new_input);
-
- Ok(())
- })?;
+ let new_inputs = curr_plan
Review comment:
same thing here -- just avoids the need for `mut` (and I think it is
clearer what is going on now too)
##########
File path: datafusion/src/logical_plan/builder.rs
##########
@@ -485,37 +485,30 @@ impl LogicalPlanBuilder {
{
let input_schema = input.schema();
- let mut new_expr = Vec::new();
- new_expr.extend(expr);
- missing_cols.iter().try_for_each::<_, Result<()>>(|c| {
- new_expr.push(normalize_col(Expr::Column(c.clone()),
&input)?);
+ let missing_exprs = missing_cols
Review comment:
this just rejiggers stuff to avoid the need for `mut` -- not critical
but I think it is more concise
##########
File path: datafusion/src/logical_plan/builder.rs
##########
@@ -467,16 +467,16 @@ impl LogicalPlanBuilder {
})))
}
- /// Add missing sort columns to downstream projection
+ /// Add missing sort columns to all downstream projection
fn add_missing_columns(
&self,
curr_plan: LogicalPlan,
missing_cols: &[Column],
) -> Result<LogicalPlan> {
- match curr_plan.clone() {
+ match curr_plan {
Review comment:
no need to clone -- we already take `cur_plan` by value
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
