feat(store): implement AvlTree::remove

[rnbguy]

Closes #140

A short summary of how AvlTree::remove is implemented,

• Find the sub-tree with the provided key.
• Remove the top node of this sub-tree.
• To remove a top node of a tree, we replaced it with a new node.
  • If the current node has no children, the new node is None.
  • If the current node has only one child, the new node is the child.
  • If the current node has both children.
  • If left child is shorter: the new node is the leftmost node in the right subtree.
    Also, the current node's children are set to the new node's children.
  • If right child is shorter, vice versa.

This runs in O(log(n)) as the recursive calls are always made on a subtree.

[rnbguy]

I noticed that the current implementation uses &mut old_value to pass around return values. It may make sense to use this if this mutable reference is modified multiple times. But it is only set once in a search. Can we switch to return rather?

For example, I want to refactor the following

basecoin-rs/crates/store/src/avl/tree.rs

Lines 66 to 78 in ef13279

	fn insert_rec(node_ref: &mut NodeRef<K, V>, key: K, value: V, old_value: &mut Option<V>) {
	if let Some(node) = node_ref {
	match node.key.cmp(&key) {
	Ordering::Greater => AvlTree::insert_rec(&mut node.left, key, value, old_value),
	Ordering::Less => AvlTree::insert_rec(&mut node.right, key, value, old_value),
	Ordering::Equal => *old_value = Some(node.set_value(value)),
	}
	node.update();
	AvlTree::balance_node(node_ref);
	} else {
	*node_ref = as_node_ref(key, value);
	}
	}

to

    fn insert_rec(node_ref: &mut NodeRef<K, V>, key: K, value: V) -> Option<V> {
        if let Some(node) = node_ref {
            let old_value = match node.key.cmp(&key) {
                Ordering::Greater => AvlTree::insert_rec(&mut node.left, key, value),
                Ordering::Less => AvlTree::insert_rec(&mut node.right, key, value),
                Ordering::Equal => Some(node.set_value(value)),
            };
            node.update();
            AvlTree::balance_node(node_ref);
            old_value
        } else {
            *node_ref = as_node_ref(key, value);
            None
        }
    }

[ancazamfir]

Did a first pass on this, found the multiple recursive functions and the mem::replace()s make the code a bit unclear, making it harder to say there are no issues with the code :)

[ancazamfir]

I guess here, since there is no right child, we could have just linked the nodes parent to the left child. Would require changes in remove_rec though. But wouldn't need the remove_leftmost.

[rnbguy]

This was very helpful ! 😄 It ended up not requiring remove_rightmost method.

6058561

[rnbguy]

I ended up keeping the same code (with a slight modification) - but handling the no-sibling cases separately.

661d2d5

[ancazamfir]

Inside this remove_leftmost function we also take the leftmost's right child and put it in the leftmost's parent left child.

[rnbguy]

Not really sure if this was a note. Note that, removed nodes are always a leaf. This is an invariant. Their left and right nodes are set to None.

So, what I am doing here is:

1. point leftmost_node.left to where node.left points.
2. point leftmost_node.right to where node.right points. (since, I called, node.{right,left}.take(), node is now a leaf too.)
3. replace node with leftmost_node and return the old replaced value (which is a leaf) as the removed value.

[ancazamfir]

sorry it was a note, that remove_leftmost had a misleading name for me in the beginning.

[ancazamfir]

Isn't this the same as:

leftmost_node.right = node.right.take();

Why do we need mem::replace?

[rnbguy]

you're right. I just wanted to assert the condition in one line. 8958a20

[ancazamfir]

Maybe it would be good to have a test where we remove one key and make sure the others are still present and the tree is balanced.

[rnbguy]

good point. I am now testing balancing factor and present keys after each insert and delete.

c8cfa04

[rnbguy]

This PR should remove/deprecate GrowingStore. Same for RevertibleStore via #160.

[ancazamfir]

Looks good!