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

chore: add ForceUpdate to BPTree #3993

Merged
merged 5 commits into from
Nov 4, 2024
Merged

chore: add ForceUpdate to BPTree #3993

merged 5 commits into from
Nov 4, 2024

Conversation

kostasrim
Copy link
Contributor

ReallocIfNeeded deleted and reinserted the same key in the BPTree stored in SortedMap. This is not really needed since the key is actually the same and we can just update the pointer within BPTree instead of doing the deletion + insertion chore.

Resolves #3963

@kostasrim kostasrim self-assigned this Oct 25, 2024
@kostasrim kostasrim changed the title chore: add ForceUpdate in BPTree chore: add ForceUpdate yo BPTree Oct 25, 2024
@kostasrim kostasrim changed the title chore: add ForceUpdate yo BPTree chore: add ForceUpdate to BPTree Oct 25, 2024
@kostasrim kostasrim marked this pull request as ready for review October 30, 2024 09:34
score_tree->Delete(old_obj);
score_tree->Insert(new_obj);
};
auto cb = [this](sds old_obj, sds new_obj) { score_tree->ForceUpdate(old_obj, new_obj); };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when I checked if old_obj is still alive when thsi callback is called, I realised that ReallocIfNeededGeneric is really a bad name because it does not REALLOCATE. The better name would be DuplicateEntryIfFragmented

Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM besides one comment. Have you checked that ForceUpdate works?

@kostasrim
Copy link
Contributor Author

LGTM besides one comment. Have you checked that ForceUpdate works?

SortedMapTest::ReallocIfNeeded test should cover this because at the end we call sm_.Iterate(0, 10000, false, cb); which traverses and accesses the BPTree. However, I could add a specific unit test if you think this is not sufficient 🤷‍♂️

@kostasrim
Copy link
Contributor Author

LGTM besides one comment. Have you checked that ForceUpdate works?

SortedMapTest::ReallocIfNeeded test should cover this because at the end we call sm_.Iterate(0, 10000, false, cb); which traverses and accesses the BPTree. However, I could add a specific unit test if you think this is not sufficient 🤷‍♂️

IMO It's not a bad idea to add a unit test so I will add one 😄

@@ -585,4 +589,15 @@ template <typename T, typename Policy> void BPTree<T, Policy>::DestroyNode(BPTre
num_nodes_--;
}

template <typename T, typename Policy> void BPTree<T, Policy>::ForceUpdate(KeyT old, KeyT new_obj) {
Copy link
Contributor Author

@kostasrim kostasrim Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth asserting that Policy::operator(old, new_obj) returns 0

romange
romange previously approved these changes Nov 4, 2024
@romange
Copy link
Collaborator

romange commented Nov 4, 2024

Please rebase. Also reallocated variable still has the old name :)

@kostasrim kostasrim merged commit 9c2fc3f into main Nov 4, 2024
12 checks passed
@kostasrim kostasrim deleted the kpr5 branch November 4, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ForceUpdate(Key) in BPTree to force update pointers in SortedMap
2 participants