60 add avl tree as an alternative backend data structures for copy efficent persistent data structures#61
Conversation
Balance is not stored as metadata and therefore does not benefit from a smaller byte field. Returning it as int is more suitable for arithmetic.
| import org.junit.Test; | ||
|
|
||
| @SuppressWarnings("MemberName") | ||
| public class PathCopyingPersistentAvlTreeMapTest { |
There was a problem hiding this comment.
The optimal way of setting up the tests would be to have all tests for PersistentMaps (i.e. tests for maps in general) in a single class and execute it as a parameterized test for all of our map implementations. JUnit 4 provides this feature relatively easily. An example implementation of ours can be found here.
I would suggest to add a new class that handles parameterized testing of PersistentMaps (similar to SolverBasedTest0) and then extend PathCopyingPersistentTreeMapTest with it + tests from this class that are not yet present in PathCopyingPersistentTreeMapTest. (The name PathCopyingPersistentTreeMapTest indicates that it runs all kinds of tree based persistent maps, instead of just RB-Trees anyways)
Test cases that use internal API of your AVL-Tree should remain here (if any).
Tests that test the (currently 2, so parameterizing does not make sense yet) maps against each other should be moved to a dedicated test-class with a proper name.
But there are some other classes that only test on PathCopyingPersistentTreeMap, e.g. CopyOnWriteSortedMapTest, that should also be parameterized.
|
|
||
| @After | ||
| public void tearDown() { | ||
| map = null; |
There was a problem hiding this comment.
This is not needed. Since your setup method uses @Before, it is guaranteed that you start with a fresh map for each test. This would also allow you to remove the @Nullable annotation (that you don't want to use anyway. Avoiding null and treating it as an error should be the default nowadays)
| private @Nullable PersistentSortedMap<String, String> llrb; | ||
| private @Nullable NavigableMap<String, String> reference; |
There was a problem hiding this comment.
Since you don't use these 2 in your setup, you should delete them and use local variables instead.
|
|
||
| try { | ||
| ((PathCopyingPersistentAvlTreeMap<?, ?>) map).checkAssertions(); | ||
| } catch (IllegalStateException e) { |
There was a problem hiding this comment.
Why not just throw/not catch the IllegalStateException?
| put(null, key, value); | ||
| } | ||
|
|
||
| private void put(@Nullable String context, String key, String value) { |
There was a problem hiding this comment.
You could improve the naming of this method and add what it does. Also, you should add some JavaDoc to explain the purpose of this method.
| @Var NavigableMap<String, String> subsubmap = submap.subMap("aaa", true, "c", false); | ||
| assertThat(subsubmap).containsExactly("b", "b"); | ||
|
|
||
| subsubmap = submap.subMap("aa", /* fromInclusive= */ true, "bb", /* toInclusive= */ false); |
There was a problem hiding this comment.
Whats the purpose of these comments: fromInclusive etc?
| return suite; | ||
| } | ||
|
|
||
| private @Nullable PersistentSortedMap<String, String> map; |
There was a problem hiding this comment.
Why only test <String, String>?
| // a library of useful utilities: | ||
| // https://github.com/sosy-lab/java-common-lib | ||
| // | ||
| // SPDX-FileCopyrightText: 2007-2020 Dirk Beyer <https://www.sosy-lab.org> |
| * Landis (AVL) tree and bottom-up path copying for copy-efficient persistency. | ||
| * | ||
| * <p> | ||
| * TODO: Finish documentation |
| import org.junit.Test; | ||
|
|
||
| @SuppressWarnings("MemberName") | ||
| public class PathCopyingPersistentAvlTreeMapTest { |
There was a problem hiding this comment.
Did you test all operations (e.g. rotation cases) of AVL trees?
If not, i would suggest to map out cases for which you can trigger all of them and add them here.
2 participants
Summary
Adds an initial path-copying persistent AVL tree map implementation based on the existing
PathCopyingPersistentTreeMapwhich is based on a LLRB tree.Changes
Changes to the existing tree map implementation focus on replacing LLRB specific logic with their AVL counterparts:
PathCopyingPersistentAvlTreeMaprebalance().restructure().(See https://www.csl.mtu.edu/cs2321/www/newLectures/20_AVL_Tree.html for more information on restructuring algorithm)
checkAssertions()to validate AVL height and balance invariants instead of LLRB invariants.putAndCopy()to use AVL rebalancing.removeAndCopy()to use standard BST deletion followed by AVL rebalancing.restructure()for the two child deletion case, choosing the predecessor/successor replacement based on the larger subtree height.(See https://arxiv.org/pdf/2406.05162 for Browns improved deletion algorithm)
Reused structures
The implementation intentionally keeps most of the existing persistent tree map structure unchanged where logic is not tied to the LLRB tree implementation and only requires the existence of a binary search tree.