From 09cf76c1421078d80920b61ddb9e2022dafe3d6c Mon Sep 17 00:00:00 2001 From: Samir Talwar Date: Mon, 24 Jun 2024 10:12:49 +0200 Subject: [PATCH] Remove redundant clones. (#506) ### What I noticed a few extra calls to `.clone()` while working on an unrelated refactor. I want to remove them for brevity and simplicity; I don't expect a performance improvement. 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. ### How I turned the warning on and applied fixes everywhere. I also saw a few that the lint didn't catch, which I fixed myself. --- Cargo.toml | 2 ++ crates/configuration/src/version3/mod.rs | 4 +--- .../src/version4/to_runtime_configuration.rs | 2 +- crates/connectors/ndc-postgres/src/schema.rs | 2 +- .../sql/src/sql/rewrites/constant_folding.rs | 12 ++++++------ .../translation/mutation/experimental/delete.rs | 2 +- .../translation/mutation/experimental/insert.rs | 2 +- .../mutation/experimental/translate.rs | 2 +- .../translation/mutation/experimental/update.rs | 15 +++++++-------- .../src/translation/mutation/translate.rs | 13 +++---------- .../src/translation/query/filtering.rs | 16 +++++++--------- .../src/translation/query/native_queries.rs | 2 +- .../src/translation/query/relationships.rs | 2 +- .../translation/src/translation/query/root.rs | 5 ++--- .../translation/src/translation/query/sorting.rs | 13 ++++++------- .../translation/src/translation/query/values.rs | 4 ++-- 16 files changed, 43 insertions(+), 55 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4bb6ea8a7..595d4f0e0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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" diff --git a/crates/configuration/src/version3/mod.rs b/crates/configuration/src/version3/mod.rs index f98238d2a..fd8352c18 100644 --- a/crates/configuration/src/version3/mod.rs +++ b/crates/configuration/src/version3/mod.rs @@ -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(), }, ) diff --git a/crates/configuration/src/version4/to_runtime_configuration.rs b/crates/configuration/src/version4/to_runtime_configuration.rs index dbe7283db..ec0fe313a 100644 --- a/crates/configuration/src/version4/to_runtime_configuration.rs +++ b/crates/configuration/src/version4/to_runtime_configuration.rs @@ -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), diff --git a/crates/connectors/ndc-postgres/src/schema.rs b/crates/connectors/ndc-postgres/src/schema.rs index 5766a76c9..f720a4ea9 100644 --- a/crates/connectors/ndc-postgres/src/schema.rs +++ b/crates/connectors/ndc-postgres/src/schema.rs @@ -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" diff --git a/crates/query-engine/sql/src/sql/rewrites/constant_folding.rs b/crates/query-engine/sql/src/sql/rewrites/constant_folding.rs index 3da039342..d0a94f4da 100644 --- a/crates/query-engine/sql/src/sql/rewrites/constant_folding.rs +++ b/crates/query-engine/sql/src/sql/rewrites/constant_folding.rs @@ -367,7 +367,7 @@ 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()); } @@ -375,7 +375,7 @@ mod tests { 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()); } @@ -383,7 +383,7 @@ mod tests { 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()); } @@ -391,7 +391,7 @@ mod tests { 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()); } @@ -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); } diff --git a/crates/query-engine/translation/src/translation/mutation/experimental/delete.rs b/crates/query-engine/translation/src/translation/mutation/experimental/delete.rs index 0db2a5e31..795d919d0 100644 --- a/crates/query-engine/translation/src/translation/mutation/experimental/delete.rs +++ b/crates/query-engine/translation/src/translation/mutation/experimental/delete.rs @@ -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, )?; diff --git a/crates/query-engine/translation/src/translation/mutation/experimental/insert.rs b/crates/query-engine/translation/src/translation/mutation/experimental/insert.rs index c0bb63569..51f6d4cd5 100644 --- a/crates/query-engine/translation/src/translation/mutation/experimental/insert.rs +++ b/crates/query-engine/translation/src/translation/mutation/experimental/insert.rs @@ -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, )?; diff --git a/crates/query-engine/translation/src/translation/mutation/experimental/translate.rs b/crates/query-engine/translation/src/translation/mutation/experimental/translate.rs index f4404513b..d67ebcc93 100644 --- a/crates/query-engine/translation/src/translation/mutation/experimental/translate.rs +++ b/crates/query-engine/translation/src/translation/mutation/experimental/translate.rs @@ -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, diff --git a/crates/query-engine/translation/src/translation/mutation/experimental/update.rs b/crates/query-engine/translation/src/translation/mutation/experimental/update.rs index d97cc3490..5e74ede12 100644 --- a/crates/query-engine/translation/src/translation/mutation/experimental/update.rs +++ b/crates/query-engine/translation/src/translation/mutation/experimental/update.rs @@ -151,6 +151,11 @@ pub fn translate( }) .collect::, 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 @@ -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, )?; @@ -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, )?; diff --git a/crates/query-engine/translation/src/translation/mutation/translate.rs b/crates/query-engine/translation/src/translation/mutation/translate.rs index 12036ddde..ca73ce316 100644 --- a/crates/query-engine/translation/src/translation/mutation/translate.rs +++ b/crates/query-engine/translation/src/translation/mutation/translate.rs @@ -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 }; @@ -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. @@ -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, diff --git a/crates/query-engine/translation/src/translation/query/filtering.rs b/crates/query-engine/translation/src/translation/query/filtering.rs index c102a0653..645b3302b 100644 --- a/crates/query-engine/translation/src/translation/query/filtering.rs +++ b/crates/query-engine/translation/src/translation/query/filtering.rs @@ -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 { @@ -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)), )]; @@ -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, }, }; @@ -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)), )]; @@ -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, }, }; diff --git a/crates/query-engine/translation/src/translation/query/native_queries.rs b/crates/query-engine/translation/src/translation/query/native_queries.rs index 867c4cfb7..7cb223d09 100644 --- a/crates/query-engine/translation/src/translation/query/native_queries.rs +++ b/crates/query-engine/translation/src/translation/query/native_queries.rs @@ -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], diff --git a/crates/query-engine/translation/src/translation/query/relationships.rs b/crates/query-engine/translation/src/translation/query/relationships.rs index 350e4d833..5b816fdc0 100644 --- a/crates/query-engine/translation/src/translation/query/relationships.rs +++ b/crates/query-engine/translation/src/translation/query/relationships.rs @@ -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 { diff --git a/crates/query-engine/translation/src/translation/query/root.rs b/crates/query-engine/translation/src/translation/query/root.rs index 5aa2b67f6..d61ca17d9 100644 --- a/crates/query-engine/translation/src/translation/query/root.rs +++ b/crates/query-engine/translation/src/translation/query/root.rs @@ -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)) } diff --git a/crates/query-engine/translation/src/translation/query/sorting.rs b/crates/query-engine/translation/src/translation/query/sorting.rs index 5ff44f8bd..3716b25b5 100644 --- a/crates/query-engine/translation/src/translation/query/sorting.rs +++ b/crates/query-engine/translation/src/translation/query/sorting.rs @@ -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, @@ -296,7 +296,7 @@ fn translate_order_by_target_group( table: sql::ast::TableReference::AliasedTable( table_alias.clone(), ), - column: column.clone(), + column, }, ), ), @@ -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(), @@ -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 { @@ -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)), @@ -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, diff --git a/crates/query-engine/translation/src/translation/query/values.rs b/crates/query-engine/translation/src/translation/query/values.rs index 5af111cf0..f63e8893a 100644 --- a/crates/query-engine/translation/src/translation/query/values.rs +++ b/crates/query-engine/translation/src/translation/query/values.rs @@ -175,7 +175,7 @@ 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(), }); @@ -183,7 +183,7 @@ pub fn translate_projected_variable( 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],