[sowashieristjaschoninteressant]

Description

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

Key Changes

2 new files added b_plus_tree and test_b_plus_tree
the files have the bPlusTree datastructure and tests to it

Validation

Checklist

• I reviewed the diff and kept it focused on the described change.
• Python changes pass uv run ruff check . and uv run ruff format --check ., or this PR does not change Python.
• Changed behavior is covered by appropriate tests, including a regression test for bug fixes; any limitations are explained under Validation.
• Validation lists the checks or manual flows I ran and their results.
• Relevant documentation is updated, or no documentation change is needed.

Closes #333

[indrakantisaidharahas]

I have a query regarding the B+ tree implementation.

The code in the pull request looks good to me, but while reading it I had a doubt. How do we map the indexes in the B+ tree classes to the pointers we have in the disk pages? Will we face any conflict there, and if yes, do we use a translation table for it?

For example, here the traversal moves through object references:

internalNode: _InternalNode = node
nodeIndex = self._find_position(internalNode._keys, key)
node = internalNode._children[nodeIndex]

[mattsva]

Hope that helps

[mattsva]

Wouldnt it be good to make it threadsafe, to make it able to be multi threaded?

[sowashieristjaschoninteressant]

yes it would be good haha one thing after another i was thinking about maby writing the datastructure not safe and have another class called smth like ConcurrentBPlusTree that just functions as a wrapper with locks instead of programming all of that into one class. maby multible strategys ( tryPop, Waitpop) idk i need to think about it

[mattsva]

Yeah, I actually like the wrapper idea. Keeping the base BPlusTree thread-unsafe avoids unnecessary overhead and complexity for single-threaded use, while a ConcurrentBPlusTree could handle synchronization separatelysingle lock around oper. I'd probably start with a ations to keep it correct and simple, then consider read/write or finer-grained locking later if performance calls for it. waitPop could use a condition variable if you want blocking queue semantics too.
I would just like to have it added in this PR here. I dont really care where you put it, imo its just needed to be added here - this is your PR so maybe you think about doing such a thing. Nevertheless, this is a good PR.

[sowashieristjaschoninteressant]

I have a query regarding the B+ tree implementation.

The code in the pull request looks good to me, but while reading it I had a doubt. How do we map the indexes in the B+ tree classes to the pointers we have in the disk pages? Will we face any conflict there, and if yes, do we use a translation table for it?

For example, here the traversal moves through object references:

internalNode: _InternalNode = node
nodeIndex = self._find_position(internalNode._keys, key)
node = internalNode._children[nodeIndex]

well that indeed is a serious concern hmmm. i guess in case of serialized leaf nodes we just put the pageId into the children array and when the serializer loads from the disk we need to switch this out for the object if its in memory??

damn i think either way its better to write the tree firstly into memory and test it and later on i guess we have to change parts of it.

[mohammedFawzy0111]

quick question we already have Node data class defined in storage_format.py and PageSerializer only accepts that type. I would rather you use the already defined type. if this just a prototype to test the algorithm if so please change before submitting this pr for review.

[sowashieristjaschoninteressant]

okay i can use this but to be honest i dont like to use the same class for internal and leafnodes and differeantiating only via a boolean. i think 2 classes is way cleaner. But ey ur the boss please awnser to this and i just use the Node class in storage_format.py or maby alter it and also a internalNode class or smth

[mohammedFawzy0111]

Well if you want you can abstract the base of the Node class and make two distinct ones for leaf and internals. But this would mean fixing the usage in the Serializer. I am not sure either. But you can keep using your own for now. I will see if I can push modification to abstract it later

[mohammedFawzy0111]

@sowashieristjaschoninteressant
Hey! Good news, abstraction is merged so you get the two-class design you wanted. storage_format.py now has a separate LeafNode and InternalNode, plus Node as a union alias of the two. PageSerializer accepts both, so you can drop your own _Node / _LeafNode / _InternalNode and use these directly. isinstance(node, LeafNode) works the same way as in your current code.

Some things will look a bit different from your code:

• Page ids instead of pointers. Children and the leaf chain are referenced by id (children_ids, next_page_id). So the tree needs some way to go from an id to a node (a plain dict is fine for now, we can swap in a pager later) and a way to allocate new page ids on splits. Page 0 is reserved for metadata, so ids start at 1.
• No _parent. Nodes don't store one. Keep the path while you descend (e.g. a stack of nodes and child indexes) and walk back up that when a split has to propagate.
• Child layout. leftmost_child_id is the child for keys below keys[0], and children_ids[i] is the child for keys >= keys[i]. The serializer expects len(keys) == len(children_ids). all_children_ids() gives you the full list, so if you use bisect index lines up with it. On an internal split, the left half keeps its leftmost child, and the right child of the middle key becomes the new node's leftmost_child_id. friendly wanrnin: That's the spot where off-by-one bugs usually happen.
• Keys are bytes. Comparison and bisect still work, you just need to change the type hints.

The serializer also raises a SerializationError if a node doesn't fit in one page. Splitting by key count is fine while you're testing the algorithm, but we'll want a size-based check eventually.

Your split logic (copy the separator up for leaves, move it up for internals) carries over as is, only the pointer handling changes. Ping me if the serializer gets in your way.

[sowashieristjaschoninteressant]

yes saw it im refactoring my tree! thx

[mohammedFawzy0111]

this follows only one path. it pops the last child each time so it checks the rightmost branch and returns at the first leaf. the rest of the tree is never validated. It should walk every node and check sorted keys, child keys ranges etc.

[mohammedFawzy0111]

the stress test only inserts ascending keys, so it only exercises right-edge splits. Please add descending and shuffled (seeded) inserts, and read every key bakc afterwards.

[mattsva]

These are just the issues I spotted during my second pass through the code, so there may be more that Ive missed. Its easy to overlook potential issues here because some parts of the code may look correct at first glance but arent necessarily implemented in the best or most robust way. Fixing the points Ive raised doesnt automatically mean the implementation is approved, so please review the code as a whole and keep an eye out for other potential issues.

[mattsva]

If the key already exists, you return False, but the _size gets still increased.
I dont think that this is the right behavior.

[mattsva]

This is currently O(n), where n is the number of keys in a node (bounded by maxChildren - 1), rather than O(log n) for searching within a node.
Wouldn't binary search be a better approach here?
We could use Python's standard library bisect module (bisect_right()), or implement binary search ourselves.
For example:

def _find_position(self, keys: list[int], key: int) -> int:
    left = 0
    right = len(keys)

    while left < right:
        mid = (left + right) // 2
        if keys[mid] <= key:
            left = mid + 1
        else:
            right = mid

    return left

This implements the same behavior as bisect_right() and reduces the search complexity from O(n) to O(log n).

Disclaimer: This only optimizes finding the insertion position. Inserting into a Python list still takes O(n) in the worst case because elements need to be shifted.

[mohammedFawzy0111]

agreed. but I would prefer if we used the standard one
we can do somethiong like this for lookup

i = bisect_left(node._keys, key)
if i < len(node._keys) and node._keys[i] == key:
    value = node._values[i]

or we can replace this whole method with:

def _find_position(self, keys: list[int], key: int) -> int:
    return bisect_right(keys, key)

[sowashieristjaschoninteressant]

well _find_position mostly needs to iterate through mostly lets say 4-5 elements depends how we set max children so were talking here about maby 3-4 (max) cycle optimisation per node _find_position. altough i generelly think you guys are completely right i dont think i want to optimize to mutch until the tree is working with all the components together firstly tbh. but again u call guys

[mohammedFawzy0111]

Just to add to that: the 4-5 keys per node is only the small maxChildren we use while testing. With the real page layout in storage_format.py (4096-byte pages, 32-byte header, 4-byte slots), an internal node with ~8-byte keys fits roughly 200+ keys, and leaves hold a few dozen depending on value size.

At that size, the linear loop in _find_position means ~200 comparisons per internal node in pure Python, on every get and put. bisect does the same in ~8 steps in C, so it isn't a micro-optimization.

It's also less code, and you're touching these functions anyway for the LeafNode/InternalNode switch. you could use bisect_right for routing in internal nodes, bisect_left for the exact-match check in leaves. but this your PR after all so pick what approach you prefer

[mattsva]

@sowashieristjaschoninteressant please use bisect. If you give me a reason, why this shouldnt be used, im okay with that, otherwise, use bisect please.
I guess @mohammedFawzy0111 explained it enough.
This PR is good, but those small details are important, because they have a big impact on performance.

[mohammedFawzy0111]

a docstring is a no-op expression, so use a comment instead.

[mattsva]

@sowashieristjaschoninteressant Id like to suggest a few changes to the code comments. Personally, I think there are too many, and theyre not particularly useful. Id prefer them to be shorter and focus more on what the code does rather than how it works. Everyone can read the Python code, so I think the comments should primarily help people navigate the codebase and understand the codes role within the overall system.
Thats obviously just my personal preference, though, so Id be interested to hear your reasoning as well.
This isnt a merge blocker, so please focus on the code itself for now. But if youd like, we can also discuss this a bit more.

(You could do this on this thread or we could create a discussion about that, Im open for booth, just ping me and Ill see it)

[VihaanReddyM]

@sowashieristjaschoninteressant any updates?

[sowashieristjaschoninteressant]

Hello Yes! im cureently searching for a job and hat a pretty big task to finish for a company in an application process. this took a lot of time. i will try to refactor my code today remove the comments and optimize the time insertion inthe find_position function aswell as writing the delete algo which will probably be rly hard. im sorry i know i said it before il make it count this time