You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/12/09 17:52:04 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request #1428: Minor Code cleanups
alamb opened a new pull request #1428:
URL: https://github.com/apache/arrow-datafusion/pull/1428
This PR contains some minor style cleanups that I noticed while reviewing https://github.com/apache/arrow-datafusion/pull/1415 from @viirya.
As that PR had already had a lot of discussion, I figured I would just propose the cleanups directly as a follow on PR rather than hold up merging #1415
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1428: Minor Code cleanups
Posted by GitBox <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #1428: Minor Code cleanups
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1428:
URL: https://github.com/apache/arrow-datafusion/pull/1428
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #1428: Minor Code cleanups
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1428:
URL: https://github.com/apache/arrow-datafusion/pull/1428#issuecomment-990191346
Thanks @alamb !
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org