Skip to content

Commit

Permalink
Enable clone_on_ref_ptr clippy lint on physical-expr crate (apache#…
Browse files Browse the repository at this point in the history 
…11240)

* Enable clone_on_ref_ptr clippy lint on physical-expr crate

* Fixup clippy

---------

Co-authored-by: Andrew Lamb <[email protected]>
  • Loading branch information
@lewiszlw @alamb
lewiszlw and alamb authored Jul 5, 2024
1 parent 0d2525e commit 9355f4a
Show file tree
Hide file tree
Showing 35 changed files with 397 additions and 341 deletions.
4 changes: 2 additions & 2 deletions datafusion/physical-expr/src/aggregate/array_agg.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ impl AggregateExpr for ArrayAgg {
}

fn expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
vec![self.expr.clone()]
vec![Arc::clone(&self.expr)]
}

fn name(&self) -> &str {
Expand Down Expand Up @@ -137,7 +137,7 @@ impl Accumulator for ArrayAggAccumulator {
return Ok(());
}
assert!(values.len() == 1, "array_agg can only take 1 param!");
let val = values[0].clone();
let val = Arc::clone(&values[0]);
self.values.push(val);
Ok(())
}
Expand Down
2 changes: 1 addition & 1 deletion datafusion/physical-expr/src/aggregate/array_agg_distinct.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ impl AggregateExpr for DistinctArrayAgg {
}

fn expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
vec![self.expr.clone()]
vec![Arc::clone(&self.expr)]
}

fn name(&self) -> &str {
Expand Down
4 changes: 2 additions & 2 deletions datafusion/physical-expr/src/aggregate/array_agg_ordered.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ impl AggregateExpr for OrderSensitiveArrayAgg {
}

fn expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
vec![self.expr.clone()]
vec![Arc::clone(&self.expr)]
}

fn order_bys(&self) -> Option<&[PhysicalSortExpr]> {
Expand All @@ -146,7 +146,7 @@ impl AggregateExpr for OrderSensitiveArrayAgg {
Some(Arc::new(Self {
name: self.name.to_string(),
input_data_type: self.input_data_type.clone(),
expr: self.expr.clone(),
expr: Arc::clone(&self.expr),
nullable: self.nullable,
order_by_data_types: self.order_by_data_types.clone(),
// Reverse requirement:
Expand Down
12 changes: 6 additions & 6 deletions datafusion/physical-expr/src/aggregate/build_in.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ pub fn create_aggregate_expr(
let input_phy_exprs = input_phy_exprs.to_vec();
Ok(match (fun, distinct) {
(AggregateFunction::ArrayAgg, false) => {
let expr = input_phy_exprs[0].clone();
let expr = Arc::clone(&input_phy_exprs[0]);
let nullable = expr.nullable(input_schema)?;

if ordering_req.is_empty() {
Expand All @@ -83,7 +83,7 @@ pub fn create_aggregate_expr(
"ARRAY_AGG(DISTINCT ORDER BY a ASC) order-sensitive aggregations are not available"
);
}
let expr = input_phy_exprs[0].clone();
let expr = Arc::clone(&input_phy_exprs[0]);
let is_expr_nullable = expr.nullable(input_schema)?;
Arc::new(expressions::DistinctArrayAgg::new(
expr,
Expand All @@ -93,12 +93,12 @@ pub fn create_aggregate_expr(
))
}
(AggregateFunction::Min, _) => Arc::new(expressions::Min::new(
input_phy_exprs[0].clone(),
Arc::clone(&input_phy_exprs[0]),
name,
data_type,
)),
(AggregateFunction::Max, _) => Arc::new(expressions::Max::new(
input_phy_exprs[0].clone(),
Arc::clone(&input_phy_exprs[0]),
name,
data_type,
)),
Expand All @@ -113,7 +113,7 @@ pub fn create_aggregate_expr(
};
let nullable = expr.nullable(input_schema)?;
Arc::new(expressions::NthValueAgg::new(
expr.clone(),
Arc::clone(expr),
n.clone().try_into()?,
name,
input_phy_types[0].clone(),
Expand Down Expand Up @@ -320,7 +320,7 @@ mod tests {
input_exprs
.iter()
.zip(coerced_types)
.map(|(expr, coerced_type)| try_cast(expr.clone(), schema, coerced_type))
.map(|(expr, coerced_type)| try_cast(Arc::clone(expr), schema, coerced_type))
.collect::<Result<Vec<_>>>()
}
}
16 changes: 8 additions & 8 deletions datafusion/physical-expr/src/aggregate/min_max.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ impl AggregateExpr for Max {
}

fn expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
vec![self.expr.clone()]
vec![Arc::clone(&self.expr)]
}

fn create_accumulator(&self) -> Result<Box<dyn Accumulator>> {
Expand Down Expand Up @@ -927,7 +927,7 @@ impl AggregateExpr for Min {
}

fn expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
vec![self.expr.clone()]
vec![Arc::clone(&self.expr)]
}

fn name(&self) -> &str {
Expand Down Expand Up @@ -1169,7 +1169,7 @@ mod tests {
let mut min =
MinAccumulator::try_new(&DataType::Interval(IntervalUnit::YearMonth))
.unwrap();
min.update_batch(&[b.clone()]).unwrap();
min.update_batch(&[Arc::clone(&b)]).unwrap();
let min_res = min.evaluate().unwrap();
assert_eq!(
min_res,
Expand All @@ -1181,7 +1181,7 @@ mod tests {
let mut max =
MaxAccumulator::try_new(&DataType::Interval(IntervalUnit::YearMonth))
.unwrap();
max.update_batch(&[b.clone()]).unwrap();
max.update_batch(&[Arc::clone(&b)]).unwrap();
let max_res = max.evaluate().unwrap();
assert_eq!(
max_res,
Expand All @@ -1202,7 +1202,7 @@ mod tests {

let mut min =
MinAccumulator::try_new(&DataType::Interval(IntervalUnit::DayTime)).unwrap();
min.update_batch(&[b.clone()]).unwrap();
min.update_batch(&[Arc::clone(&b)]).unwrap();
let min_res = min.evaluate().unwrap();
assert_eq!(
min_res,
Expand All @@ -1211,7 +1211,7 @@ mod tests {

let mut max =
MaxAccumulator::try_new(&DataType::Interval(IntervalUnit::DayTime)).unwrap();
max.update_batch(&[b.clone()]).unwrap();
max.update_batch(&[Arc::clone(&b)]).unwrap();
let max_res = max.evaluate().unwrap();
assert_eq!(
max_res,
Expand All @@ -1231,7 +1231,7 @@ mod tests {
let mut min =
MinAccumulator::try_new(&DataType::Interval(IntervalUnit::MonthDayNano))
.unwrap();
min.update_batch(&[b.clone()]).unwrap();
min.update_batch(&[Arc::clone(&b)]).unwrap();
let min_res = min.evaluate().unwrap();
assert_eq!(
min_res,
Expand All @@ -1243,7 +1243,7 @@ mod tests {
let mut max =
MaxAccumulator::try_new(&DataType::Interval(IntervalUnit::MonthDayNano))
.unwrap();
max.update_batch(&[b.clone()]).unwrap();
max.update_batch(&[Arc::clone(&b)]).unwrap();
let max_res = max.evaluate().unwrap();
assert_eq!(
max_res,
Expand Down
4 changes: 2 additions & 2 deletions datafusion/physical-expr/src/aggregate/nth_value.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ impl AggregateExpr for NthValueAgg {

fn expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
let n = Arc::new(Literal::new(ScalarValue::Int64(Some(self.n)))) as _;
vec![self.expr.clone(), n]
vec![Arc::clone(&self.expr), n]
}

fn order_bys(&self) -> Option<&[PhysicalSortExpr]> {
Expand All @@ -138,7 +138,7 @@ impl AggregateExpr for NthValueAgg {
Some(Arc::new(Self {
name: self.name.to_string(),
input_data_type: self.input_data_type.clone(),
expr: self.expr.clone(),
expr: Arc::clone(&self.expr),
// index should be from the opposite side
n: -self.n,
nullable: self.nullable,
Expand Down
2 changes: 1 addition & 1 deletion datafusion/physical-expr/src/analysis.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ pub fn analyze(
) -> Result<AnalysisContext> {
let target_boundaries = context.boundaries;

let mut graph = ExprIntervalGraph::try_new(expr.clone(), schema)?;
let mut graph = ExprIntervalGraph::try_new(Arc::clone(expr), schema)?;

let columns = collect_columns(expr)
.into_iter()
Expand Down
35 changes: 19 additions & 16 deletions datafusion/physical-expr/src/equivalence/class.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,17 +267,19 @@ impl EquivalenceGroup {
}
(Some(group_idx), None) => {
// Right side is new, extend left side's class:
self.classes[group_idx].push(right.clone());
self.classes[group_idx].push(Arc::clone(right));
}
(None, Some(group_idx)) => {
// Left side is new, extend right side's class:
self.classes[group_idx].push(left.clone());
self.classes[group_idx].push(Arc::clone(left));
}
(None, None) => {
// None of the expressions is among existing classes.
// Create a new equivalence class and extend the group.
self.classes
.push(EquivalenceClass::new(vec![left.clone(), right.clone()]));
self.classes.push(EquivalenceClass::new(vec![
Arc::clone(left),
Arc::clone(right),
]));
}
}
}
Expand Down Expand Up @@ -328,7 +330,7 @@ impl EquivalenceGroup {
/// The expression is replaced with the first expression in the equivalence
/// class it matches with (if any).
pub fn normalize_expr(&self, expr: Arc<dyn PhysicalExpr>) -> Arc<dyn PhysicalExpr> {
expr.clone()
Arc::clone(&expr)
.transform(|expr| {
for cls in self.iter() {
if cls.contains(&expr) {
Expand Down Expand Up @@ -429,7 +431,7 @@ impl EquivalenceGroup {
.get_equivalence_class(source)
.map_or(false, |group| group.contains(expr))
{
return Some(target.clone());
return Some(Arc::clone(target));
}
}
}
Expand All @@ -443,7 +445,7 @@ impl EquivalenceGroup {
.into_iter()
.map(|child| self.project_expr(mapping, child))
.collect::<Option<Vec<_>>>()
.map(|children| expr.clone().with_new_children(children).unwrap())
.map(|children| Arc::clone(expr).with_new_children(children).unwrap())
}

/// Projects this equivalence group according to the given projection mapping.
Expand All @@ -461,13 +463,13 @@ impl EquivalenceGroup {
let mut new_classes = vec![];
for (source, target) in mapping.iter() {
if new_classes.is_empty() {
new_classes.push((source, vec![target.clone()]));
new_classes.push((source, vec![Arc::clone(target)]));
}
if let Some((_, values)) =
new_classes.iter_mut().find(|(key, _)| key.eq(source))
{
if !physical_exprs_contains(values, target) {
values.push(target.clone());
values.push(Arc::clone(target));
}
}
}
Expand Down Expand Up @@ -515,10 +517,9 @@ impl EquivalenceGroup {
// are equal in the resulting table.
if join_type == &JoinType::Inner {
for (lhs, rhs) in on.iter() {
let new_lhs = lhs.clone() as _;
let new_lhs = Arc::clone(lhs) as _;
// Rewrite rhs to point to the right side of the join:
let new_rhs = rhs
.clone()
let new_rhs = Arc::clone(rhs)
.transform(|expr| {
if let Some(column) =
expr.as_any().downcast_ref::<Column>()
Expand Down Expand Up @@ -649,7 +650,7 @@ mod tests {
let eq_group = eq_properties.eq_group();
for (expr, expected_eq) in expressions {
assert!(
expected_eq.eq(&eq_group.normalize_expr(expr.clone())),
expected_eq.eq(&eq_group.normalize_expr(Arc::clone(expr))),
"error in test: expr: {expr:?}"
);
}
Expand All @@ -669,9 +670,11 @@ mod tests {
Arc::new(Literal::new(ScalarValue::Int32(Some(1)))) as Arc<dyn PhysicalExpr>;
let col_b_expr = Arc::new(Column::new("b", 1)) as Arc<dyn PhysicalExpr>;

let cls1 = EquivalenceClass::new(vec![lit_true.clone(), lit_false.clone()]);
let cls2 = EquivalenceClass::new(vec![lit_true.clone(), col_b_expr.clone()]);
let cls3 = EquivalenceClass::new(vec![lit2.clone(), lit1.clone()]);
let cls1 =
EquivalenceClass::new(vec![Arc::clone(&lit_true), Arc::clone(&lit_false)]);
let cls2 =
EquivalenceClass::new(vec![Arc::clone(&lit_true), Arc::clone(&col_b_expr)]);
let cls3 = EquivalenceClass::new(vec![Arc::clone(&lit2), Arc::clone(&lit1)]);

// lit_true is common
assert!(cls1.contains_any(&cls2));
Expand Down
Loading

0 comments on commit 9355f4a

Please sign in to comment.