Minor: Remove some clone in TypeCoercion
#10203
Conversation
| } | ||
|
|
||
| /// Returns `expressions` coerced to types compatible with | ||
| /// `signature`, if possible. | ||
| /// | ||
| /// See the module level documentation for more detail on coercion. | ||
| fn coerce_arguments_for_signature( | ||
| expressions: &[Expr], | ||
| expressions: Vec<Expr>, |
There was a problem hiding this comment.
this will stop all the arguments to function calls being copied, which seems good
There was a problem hiding this comment.
I always thought having less possible coercible type is preferred 🤔
There was a problem hiding this comment.
I am not sure what you mean
Perhaps you are referring to preferring &[Expr] over &Vec<Expr>? If so the difference here is that the Vec was owned (and thus we don't have to copy the contents via to_vec())
| .into_iter() | ||
| .map(|expr| { | ||
| let data_type = expr.get_type(schema).unwrap(); | ||
| if let DataType::FixedSizeList(field, _) = data_type { | ||
| let field = field.as_ref().clone(); | ||
| let to_type = DataType::List(Arc::new(field)); | ||
| let to_type = DataType::List(field.clone()); |
There was a problem hiding this comment.
FieldRef is an Arc<Field> -- so this clone is just an Arc::clone (rather than copying the entire filed)
| } | ||
|
|
||
| /// Cast `expr` to the specified type, if possible | ||
| fn cast_expr(expr: &Expr, to_type: &DataType, schema: &DFSchema) -> Result<Expr> { |
There was a problem hiding this comment.
This function just wrapped Expr::cast_to in an API that forced a clone, so I removed it
|
Thank you @alamb |
* Remove some clone in TypeCoercion * Less clone * less copy
Which issue does this PR close?
Part of #9637
Rationale for this change
While working on #10039 I saw a few copies that could be avoided that are independent of the larger refactor (which is proving trickier than I had hoped)
What changes are included in this PR?
clones inExprRewriter(these are pretty minor in the grand scheme of things)Are these changes tested?
Existing CI
Are there any user-facing changes?