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

refactor: remove result where not required on tree and snapshot #38

Merged
merged 1 commit into from
Jul 4, 2024
Merged

refactor: remove result where not required on tree and snapshot #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
96 changes: 40 additions & 56 deletions src/art.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@ impl<P: KeyTrait, V: Clone> Node<P, V> {
commit_version: u64,
ts: u64,
depth: usize,
) -> Result<(NodeArc<P, V>, Option<V>), TrieError> {
) -> (NodeArc<P, V>, Option<V>) {
// Obtain the current node's prefix and its length.
let cur_node_prefix = cur_node.prefix().clone();
let cur_node_prefix_len = cur_node.prefix().len();
Expand All @@ -662,19 +662,19 @@ impl<P: KeyTrait, V: Clone> Node<P, V> {
if is_prefix_match && cur_node_prefix.len() == key_prefix.len() {
let new_twig = twig.insert(value, commit_version, ts);
if let Some(old_val) = twig.get_leaf_by_version(commit_version) {
return Ok((
return (
Arc::new(Node {
node_type: NodeType::Twig(new_twig),
}),
Some(old_val.value.clone()),
));
);
} else {
return Ok((
return (
Arc::new(Node {
node_type: NodeType::Twig(new_twig),
}),
None,
));
);
}
}
}
Expand All @@ -695,30 +695,24 @@ impl<P: KeyTrait, V: Clone> Node<P, V> {
ts,
);
n4 = n4.add_child(k1, old_node).add_child(k2, new_twig);
return Ok((Arc::new(n4), None));
return (Arc::new(n4), None);
}

// Continue the insertion process by finding or creating the appropriate child node for the next character.
let k = key_prefix[longest_common_prefix];
let child_for_key = cur_node.find_child(k);
if let Some(child) = child_for_key {
match Node::insert_recurse(
let (new_child, old_value) = Node::insert_recurse(
child,
key,
value,
commit_version,
ts,
depth + longest_common_prefix,
) {
Ok((new_child, old_value)) => {
let new_node = cur_node.replace_child(k, new_child);
return Ok((Arc::new(new_node), old_value));
}
Err(err) => {
return Err(err);
}
}
};
);
let new_node = cur_node.replace_child(k, new_child);
return (Arc::new(new_node), old_value);
}

// If no child exists for the key's character, create a new Twig node and add it as a child.
let new_twig = Node::new_twig(
Expand All @@ -729,7 +723,7 @@ impl<P: KeyTrait, V: Clone> Node<P, V> {
ts,
);
let new_node = cur_node.add_child(k, new_twig);
Ok((Arc::new(new_node), None))
(Arc::new(new_node), None)
}

/// Removes a key recursively from the node and its children.
Expand Down Expand Up @@ -953,7 +947,7 @@ impl<P: KeyTrait, V: Clone> Tree<P, V> {
} else if check_version && curr_version >= version {
return Err(TrieError::VersionIsOld);
}
Node::insert_recurse(root, key, value, commit_version, ts, 0)?
Node::insert_recurse(root, key, value, commit_version, ts, 0)
}
};

Expand Down Expand Up @@ -1042,21 +1036,15 @@ impl<P: KeyTrait, V: Clone> Tree<P, V> {
)))
}
Some(root) => {
match Node::insert_recurse(
let (new_node, _) = Node::insert_recurse(
root,
&new_kv.key,
new_kv.value,
new_kv.version,
new_kv.ts,
0,
) {
Ok((new_node, _)) => {
self.root = Some(new_node);
}
Err(err) => {
return Err(err);
}
}
);
self.root = Some(new_node)
}
}

Expand Down Expand Up @@ -1116,15 +1104,15 @@ impl<P: KeyTrait, V: Clone> Tree<P, V> {
(new_root, is_deleted)
}

pub fn remove(&mut self, key: &P) -> Result<bool, TrieError> {
pub fn remove(&mut self, key: &P) -> bool {
// Directly match on the root to simplify logic
let (new_root, is_deleted) = Tree::remove_from_node(self.root.as_ref(), key);

// Update the root and return the deletion status.
if is_deleted {
self.root = new_root;
}
Ok(is_deleted)
is_deleted
}

pub fn get(&self, key: &P, version: u64) -> Option<(V, u64, u64)> {
Expand Down Expand Up @@ -1163,12 +1151,10 @@ impl<P: KeyTrait, V: Clone> Tree<P, V> {
/// Returns a `Result` containing the `Snapshot` if the snapshot is created successfully,
/// or an `Err` with an appropriate error message if creation fails.
///
pub fn create_snapshot(&self) -> Result<Snapshot<P, V>, TrieError> {
pub fn create_snapshot(&self) -> Snapshot<P, V> {
let root = self.root.as_ref().cloned();
let version = self.root.as_ref().map_or(1, |root| root.version() + 1);
let new_snapshot = Snapshot::new(root, version);

Ok(new_snapshot)
Snapshot::new(root, version)
}

/// Creates an iterator over the Trie's key-value pairs.
Expand Down Expand Up @@ -1300,7 +1286,7 @@ mod tests {
// Deletion phase
for word in &words {
let key = VariableSizeKey::from_str(word).unwrap();
assert!(tree.remove(&key).unwrap());
assert!(tree.remove(&key));
}
} else if let Err(err) = read_words_from_file(file_path) {
eprintln!("Error reading file: {}", err);
Expand All @@ -1324,9 +1310,7 @@ mod tests {

// Deletion phase
for word in &insert_words {
assert!(tree
.remove(&VariableSizeKey::from_str(word).unwrap())
.unwrap());
assert!(tree.remove(&VariableSizeKey::from_str(word).unwrap()));
}
}

Expand Down Expand Up @@ -1394,7 +1378,7 @@ mod tests {
let _ = tree.insert(&key, value, 0, 0);

// Removal
assert!(tree.remove(&key).unwrap());
assert!(tree.remove(&key));

// Verification
assert!(tree.get(&key, 0).is_none());
Expand All @@ -1412,7 +1396,7 @@ mod tests {
tree.insert(&key2, 1, 0, 0).unwrap();

// Removal
assert!(tree.remove(&key1).unwrap());
assert!(tree.remove(&key1));

// Root verification
if let Some(root) = &tree.root {
Expand All @@ -1436,7 +1420,7 @@ mod tests {
tree.insert(&key2, 1, 0, 0).unwrap();

// Removal
assert!(tree.remove(&key1).unwrap());
assert!(tree.remove(&key1));

// Root verification
if let Some(root) = &tree.root {
Expand Down Expand Up @@ -1481,7 +1465,7 @@ mod tests {

// Removal
let key_to_remove = VariableSizeKey::from_slice(&1u32.to_be_bytes());
assert!(tree.remove(&key_to_remove).unwrap());
assert!(tree.remove(&key_to_remove));

// Root verification
if let Some(root) = &tree.root {
Expand Down Expand Up @@ -1529,7 +1513,7 @@ mod tests {

// Removal
let key_to_remove = VariableSizeKey::from_slice(&2u32.to_be_bytes());
assert!(tree.remove(&key_to_remove).unwrap());
assert!(tree.remove(&key_to_remove));

// Root verification
if let Some(root) = &tree.root {
Expand Down Expand Up @@ -1596,7 +1580,7 @@ mod tests {

// Removal
let key_to_remove = VariableSizeKey::from_slice(&2u32.to_be_bytes());
assert!(tree.remove(&key_to_remove).unwrap());
assert!(tree.remove(&key_to_remove));

// Root verification
if let Some(root) = &tree.root {
Expand Down Expand Up @@ -1807,8 +1791,8 @@ mod tests {
assert_eq!(tree.version(), 2);

// Deletions
assert!(tree.remove(&key1).unwrap());
assert!(tree.remove(&key2).unwrap());
assert!(tree.remove(&key1));
assert!(tree.remove(&key2));
assert_eq!(tree.version(), 0);
}

Expand Down Expand Up @@ -2086,7 +2070,7 @@ mod tests {
let key_to_delete = VariableSizeKey {
data: key_data_to_delete.to_vec(),
};
tree.remove(&key_to_delete).unwrap();
tree.remove(&key_to_delete);

// Check remaining keys are still present
for (remaining_index, remaining_key_data) in set_keys.iter().enumerate() {
Expand Down Expand Up @@ -2150,7 +2134,7 @@ mod tests {
let key_to_delete = VariableSizeKey {
data: key_data_to_delete.to_vec(),
};
tree.remove(&key_to_delete).unwrap();
tree.remove(&key_to_delete);

// Check remaining keys are still present
for (remaining_index, remaining_key_data) in set_keys.iter().enumerate() {
Expand Down Expand Up @@ -2252,15 +2236,15 @@ mod tests {
fn remove_non_existent_key() {
let mut tree = Tree::<VariableSizeKey, i32>::new();
let key = VariableSizeKey::from_str("nonexistent").unwrap();
let is_removed = tree.remove(&key).unwrap();
let is_removed = tree.remove(&key);
assert!(!is_removed);
}

#[test]
fn remove_key_from_empty_tree() {
let mut tree = Tree::<VariableSizeKey, i32>::new();
let key = VariableSizeKey::from_str("test").unwrap();
let is_removed = tree.remove(&key).unwrap();
let is_removed = tree.remove(&key);
assert!(!is_removed);
}

Expand All @@ -2279,7 +2263,7 @@ mod tests {

// Remove keys sequentially
for key in &keys {
assert!(tree.remove(key).unwrap());
assert!(tree.remove(key));
}

// Verify all keys are removed
Expand All @@ -2303,7 +2287,7 @@ mod tests {

// Remove all keys
for key in &keys {
let is_removed = tree.remove(key).unwrap();
let is_removed = tree.remove(key);
assert!(is_removed);
}
}
Expand All @@ -2317,7 +2301,7 @@ mod tests {
// Initial insert
let _ = tree.insert(&key1, 1, 0, 0);
// Remove
assert!(tree.remove(&key1).unwrap());
assert!(tree.remove(&key1));
// Insert another key
let _ = tree.insert(&key2, 2, 0, 0);

Expand Down Expand Up @@ -2501,7 +2485,7 @@ mod tests {
for i in 76..=100 {
let key = VariableSizeKey::from_str(&format!("key{}", i)).unwrap();
// Since these keys do not exist, remove should return an Err or false depending on implementation
assert!(tree.remove(&key).is_err() || !tree.remove(&key).unwrap());
assert!(!tree.remove(&key));
}

// Verify versions of a few keys
Expand All @@ -2525,7 +2509,7 @@ mod tests {
for i in 2..=2 {
let key = VariableSizeKey::from_str(&format!("key{}", i)).unwrap();
// Since these keys do not exist, remove should return an Err or false depending on implementation
assert!(tree.remove(&key).is_err() || !tree.remove(&key).unwrap());
assert!(!tree.remove(&key));
}

let key1 = VariableSizeKey::from_str("key1").unwrap();
Expand All @@ -2542,7 +2526,7 @@ mod tests {
tree.insert(&key, 1, 1, 0).unwrap();

// Remove the inserted key
tree.remove(&key).unwrap();
tree.remove(&key);

// Verify the tree is empty
assert!(tree.root.is_none());
Expand Down
Loading
Loading