Minor: Remove unnecessary clone in datafusion_proto - #7921
Conversation
| use protobuf::repartition_node::PartitionMethod; | ||
| let pb_partition_method = repartition.partition_method.clone().ok_or_else(|| { | ||
| let pb_partition_method = repartition.partition_method.as_ref().ok_or_else(|| { | ||
| DataFusionError::Internal(String::from( | ||
| "Protobuf deserialization error, RepartitionNode was missing required field 'partition_method'", | ||
| )) |
There was a problem hiding this comment.
May be returned early and the clone could be avoided.
| let input_schema = hash_agg | ||
| .input_schema | ||
| .as_ref() | ||
| .ok_or_else(|| { | ||
| DataFusionError::Internal( | ||
| "input_schema in AggregateNode is missing.".to_owned(), | ||
| ) | ||
| })? | ||
| .clone(); | ||
| let physical_schema: SchemaRef = | ||
| SchemaRef::new((&input_schema).try_into()?); | ||
| let input_schema = hash_agg.input_schema.as_ref().ok_or_else(|| { | ||
| DataFusionError::Internal( | ||
| "input_schema in AggregateNode is missing.".to_owned(), | ||
| ) | ||
| })?; | ||
| let physical_schema: SchemaRef = SchemaRef::new(input_schema.try_into()?); |
There was a problem hiding this comment.
The SchemaRef seems able create from ref of input_schema.
There was a problem hiding this comment.
I is confusing, but SchemaRef is an alias for Arc<Schema> which is fast and quick to copy
There was a problem hiding this comment.
The input_schema here is a &protobuf::Schema, and the SchemaRef is an alias of Arc<arrow_schema::Schema>.
This really confusing me at first glance, it's consume a &protobuf::Schema when calling try_into(). Seems fields will be recreated anyway, I think the clone of input_schema could be eliminated.
I don't understand why these convert trait methods convert into U from a &T rather than a T, this makes implicit clones in convert methods.
And I just notice this line could be replaced by physical_schema.
There was a problem hiding this comment.
I agree it is confusing. Any help to make it better would be most appreciated
2 participants
Which issue does this PR close?
Closes #.
Rationale for this change
What changes are included in this PR?
Remove unnecessary clone in
LogicalPlanNode::try_into_logical_planandPhysicalPlanNode::try_into_physical_plan.Are these changes tested?
Are there any user-facing changes?