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.

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

Remove redundant clones. #506

Merged
merged 1 commit into from
Jun 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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