From 4c5b1be83462f8804f64f00bc588775e8704383f Mon Sep 17 00:00:00 2001
From: Denis Smirnov <sd@picodata.io>
Date: Thu, 18 Aug 2022 13:53:25 +0700
Subject: [PATCH] perf: remove redundant clones
---
.../engine/cartridge/backend/sql/tree.rs | 16 ++++++++++------
src/executor/shard.rs | 6 +++---
src/executor/vtable.rs | 4 ++--
src/frontend/sql.rs | 8 ++++----
src/frontend/sql/ir.rs | 4 ++--
src/ir/expression.rs | 12 ++++++------
src/ir/operator.rs | 9 +--------
src/ir/transformation/redistribution.rs | 10 +++++-----
8 files changed, 33 insertions(+), 36 deletions(-)
diff --git a/src/executor/engine/cartridge/backend/sql/tree.rs b/src/executor/engine/cartridge/backend/sql/tree.rs
index c06808c8ec..142f3ac5ff 100644
--- a/src/executor/engine/cartridge/backend/sql/tree.rs
+++ b/src/executor/engine/cartridge/backend/sql/tree.rs
@@ -52,9 +52,9 @@ pub struct SyntaxNode {
}
impl SyntaxNode {
- fn new_alias(name: &str) -> Self {
+ fn new_alias(name: String) -> Self {
SyntaxNode {
- data: SyntaxData::Alias(name.into()),
+ data: SyntaxData::Alias(name),
left: None,
right: Vec::new(),
}
@@ -168,7 +168,7 @@ impl SyntaxNodes {
self.push_syntax_node(SyntaxNode::new_close()),
];
if let Some(name) = alias {
- children.push(self.push_syntax_node(SyntaxNode::new_alias(name)));
+ children.push(self.push_syntax_node(SyntaxNode::new_alias(name.clone())));
}
let sn = SyntaxNode::new_pointer(id, None, children);
Ok(self.push_syntax_node(sn))
@@ -563,7 +563,9 @@ impl<'p> SyntaxPlan<'p> {
}
Relational::ScanRelation { alias, .. } => {
let children: Vec<usize> = if let Some(name) = alias {
- vec![self.nodes.push_syntax_node(SyntaxNode::new_alias(name))]
+ vec![self
+ .nodes
+ .push_syntax_node(SyntaxNode::new_alias(name.clone()))]
} else {
Vec::new()
};
@@ -572,7 +574,7 @@ impl<'p> SyntaxPlan<'p> {
}
Relational::Motion { .. } => {
let vtable = self.plan.get_motion_vtable(id)?.clone();
- let vtable_alias = &vtable.get_alias();
+ let vtable_alias = vtable.get_alias().map(String::from);
let child_id = self.plan.get_motion_child(id)?;
let child_rel = self.plan.get_ir_plan().get_relation_node(child_id)?;
let mut children: Vec<usize> = Vec::new();
@@ -645,7 +647,9 @@ impl<'p> SyntaxPlan<'p> {
let sn = SyntaxNode::new_pointer(
id,
Some(self.nodes.get_syntax_node_id(*child)?),
- vec![self.nodes.push_syntax_node(SyntaxNode::new_alias(name))],
+ vec![self
+ .nodes
+ .push_syntax_node(SyntaxNode::new_alias(name.clone()))],
);
Ok(self.nodes.push_syntax_node(sn))
}
diff --git a/src/executor/shard.rs b/src/executor/shard.rs
index 17a54ab191..d4f5da62b1 100644
--- a/src/executor/shard.rs
+++ b/src/executor/shard.rs
@@ -62,7 +62,7 @@ impl<'e> ExecutionPlan<'e> {
dist_keys.extend(motion_shard_keys);
let filter_nodes = self.get_bool_eq_with_rows(node_id);
- let ir_plan = &mut self.get_ir_plan().clone();
+ let ir_plan = &mut self.get_ir_plan();
for filter_id in filter_nodes {
let node = ir_plan.get_expression_node(filter_id)?;
@@ -81,7 +81,7 @@ impl<'e> ExecutionPlan<'e> {
}
let right_expr = ir_plan.get_expression_node(right_id)?;
let right_columns = if let Expression::Row { list, .. } = right_expr {
- list.clone()
+ list
} else {
// We should never get here.
continue;
@@ -97,7 +97,7 @@ impl<'e> ExecutionPlan<'e> {
// Gather right constants corresponding to the left keys.
if let Distribution::Segment { keys } = left_dist {
for key in keys {
- let positions: Vec<usize> = key.positions.clone();
+ let positions: Vec<usize> = key.positions;
// FIXME: we don't support complex distribution keys (ignore them).
if positions.len() > 1 {
return Ok(None);
diff --git a/src/executor/vtable.rs b/src/executor/vtable.rs
index 9d99599493..f3220765c5 100644
--- a/src/executor/vtable.rs
+++ b/src/executor/vtable.rs
@@ -165,8 +165,8 @@ impl VirtualTable {
/// Get vtable alias name
#[must_use]
- pub fn get_alias(&self) -> Option<String> {
- self.name.clone()
+ pub fn get_alias(&self) -> Option<&String> {
+ self.name.as_ref()
}
}
diff --git a/src/frontend/sql.rs b/src/frontend/sql.rs
index 6aefb34e68..0c995c1295 100644
--- a/src/frontend/sql.rs
+++ b/src/frontend/sql.rs
@@ -109,7 +109,7 @@ impl Ast for AbstractSyntaxTree {
let mut col_idx: usize = 0;
for (_, id) in dft_post {
- let node = self.nodes.get_node(*id)?.clone();
+ let node = self.nodes.get_node(*id)?;
match &node.rule {
Type::Scan => {
let ast_child_id = node.children.get(0).ok_or_else(|| {
@@ -386,7 +386,7 @@ impl Ast for AbstractSyntaxTree {
| Type::Null
| Type::True
| Type::False => {
- let val = Value::from_node(&node)?;
+ let val = Value::from_node(node)?;
map.add(*id, plan.add_const(val));
}
Type::Parameter => {
@@ -447,8 +447,8 @@ impl Ast for AbstractSyntaxTree {
}
Type::Row => {
let mut plan_col_list = Vec::new();
- for ast_child_id in node.children {
- let plan_child_id = map.get(ast_child_id)?;
+ for ast_child_id in &node.children {
+ let plan_child_id = map.get(*ast_child_id)?;
// If the child is a row that was generated by our
// reference-to-row logic in AST code, we should unwrap it back.
let plan_id = if rows.get(&plan_child_id).is_some() {
diff --git a/src/frontend/sql/ir.rs b/src/frontend/sql/ir.rs
index 7406e2857f..2456a5128f 100644
--- a/src/frontend/sql/ir.rs
+++ b/src/frontend/sql/ir.rs
@@ -60,8 +60,8 @@ impl Value {
/// Returns `QueryPlannerError` when the operator is invalid.
#[allow(dead_code)]
pub(super) fn from_node(s: &ParseNode) -> Result<Self, QueryPlannerError> {
- let val = match s.clone().value {
- Some(v) => v,
+ let val = match &s.value {
+ Some(v) => v.clone(),
None => "".into(),
};
diff --git a/src/ir/expression.rs b/src/ir/expression.rs
index 855ef008f0..9c23ed2539 100644
--- a/src/ir/expression.rs
+++ b/src/ir/expression.rs
@@ -896,19 +896,19 @@ impl Plan {
let nodes: Vec<usize> = subtree.map(|(_, id)| *id).collect();
let mut map: HashMap<usize, usize> = HashMap::with_capacity(nodes.len());
for id in nodes {
- let expr = self.get_expression_node(id)?.clone();
+ let expr = self.get_expression_node(id)?;
let new_id = match expr {
Expression::Alias { .. }
| Expression::Constant { .. }
- | Expression::Reference { .. } => self.nodes.push(Node::Expression(expr)),
+ | Expression::Reference { .. } => self.nodes.push(Node::Expression(expr.clone())),
Expression::Bool { left, op, right } => {
- let new_left_id = *map.get(&left).ok_or_else(|| {
+ let new_left_id = *map.get(left).ok_or_else(|| {
QueryPlannerError::CustomError(format!(
"Left child of bool node {} wasn't found in the clone map",
id
))
})?;
- let new_right_id = *map.get(&right).ok_or_else(|| {
+ let new_right_id = *map.get(right).ok_or_else(|| {
QueryPlannerError::CustomError(format!(
"Right child of bool node {} wasn't found in the clone map",
id
@@ -919,7 +919,7 @@ impl Plan {
Expression::Row { list, distribution } => {
let mut new_list: Vec<usize> = Vec::with_capacity(list.len());
for column_id in list {
- let new_column_id = *map.get(&column_id).ok_or_else(|| {
+ let new_column_id = *map.get(column_id).ok_or_else(|| {
QueryPlannerError::CustomError(format!(
"Row column node {} wasn't found in the clone map",
id
@@ -930,7 +930,7 @@ impl Plan {
self.nodes.add_row(new_list, distribution.clone())
}
Expression::Unary { op, child } => {
- let new_child_id = *map.get(&child).ok_or_else(|| {
+ let new_child_id = *map.get(child).ok_or_else(|| {
QueryPlannerError::CustomError(format!(
"Child of unary node {} wasn't found in the clone map",
id
diff --git a/src/ir/operator.rs b/src/ir/operator.rs
index bc659259a3..be11832eb3 100644
--- a/src/ir/operator.rs
+++ b/src/ir/operator.rs
@@ -865,14 +865,7 @@ impl Plan {
col_idx: &mut usize,
) -> Result<usize, QueryPlannerError> {
let row = self.get_expression_node(row_id)?;
- let columns = if let Expression::Row { list, .. } = row {
- list.clone()
- } else {
- return Err(QueryPlannerError::CustomError(format!(
- "Expression {} is not a row.",
- row_id
- )));
- };
+ let columns = row.clone_row_list()?;
let mut aliases: Vec<usize> = Vec::with_capacity(columns.len());
for col_id in columns {
// Generate a row of aliases for the incoming row.
diff --git a/src/ir/transformation/redistribution.rs b/src/ir/transformation/redistribution.rs
index 2bfd9bbb9b..58d53f0efd 100644
--- a/src/ir/transformation/redistribution.rs
+++ b/src/ir/transformation/redistribution.rs
@@ -564,8 +564,8 @@ impl Plan {
left_row_id: usize,
right_row_id: usize,
) -> Result<MotionPolicy, QueryPlannerError> {
- let left_dist = self.get_distribution(left_row_id)?.clone();
- let right_dist = self.get_distribution(right_row_id)?.clone();
+ let left_dist = self.get_distribution(left_row_id)?;
+ let right_dist = self.get_distribution(right_row_id)?;
let get_policy_for_one_side_segment = |row_map: &HashMap<usize, usize>,
keys_set: &HashSet<Key>|
@@ -605,7 +605,7 @@ impl Plan {
for (outer_keys, inner_keys) in pairs {
for outer_key in outer_keys {
if first_outer_key.is_none() {
- first_outer_key = Some(outer_key.clone());
+ first_outer_key = Some(outer_key);
}
for inner_key in inner_keys {
if outer_key == inner_key {
@@ -623,11 +623,11 @@ impl Plan {
}
(Distribution::Segment { keys: keys_set }, _) => {
let row_map = self.build_row_map(left_row_id)?;
- get_policy_for_one_side_segment(&row_map, &keys_set)
+ get_policy_for_one_side_segment(&row_map, keys_set)
}
(_, Distribution::Segment { keys: keys_set }) => {
let row_map = self.build_row_map(right_row_id)?;
- get_policy_for_one_side_segment(&row_map, &keys_set)
+ get_policy_for_one_side_segment(&row_map, keys_set)
}
_ => Ok(MotionPolicy::Full),
}
--
GitLab