shardtree: Use annotated parent nodes with empty children for inserted frontier ommers. #107
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nuttycom
commented
Jun 26, 2024
shardtree/src/prunable.rs
Outdated
| let LocatedTree { root_addr, root } = self; | ||
| if root_addr.contains(&subtree.root_addr) { | ||
| go(*root_addr, root, subtree, contains_marked).map(|(root, incomplete)| { | ||
| let new_tree = LocatedTree { | ||
| root_addr: *root_addr, | ||
| root, | ||
| }; | ||
| assert!(new_tree.max_position() >= max_position); |
There was a problem hiding this comment.
The removal of this assertion is intentional; given the way that shardtree was used prior to this PR, it is not possible to determine whether a Node::Leaf value in a persisted tree was present as a pruned subtree root, or because of direct insertion as a shard root or the ommer of an inserted frontier. In general, the maximum position of populated nodes within a subtree should only be used when considering the maximum inserted position of the overall tree.
nuttycom
force-pushed
the
shardtree/fix_inserted_parents
branch
from
3b66ad3 to
68935c8
Compare
str4d
requested changes
Jun 27, 2024
| @@ -235,17 +235,15 @@ impl< | |||
|
|
|||
| let (append_result, position, checkpoint_id) = | |||
| if let Some(subtree) = self.store.last_shard().map_err(ShardTreeError::Storage)? { | |||
| match subtree.max_position() { | |||
| // If the subtree is full, then construct a successor tree. | |||
|
Also, fix the clippy lints :) |
The current behavior of `unite` incorrectly discards annotation data.
nuttycom
force-pushed
the
shardtree/fix_inserted_parents
branch
2 times, most recently
from
25242e2 to
ca3e553
Compare
In 7e48886, `Node::Pruned` was introduced in order to fix problems with `LocatedTree::max_position` resulting from reinsertion of subtree information as children of a previously pruned subtree represented by a `Node::Leaf` at its root. This fix introduced a large amount of complexity that is better resolved by fixing the `max_position` function in a different way and generally minimizing its usage.
nuttycom
force-pushed
the
shardtree/fix_inserted_parents
branch
from
ca3e553 to
192cb3f
Compare
str4d
approved these changes
Jun 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.