Skip to content

Commit f204869

Browse files
lewiszlwlewiszlw
authored
Enable clone_on_ref_ptr clippy lint on functions* (#11468)
* Enable clone_on_ref_ptr clippy lint on functions * Remove unnecessary Arc::clone
1 parent 7bd0e74 commit f204869

File tree

27 files changed

+106
-85
lines changed

27 files changed

+106
-85
lines changed

‎datafusion/functions-aggregate/src/correlation.rs‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
2020
use std::any::Any;
2121
use std::fmt::Debug;
22+
use std::sync::Arc;
2223

2324
use arrow::compute::{and, filter, is_not_null};
2425
use arrow::{
@@ -192,13 +193,21 @@ impl Accumulator for CorrelationAccumulator {
192193

193194
fn merge_batch(&mut self, states: &[ArrayRef]) -> Result<()> {
194195
let states_c = [
195-
states[0].clone(),
196-
states[1].clone(),
197-
states[3].clone(),
198-
states[5].clone(),
196+
Arc::clone(&states[0]),
197+
Arc::clone(&states[1]),
198+
Arc::clone(&states[3]),
199+
Arc::clone(&states[5]),
200+
];
201+
let states_s1 = [
202+
Arc::clone(&states[0]),
203+
Arc::clone(&states[1]),
204+
Arc::clone(&states[2]),
205+
];
206+
let states_s2 = [
207+
Arc::clone(&states[0]),
208+
Arc::clone(&states[3]),
209+
Arc::clone(&states[4]),
199210
];
200-
let states_s1 = [states[0].clone(), states[1].clone(), states[2].clone()];
201-
let states_s2 = [states[0].clone(), states[3].clone(), states[4].clone()];
202211

203212
self.covar.merge_batch(&states_c)?;
204213
self.stddev1.merge_batch(&states_s1)?;

‎datafusion/functions-aggregate/src/first_last.rs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ impl FirstValueAccumulator {
247247
.iter()
248248
.zip(self.ordering_req.iter())
249249
.map(|(values, req)| SortColumn {
250-
values: values.clone(),
250+
values: Arc::clone(values),
251251
options: Some(req.options),
252252
})
253253
.collect::<Vec<_>>();
@@ -547,7 +547,7 @@ impl LastValueAccumulator {
547547
// Take the reverse ordering requirement. This enables us to
548548
// use "fetch = 1" to get the last value.
549549
SortColumn {
550-
values: values.clone(),
550+
values: Arc::clone(values),
551551
options: Some(!req.options),
552552
}
553553
})
@@ -676,7 +676,7 @@ fn convert_to_sort_cols(
676676
arrs.iter()
677677
.zip(sort_exprs.iter())
678678
.map(|(item, sort_expr)| SortColumn {
679-
values: item.clone(),
679+
values: Arc::clone(item),
680680
options: Some(sort_expr.options),
681681
})
682682
.collect::<Vec<_>>()
@@ -707,7 +707,7 @@ mod tests {
707707
for arr in arrs {
708708
// Once first_value is set, accumulator should remember it.
709709
// It shouldn't update first_value for each new batch
710-
first_accumulator.update_batch(&[arr.clone()])?;
710+
first_accumulator.update_batch(&[Arc::clone(&arr)])?;
711711
// last_value should be updated for each new batch.
712712
last_accumulator.update_batch(&[arr])?;
713713
}
@@ -733,12 +733,12 @@ mod tests {
733733
let mut first_accumulator =
734734
FirstValueAccumulator::try_new(&DataType::Int64, &[], vec![], false)?;
735735

736-
first_accumulator.update_batch(&[arrs[0].clone()])?;
736+
first_accumulator.update_batch(&[Arc::clone(&arrs[0])])?;
737737
let state1 = first_accumulator.state()?;
738738

739739
let mut first_accumulator =
740740
FirstValueAccumulator::try_new(&DataType::Int64, &[], vec![], false)?;
741-
first_accumulator.update_batch(&[arrs[1].clone()])?;
741+
first_accumulator.update_batch(&[Arc::clone(&arrs[1])])?;
742742
let state2 = first_accumulator.state()?;
743743

744744
assert_eq!(state1.len(), state2.len());
@@ -763,12 +763,12 @@ mod tests {
763763
let mut last_accumulator =
764764
LastValueAccumulator::try_new(&DataType::Int64, &[], vec![], false)?;
765765

766-
last_accumulator.update_batch(&[arrs[0].clone()])?;
766+
last_accumulator.update_batch(&[Arc::clone(&arrs[0])])?;
767767
let state1 = last_accumulator.state()?;
768768

769769
let mut last_accumulator =
770770
LastValueAccumulator::try_new(&DataType::Int64, &[], vec![], false)?;
771-
last_accumulator.update_batch(&[arrs[1].clone()])?;
771+
last_accumulator.update_batch(&[Arc::clone(&arrs[1])])?;
772772
let state2 = last_accumulator.state()?;
773773

774774
assert_eq!(state1.len(), state2.len());

‎datafusion/functions-aggregate/src/lib.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
// KIND, either express or implied. See the License for the
1515
// specific language governing permissions and limitations
1616
// under the License.
17+
// Make cheap clones clear: https://github.com/apache/datafusion/issues/11143
18+
#![deny(clippy::clone_on_ref_ptr)]
1719

1820
//! Aggregate Function packages for [DataFusion].
1921
//!

‎datafusion/functions-array/src/array_has.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,7 @@ fn general_array_has_dispatch<O: OffsetSizeTrait>(
279279

280280
let converter = RowConverter::new(vec![SortField::new(array.value_type())])?;
281281

282-
let element = sub_array.clone();
282+
let element = Arc::clone(sub_array);
283283
let sub_array = if comparison_type != ComparisonType::Single {
284284
as_generic_list_array::<O>(sub_array)?
285285
} else {
@@ -292,7 +292,7 @@ fn general_array_has_dispatch<O: OffsetSizeTrait>(
292292
let sub_arr_values = if comparison_type != ComparisonType::Single {
293293
converter.convert_columns(&[sub_arr])?
294294
} else {
295-
converter.convert_columns(&[element.clone()])?
295+
converter.convert_columns(&[Arc::clone(&element)])?
296296
};
297297

298298
let mut res = match comparison_type {

‎datafusion/functions-array/src/concat.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ pub(crate) fn array_concat_inner(args: &[ArrayRef]) -> Result<ArrayRef> {
249249
return not_impl_err!("Array is not type '{base_type:?}'.");
250250
}
251251
if !base_type.eq(&DataType::Null) {
252-
new_args.push(arg.clone());
252+
new_args.push(Arc::clone(arg));
253253
}
254254
}
255255

‎datafusion/functions-array/src/flatten.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ impl ScalarUDFImpl for Flatten {
7777
get_base_type(field.data_type())
7878
}
7979
Null | List(_) | LargeList(_) => Ok(data_type.to_owned()),
80-
FixedSizeList(field, _) => Ok(List(field.clone())),
80+
FixedSizeList(field, _) => Ok(List(Arc::clone(field))),
8181
_ => exec_err!(
8282
"Not reachable, data_type should be List, LargeList or FixedSizeList"
8383
),
@@ -115,7 +115,7 @@ pub fn flatten_inner(args: &[ArrayRef]) -> Result<ArrayRef> {
115115
let flattened_array = flatten_internal::<i64>(list_arr.clone(), None)?;
116116
Ok(Arc::new(flattened_array) as ArrayRef)
117117
}
118-
Null => Ok(args[0].clone()),
118+
Null => Ok(Arc::clone(&args[0])),
119119
_ => {
120120
exec_err!("flatten does not support type '{array_type:?}'")
121121
}

‎datafusion/functions-array/src/lib.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
// KIND, either express or implied. See the License for the
1515
// specific language governing permissions and limitations
1616
// under the License.
17+
// Make cheap clones clear: https://github.com/apache/datafusion/issues/11143
18+
#![deny(clippy::clone_on_ref_ptr)]
1719

1820
//! Array Functions for [DataFusion].
1921
//!

‎datafusion/functions-array/src/resize.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,8 @@ impl ScalarUDFImpl for ArrayResize {
6767

6868
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
6969
match &arg_types[0] {
70-
List(field) | FixedSizeList(field, _) => Ok(List(field.clone())),
71-
LargeList(field) => Ok(LargeList(field.clone())),
70+
List(field) | FixedSizeList(field, _) => Ok(List(Arc::clone(field))),
71+
LargeList(field) => Ok(LargeList(Arc::clone(field))),
7272
_ => exec_err!(
7373
"Not reachable, data_type should be List, LargeList or FixedSizeList"
7474
),
@@ -92,7 +92,7 @@ pub(crate) fn array_resize_inner(arg: &[ArrayRef]) -> Result<ArrayRef> {
9292

9393
let new_len = as_int64_array(&arg[1])?;
9494
let new_element = if arg.len() == 3 {
95-
Some(arg[2].clone())
95+
Some(Arc::clone(&arg[2]))
9696
} else {
9797
None
9898
};
@@ -168,7 +168,7 @@ fn general_list_resize<O: OffsetSizeTrait + TryInto<i64>>(
168168

169169
let data = mutable.freeze();
170170
Ok(Arc::new(GenericListArray::<O>::try_new(
171-
field.clone(),
171+
Arc::clone(field),
172172
OffsetBuffer::<O>::new(offsets.into()),
173173
arrow_array::make_array(data),
174174
None,

‎datafusion/functions-array/src/reverse.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ pub fn array_reverse_inner(arg: &[ArrayRef]) -> Result<ArrayRef> {
9393
let array = as_large_list_array(&arg[0])?;
9494
general_array_reverse::<i64>(array, field)
9595
}
96-
Null => Ok(arg[0].clone()),
96+
Null => Ok(Arc::clone(&arg[0])),
9797
array_type => exec_err!("array_reverse does not support type '{array_type:?}'."),
9898
}
9999
}
@@ -137,7 +137,7 @@ fn general_array_reverse<O: OffsetSizeTrait + TryFrom<i64>>(
137137

138138
let data = mutable.freeze();
139139
Ok(Arc::new(GenericListArray::<O>::try_new(
140-
field.clone(),
140+
Arc::clone(field),
141141
OffsetBuffer::<O>::new(offsets.into()),
142142
arrow_array::make_array(data),
143143
Some(nulls.into()),

‎datafusion/functions-array/src/set_ops.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ fn array_distinct_inner(args: &[ArrayRef]) -> Result<ArrayRef> {
213213

214214
// handle null
215215
if args[0].data_type() == &Null {
216-
return Ok(args[0].clone());
216+
return Ok(Arc::clone(&args[0]));
217217
}
218218

219219
// handle for list & largelist
@@ -314,7 +314,7 @@ fn generic_set_lists<OffsetSize: OffsetSizeTrait>(
314314
offsets.push(last_offset + OffsetSize::usize_as(rows.len()));
315315
let arrays = converter.convert_rows(rows)?;
316316
let array = match arrays.first() {
317-
Some(array) => array.clone(),
317+
Some(array) => Arc::clone(array),
318318
None => {
319319
return internal_err!("{set_op}: failed to get array from rows");
320320
}
@@ -370,12 +370,12 @@ fn general_set_op(
370370
(List(field), List(_)) => {
371371
let array1 = as_list_array(&array1)?;
372372
let array2 = as_list_array(&array2)?;
373-
generic_set_lists::<i32>(array1, array2, field.clone(), set_op)
373+
generic_set_lists::<i32>(array1, array2, Arc::clone(field), set_op)
374374
}
375375
(LargeList(field), LargeList(_)) => {
376376
let array1 = as_large_list_array(&array1)?;
377377
let array2 = as_large_list_array(&array2)?;
378-
generic_set_lists::<i64>(array1, array2, field.clone(), set_op)
378+
generic_set_lists::<i64>(array1, array2, Arc::clone(field), set_op)
379379
}
380380
(data_type1, data_type2) => {
381381
internal_err!(
@@ -426,7 +426,7 @@ fn general_array_distinct<OffsetSize: OffsetSizeTrait>(
426426
offsets.push(last_offset + OffsetSize::usize_as(rows.len()));
427427
let arrays = converter.convert_rows(rows)?;
428428
let array = match arrays.first() {
429-
Some(array) => array.clone(),
429+
Some(array) => Arc::clone(array),
430430
None => {
431431
return internal_err!("array_distinct: failed to get array from rows")
432432
}
@@ -437,7 +437,7 @@ fn general_array_distinct<OffsetSize: OffsetSizeTrait>(
437437
let new_arrays_ref = new_arrays.iter().map(|v| v.as_ref()).collect::<Vec<_>>();
438438
let values = compute::concat(&new_arrays_ref)?;
439439
Ok(Arc::new(GenericListArray::<OffsetSize>::try_new(
440-
field.clone(),
440+
Arc::clone(field),
441441
offsets,
442442
values,
443443
None,

0 commit comments

Comments
 (0)