Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable clone_on_ref_ptr clippy lint on physical-expr crate #11240

Merged
merged 4 commits into from
Jul 5, 2024
Merged

Enable clone_on_ref_ptr clippy lint on physical-expr crate #11240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
25a1dc7
Enable clone_on_ref_ptr clippy lint on physical-expr crate
lewiszlw Jul 3, 2024
2d578ab
Merge main
lewiszlw Jul 3, 2024
fcffb65
Merge remote-tracking branch 'apache/main' into enable-clone_on_ref_p…
alamb Jul 5, 2024
2876cd1
Fixup clippy
alamb Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions datafusion/physical-expr/src/aggregate/array_agg.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading