Skip to content

Commit

Permalink
Remove redundant clones.
Browse files Browse the repository at this point in the history
This turns on the Clippy warning `redundant_clone`, which detects
unnecessary calls to `.clone()` (and `.to_string()`).

It is an unstable warning and so might reports some false positives. If
we find any, we can suppress the warning there.
  • Loading branch information
SamirTalwar committed Jun 24, 2024
1 parent 097a4e9 commit 2fcb385
Show file tree
Hide file tree
Showing 16 changed files with 43 additions and 55 deletions.
2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ missing_panics_doc = "allow"
module_name_repetitions = "allow"
must_use_candidate = "allow"
wildcard_imports = "allow"
# unstable warnings; we might need to suppress them
redundant_clone = "warn"
# disable these for now, but we should probably fix them
similar_names = "allow"
too_many_lines = "allow"
Expand Down
4 changes: 1 addition & 3 deletions crates/configuration/src/version3/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -674,9 +674,7 @@ fn convert_scalar_types(

type_representation: representations
.0
.get(&query_engine_metadata::metadata::ScalarTypeName(
t.0.clone(),
))
.get(&query_engine_metadata::metadata::ScalarTypeName(t.0))
.cloned(),
},
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ fn convert_scalar_types(
.into_iter()
.map(|(scalar_type_name, scalar_type)| {
(
convert_scalar_type_name(scalar_type_name.clone()),
convert_scalar_type_name(scalar_type_name),
query_engine_metadata::metadata::ScalarType {
type_name: scalar_type.type_name,
schema_name: Some(scalar_type.schema_name),
Expand Down
2 changes: 1 addition & 1 deletion crates/connectors/ndc-postgres/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -787,7 +787,7 @@ fn make_update_column_type(
);

(
object_type_name.clone(),
object_type_name,
models::ObjectType {
description: Some(format!(
"Update the '{column_name}' column in the '{collection_name}' collection"
Expand Down
12 changes: 6 additions & 6 deletions crates/query-engine/sql/src/sql/rewrites/constant_folding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -367,31 +367,31 @@ mod tests {
fn true_and_true_is_true() {
let left_side = expr_true();
let right_side = expr_true();
let expr = expr_and(left_side, right_side.clone());
let expr = expr_and(left_side, right_side);
assert_eq!(normalize_expr(expr), expr_true());
}

#[test]
fn false_or_false_is_false() {
let left_side = expr_false();
let right_side = expr_false();
let expr = expr_or(left_side, right_side.clone());
let expr = expr_or(left_side, right_side);
assert_eq!(normalize_expr(expr), expr_false());
}

#[test]
fn true_and_false_is_false() {
let left_side = expr_true();
let right_side = expr_false();
let expr = expr_and(left_side, right_side.clone());
let expr = expr_and(left_side, right_side);
assert_eq!(normalize_expr(expr), expr_false());
}

#[test]
fn false_or_true_is_true() {
let left_side = expr_false();
let right_side = expr_true();
let expr = expr_or(left_side, right_side.clone());
let expr = expr_or(left_side, right_side);
assert_eq!(normalize_expr(expr), expr_true());
}

Expand Down Expand Up @@ -424,8 +424,8 @@ mod tests {
fn eq_expr_is_not_removed() {
let eq_expr = expr_eq(expr_seven(), expr_seven());
let left_side = expr_seven();
let right_side = expr_and(eq_expr.clone(), eq_expr.clone());
let expr = expr_and(left_side, right_side.clone());
let right_side = expr_and(eq_expr.clone(), eq_expr);
let expr = expr_and(left_side, right_side);
assert_eq!(normalize_expr(expr.clone()), expr);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ pub fn translate(
state,
&helpers::RootAndCurrentTables {
root_table: table_name_and_reference.clone(),
current_table: table_name_and_reference.clone(),
current_table: table_name_and_reference,
},
&predicate,
)?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ pub fn translate(
state,
&helpers::RootAndCurrentTables {
root_table: table_name_and_reference.clone(),
current_table: table_name_and_reference.clone(),
current_table: table_name_and_reference,
},
&predicate,
)?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ pub fn translate(

let super::update::UpdateMutation::UpdateByKey(update_by_key) = update;

let return_collection = update_by_key.collection_name.clone();
let return_collection = update_by_key.collection_name;

(
return_collection,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,11 @@ pub fn translate(
})
.collect::<Result<Vec<sql::ast::Expression>, Error>>()?;

let root_and_current_tables = helpers::RootAndCurrentTables {
root_table: table_name_and_reference.clone(),
current_table: table_name_and_reference,
};

// Build the `pre_constraint` argument boolean expression.
let pre_predicate_json =
arguments
Expand All @@ -167,10 +172,7 @@ pub fn translate(
let pre_predicate_expression = filtering::translate_expression(
env,
state,
&helpers::RootAndCurrentTables {
root_table: table_name_and_reference.clone(),
current_table: table_name_and_reference.clone(),
},
&root_and_current_tables,
&pre_predicate,
)?;

Expand All @@ -187,10 +189,7 @@ pub fn translate(
let post_predicate_expression = filtering::translate_expression(
env,
state,
&helpers::RootAndCurrentTables {
root_table: table_name_and_reference.clone(),
current_table: table_name_and_reference.clone(),
},
&root_and_current_tables,
&post_predicate,
)?;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ fn translate_mutation(
// create a from clause for the query selecting from the CTE.
query.from = Some(sql::ast::From::Table {
reference: sql::ast::TableReference::AliasedTable(cte_table_alias.clone()),
alias: select_from_cte_table_alias_2.clone(),
alias: select_from_cte_table_alias_2,
});
query
};
Expand Down Expand Up @@ -189,14 +189,7 @@ fn translate_native_query(
// this is what our query processing expects
let arguments = arguments
.into_iter()
.map(|(key, value)| {
(
key.clone(),
models::Argument::Literal {
value: value.clone(),
},
)
})
.map(|(key, value)| (key, models::Argument::Literal { value }))
.collect();

// insert the procedure as a native query and get a reference to it.
Expand All @@ -222,7 +215,7 @@ fn translate_native_query(
&mut state,
&crate::translation::query::root::MakeFrom::TableReference {
name: procedure_name.clone(),
reference: table_reference.clone(),
reference: table_reference,
},
&None,
&query,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,10 +325,8 @@ fn translate_comparison_pathelements(
let relationship = env.lookup_relationship(relationship_name)?;

// new alias for the target table
let target_table_alias: sql::ast::TableAlias = state
.make_boolean_expression_table_alias(
&relationship.target_collection.clone().to_string(),
);
let target_table_alias: sql::ast::TableAlias =
state.make_boolean_expression_table_alias(&relationship.target_collection);

let arguments = relationships::make_relationship_arguments(
relationships::MakeRelationshipArguments {
Expand Down Expand Up @@ -538,7 +536,7 @@ pub fn translate_exists_in_collection(
let column_alias = sql::helpers::make_column_alias("one".to_string());

let select_cols = vec![(
column_alias.clone(),
column_alias,
sql::ast::Expression::Value(sql::ast::Value::Int4(1)),
)];

Expand All @@ -549,8 +547,8 @@ pub fn translate_exists_in_collection(
let new_root_and_current_tables = RootAndCurrentTables {
root_table: root_and_current_tables.root_table.clone(),
current_table: TableNameAndReference {
reference: table.reference.clone(),
name: table.name.clone(),
reference: table.reference,
name: table.name,
},
};

Expand Down Expand Up @@ -599,7 +597,7 @@ pub fn translate_exists_in_collection(
let column_alias = sql::helpers::make_column_alias("one".to_string());

let select_cols = vec![(
column_alias.clone(),
column_alias,
sql::ast::Expression::Value(sql::ast::Value::Int4(1)),
)];

Expand All @@ -611,7 +609,7 @@ pub fn translate_exists_in_collection(
root_table: root_and_current_tables.root_table.clone(),
current_table: TableNameAndReference {
reference: table.reference.clone(),
name: table.name.clone(),
name: table.name,
},
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ pub fn wrap_cte_in_cte(
let nested_cte_select = sql::ast::CTExpr::Select({
let mut select = sql::helpers::star_select(sql::ast::From::Table {
reference: sql::ast::TableReference::AliasedTable(cte.alias.clone()),
alias: nested_cte_alias.clone(),
alias: nested_cte_alias,
});
select.with = sql::ast::With {
common_table_expressions: vec![cte],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub fn translate_joins(
state,
&root::MakeFrom::Collection {
name: relationship.target_collection.clone(),
arguments: arguments.clone(),
arguments,
},
// We ask to inject the join predicate into the where clause.
&Some(root::JoinPredicate {
Expand Down
5 changes: 2 additions & 3 deletions crates/query-engine/translation/src/translation/query/root.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,15 +233,14 @@ pub fn make_from_clause_and_reference(
None => state.make_table_alias(collection_name.to_string()),
Some(alias) => alias,
};
let collection_alias_name = sql::ast::TableReference::AliasedTable(collection_alias.clone());

// find the table according to the metadata.
let collection_info = env.lookup_collection(collection_name)?;
let from_clause = make_from_clause(state, &collection_alias, &collection_info, arguments);

let collection_alias_name = sql::ast::TableReference::AliasedTable(collection_alias);
let current_table = TableNameAndReference {
name: collection_name.to_string(),
reference: collection_alias_name.clone(),
reference: collection_alias_name,
};
Ok((current_table, from_clause))
}
Expand Down
13 changes: 6 additions & 7 deletions crates/query-engine/translation/src/translation/query/sorting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ fn translate_order_by_target_group(
sql::ast::OrderByElement {
target: wrap_in_field_path(
&field_path,
sql::ast::Expression::ColumnReference(column_name.clone()),
sql::ast::Expression::ColumnReference(column_name),
),
direction: match direction {
models::OrderDirection::Asc => sql::ast::OrderByDirection::Asc,
Expand Down Expand Up @@ -296,7 +296,7 @@ fn translate_order_by_target_group(
table: sql::ast::TableReference::AliasedTable(
table_alias.clone(),
),
column: column.clone(),
column,
},
),
),
Expand Down Expand Up @@ -578,8 +578,7 @@ fn process_path_element_for_order_by_targets(
let selected_column = collection.lookup_column(source_col)?;
// we are going to deliberately use the table column name and not an alias we get from
// the query request because this is internal to the sorting mechanism.
let selected_column_alias =
sql::helpers::make_column_alias(selected_column.name.0.clone());
let selected_column_alias = sql::helpers::make_column_alias(selected_column.name.0);
// we use the real name of the column as an alias as well.
Ok(OrderByRelationshipColumn {
alias: selected_column_alias.clone(),
Expand Down Expand Up @@ -658,7 +657,7 @@ fn translate_targets(
// we are going to deliberately use the table column name and not an alias we get from
// the query request because this is internal to the sorting mechanism.
let selected_column_alias =
sql::helpers::make_column_alias(selected_column.name.0.clone());
sql::helpers::make_column_alias(selected_column.name.0);

// we use the real name of the column as an alias as well.
Ok::<OrderBySelectExpression, Error>(OrderBySelectExpression {
Expand Down Expand Up @@ -689,7 +688,7 @@ fn translate_targets(
Ok(OrderBySelectExpression {
index: element.index,
direction: element.direction,
alias: column_alias.clone(),
alias: column_alias,
// Aggregates do not have a field path.
field_path: (&None).into(),
expression: sql::ast::Expression::Value(sql::ast::Value::Int4(1)),
Expand All @@ -701,7 +700,7 @@ fn translate_targets(
// we are going to deliberately use the table column name and not an alias we get from
// the query request because this is internal to the sorting mechanism.
let selected_column_alias =
sql::helpers::make_column_alias(selected_column.name.0.clone());
sql::helpers::make_column_alias(selected_column.name.0);
// we use the real name of the column as an alias as well.
Ok(OrderBySelectExpression {
index: element.index,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,15 +175,15 @@ pub fn translate_projected_variable(

let element_expression =
sql::ast::Expression::ColumnReference(ColumnReference::AliasedColumn {
table: sql::ast::TableReference::AliasedTable(array_table.clone()),
table: sql::ast::TableReference::AliasedTable(array_table),
column: element_column.clone(),
});

let converted_element_exp =
translate_projected_variable(env, state, type_name, element_expression)?;

let mut result_select = simple_select(vec![(
element_column.clone(),
element_column,
sql::ast::Expression::FunctionCall {
function: sql::ast::Function::Unknown("array_agg".to_string()),
args: vec![converted_element_exp],
Expand Down

0 comments on commit 2fcb385

Please sign in to comment.