Skip to content

Commit 98258ae

Browse files
lewiszlwwiedld
lewiszlw
authored and
wiedld
committed
Enable clone_on_ref_ptr clippy lint on functions* (apache#11468)
* Enable clone_on_ref_ptr clippy lint on functions * Remove unnecessary Arc::clone
1 parent e6125c3 commit 98258ae

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,

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ pub fn array_sort_inner(args: &[ArrayRef]) -> Result<ArrayRef> {
121121
let list_array = as_list_array(&args[0])?;
122122
let row_count = list_array.len();
123123
if row_count == 0 {
124-
return Ok(args[0].clone());
124+
return Ok(Arc::clone(&args[0]));
125125
}
126126

127127
let mut array_lengths = vec![];

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,7 @@ pub(super) fn array_to_string_inner(args: &[ArrayRef]) -> Result<ArrayRef> {
381381
let delimiter = delimiters[0].unwrap();
382382
let s = compute_array_to_string(
383383
&mut arg,
384-
arr.clone(),
384+
Arc::clone(arr),
385385
delimiter.to_string(),
386386
null_string,
387387
with_null_string,

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

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ pub(crate) fn align_array_dimensions<O: OffsetSizeTrait>(
105105
.zip(args_ndim.iter())
106106
.map(|(array, ndim)| {
107107
if ndim < max_ndim {
108-
let mut aligned_array = array.clone();
108+
let mut aligned_array = Arc::clone(&array);
109109
for _ in 0..(max_ndim - ndim) {
110110
let data_type = aligned_array.data_type().to_owned();
111111
let array_lengths = vec![1; aligned_array.len()];
@@ -120,7 +120,7 @@ pub(crate) fn align_array_dimensions<O: OffsetSizeTrait>(
120120
}
121121
Ok(aligned_array)
122122
} else {
123-
Ok(array.clone())
123+
Ok(Arc::clone(&array))
124124
}
125125
})
126126
.collect();
@@ -277,10 +277,12 @@ mod tests {
277277
Some(vec![Some(6), Some(7), Some(8)]),
278278
]));
279279

280-
let array2d_1 =
281-
Arc::new(array_into_list_array_nullable(array1d_1.clone())) as ArrayRef;
282-
let array2d_2 =
283-
Arc::new(array_into_list_array_nullable(array1d_2.clone())) as ArrayRef;
280+
let array2d_1 = Arc::new(array_into_list_array_nullable(
281+
Arc::clone(&array1d_1) as ArrayRef
282+
)) as ArrayRef;
283+
let array2d_2 = Arc::new(array_into_list_array_nullable(
284+
Arc::clone(&array1d_2) as ArrayRef
285+
)) as ArrayRef;
284286

285287
let res = align_array_dimensions::<i32>(vec![
286288
array1d_1.to_owned(),

‎datafusion/functions/benches/concat.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
// specific language governing permissions and limitations
1616
// under the License.
1717

18+
use arrow::array::ArrayRef;
1819
use arrow::util::bench_util::create_string_array_with_len;
1920
use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion};
2021
use datafusion_common::ScalarValue;
@@ -26,7 +27,7 @@ fn create_args(size: usize, str_len: usize) -> Vec<ColumnarValue> {
2627
let array = Arc::new(create_string_array_with_len::<i32>(size, 0.2, str_len));
2728
let scalar = ScalarValue::Utf8(Some(", ".to_string()));
2829
vec![
29-
ColumnarValue::Array(array.clone()),
30+
ColumnarValue::Array(Arc::clone(&array) as ArrayRef),
3031
ColumnarValue::Scalar(scalar),
3132
ColumnarValue::Array(array),
3233
]

‎datafusion/functions/benches/regx.rs

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,12 @@ fn criterion_benchmark(c: &mut Criterion) {
8383

8484
b.iter(|| {
8585
black_box(
86-
regexp_like::<i32>(&[data.clone(), regex.clone(), flags.clone()])
87-
.expect("regexp_like should work on valid values"),
86+
regexp_like::<i32>(&[
87+
Arc::clone(&data),
88+
Arc::clone(&regex),
89+
Arc::clone(&flags),
90+
])
91+
.expect("regexp_like should work on valid values"),
8892
)
8993
})
9094
});
@@ -97,8 +101,12 @@ fn criterion_benchmark(c: &mut Criterion) {
97101

98102
b.iter(|| {
99103
black_box(
100-
regexp_match::<i32>(&[data.clone(), regex.clone(), flags.clone()])
101-
.expect("regexp_match should work on valid values"),
104+
regexp_match::<i32>(&[
105+
Arc::clone(&data),
106+
Arc::clone(&regex),
107+
Arc::clone(&flags),
108+
])
109+
.expect("regexp_match should work on valid values"),
102110
)
103111
})
104112
});
@@ -115,10 +123,10 @@ fn criterion_benchmark(c: &mut Criterion) {
115123
b.iter(|| {
116124
black_box(
117125
regexp_replace::<i32>(&[
118-
data.clone(),
119-
regex.clone(),
120-
replacement.clone(),
121-
flags.clone(),
126+
Arc::clone(&data),
127+
Arc::clone(&regex),
128+
Arc::clone(&replacement),
129+
Arc::clone(&flags),
122130
])
123131
.expect("regexp_replace should work on valid values"),
124132
)

0 commit comments

Comments
 (0)