Merge pull request #33 from CraigMacomber/bugFixes

fix mergeSibling not setting isShared and greedyClone(true) not deeply copying

Author: taylorsw04

b+tree.js

@@ -1390,6 +1390,10 @@ var BNode = /** @class */ (function () {
 /** Internal node (non-leaf node) ********************************************/
 var BNodeInternal = /** @class */ (function (_super) {
     __extends(BNodeInternal, _super);
+    /**
+     * This does not mark `children` as shared, so it is the responsibility of the caller
+     * to ensure that either children are marked shared, or it are not included in another tree.
+     */
     function BNodeInternal(children, keys) {
         var _this = this;
         if (!keys) {
@@ -1412,7 +1416,7 @@ var BNodeInternal = /** @class */ (function (_super) {
             return this;
         var nu = new BNodeInternal(this.children.slice(0), this.keys.slice(0));
         for (var i = 0; i < nu.children.length; i++)
-            nu.children[i] = nu.children[i].greedyClone();
+            nu.children[i] = nu.children[i].greedyClone(force);
         return nu;
     };
     BNodeInternal.prototype.minKey = function () {
@@ -1517,11 +1521,21 @@ var BNodeInternal = /** @class */ (function (_super) {
             return newRightSibling;
         }
     };
+    /**
+     * Inserts `child` at index `i`.
+     * This does not mark `child` as shared, so it is the responsibility of the caller
+     * to ensure that either child is marked shared, or it is not included in another tree.
+     */
     BNodeInternal.prototype.insert = function (i, child) {
         this.children.splice(i, 0, child);
         this.keys.splice(i, 0, child.maxKey());
     };
+    /**
+     * Split this node.
+     * Modifies this to remove the second half of the items, returning a separate node containing them.
+     */
     BNodeInternal.prototype.splitOffRightSide = function () {
+        // assert !this.isShared;
         var half = this.children.length >> 1;
         return new BNodeInternal(this.children.splice(half), this.keys.splice(half));
     };
@@ -1610,11 +1624,24 @@ var BNodeInternal = /** @class */ (function (_super) {
         }
         return false;
     };
+    /**
+     * Move children from `rhs` into this.
+     * `rhs` must be part of this tree, and be removed from it after this call
+     * (otherwise isShared for its children could be incorrect).
+     */
     BNodeInternal.prototype.mergeSibling = function (rhs, maxNodeSize) {
         // assert !this.isShared;
         var oldLength = this.keys.length;
         this.keys.push.apply(this.keys, rhs.keys);
-        this.children.push.apply(this.children, rhs.children);
+        var rhsChildren = rhs.children;
+        this.children.push.apply(this.children, rhsChildren);
+        if (rhs.isShared) {
+            // Because rhs might continue to be used in another tree since it is shared,
+            // this is adding a parent to its children instead of just changing what their parent is.
+            // Thus they need to be marked as shared.
+            for (var i = 0; i < rhsChildren.length; i++)
+                rhsChildren[i].isShared = true;
+        }
         // If our children are themselves almost empty due to a mass-delete,
         // they may need to be merged too (but only the oldLength-1 and its
         // right sibling should need this).

b+tree.test.ts

@@ -379,39 +379,121 @@ describe("cloning and sharing tests", () => {
     // This tests make a full 3 layer tree (height = 2), so use a small branching factor.
     const maxNodeSize = 4;
     const tree = new BTree<number, number>(
-    undefined,
-    simpleComparator,
-    maxNodeSize
+      undefined,
+      simpleComparator,
+      maxNodeSize
     );
     // Build a 3 layer complete tree, all values 0.
     for (
-    let index = 0;
-    index < maxNodeSize * maxNodeSize * maxNodeSize;
-    index++
+      let index = 0;
+      index < maxNodeSize * maxNodeSize * maxNodeSize;
+      index++
     ) {
-    tree.set(index, 0);
+      tree.set(index, 0);
     }
     // Leaf nodes don't count, so this is depth 2
     expect(tree.height).toBe(2);
-  
+
     // Edit the tree so it has a node in the second layer with exactly one child (key 0).
     tree.deleteRange(1, maxNodeSize * maxNodeSize, false);
     expect(tree.height).toBe(2);
-  
+
     // Make a clone that should never be mutated.
     const clone = tree.clone();
-  
+
     // Mutate the original tree in such a way that clone gets mutated due to incorrect is shared tracking.
     // Delete everything outside of the internal node with only one child, so its child becomes the new root.
     tree.deleteRange(maxNodeSize, Number.POSITIVE_INFINITY, false);
     expect(tree.height).toBe(0);
-  
+
     // Modify original
     tree.set(0, 1);
-  
+
     // Check that clone is not modified as well:
     expect(clone.get(0)).toBe(0);
   });
+
+  test("Regression test for greedyClone(true) not copying all nodes", () => {
+    const maxNodeSize = 4;
+    const tree = new BTree<number, number>(
+      undefined,
+      simpleComparator,
+      maxNodeSize
+    );
+    // Build a 3 layer tree.
+    for (
+      let index = 0;
+      index < maxNodeSize * maxNodeSize + 1;
+      index++
+    ) {
+      tree.set(index, 0);
+    }
+    // Leaf nodes don't count, so this is depth 2
+    expect(tree.height).toBe(2);
+    const clone = tree.greedyClone(true);
+
+    // The bug was that `force` was not passed down. This meant that non-shared nodes below the second layer would not be cloned.
+    // Thus we check that the third layer of this tree did get cloned.
+    // Since this depends on private APIs and types,
+    // and this package currently has no way to expose them to tests without exporting them from the package,
+    // do some private field access and any casts to make it work.
+    expect((clone['_root'] as any).children[0].children[0]).not.toBe((tree['_root'] as any).children[0].children[0]);
+  });
+
+  test("Regression test for mergeSibling setting isShared", () => {
+    // This tests make a 3 layer tree (height = 2), so use a small branching factor.
+    const maxNodeSize = 4;
+    const tree = new BTree<number, number>(
+      undefined,
+      simpleComparator,
+      maxNodeSize
+    );
+    // Build a 3 layer tree
+    const count = maxNodeSize * maxNodeSize * maxNodeSize;
+    for (
+      let index = 0;
+      index < count;
+      index++
+    ) {
+      tree.set(index, 0);
+    }
+    // Leaf nodes don't count, so this is depth 2
+    expect(tree.height).toBe(2);
+
+    // Delete most of the keys so merging interior nodes is possible, marking all nodes as shared.
+    for (
+      let index = 0;
+      index < count;
+      index++
+    ) {
+      if (index % 4 !== 0) {
+        tree.delete(index);
+      }
+    }
+
+    const deepClone = tree.greedyClone(true);
+    const cheapClone = tree.clone();
+
+    // These two clones should remain unchanged forever.
+    // The bug this is testing for resulted in the cheap clone getting modified:
+    // we will compare it against the deep clone to confirm it does not.
+
+    // Delete a bunch more nodes, causing merging.
+    for (
+      let index = 0;
+      index < count;
+      index++
+    ) {
+      if (index % 16 !== 0) {
+        tree.delete(index);
+      }
+    }
+
+    const different: number[] = [];
+    const onDiff = (k: number) => { different.push(k); }
+    deepClone.diffAgainst(cheapClone, onDiff, onDiff, onDiff);
+    expect(different).toEqual([]);
+  });
 });
 
 describe('B+ tree with fanout 32', testBTree.bind(null, 32));

b+tree.ts

@@ -1206,6 +1206,8 @@ class BNode<K,V> {
   // If this is an internal node, _keys[i] is the highest key in children[i].
   keys: K[];
   values: V[];
+  // True if this node might multiple parents (equivalently: could be in multiple b-trees).
+  // This means it must be cloned before being mutated to avoid changing an unrelated tree.
   isShared: true | undefined;
   get isLeaf() { return (this as any).children === undefined; }
 
@@ -1528,6 +1530,10 @@ class BNodeInternal<K,V> extends BNode<K,V> {
   // keys[i] caches the value of children[i].maxKey().
   children: BNode<K,V>[];
 
+  /** 
+   * This does not mark `children` as shared, so it is the responsibility of the caller
+   * to ensure that either children are marked shared, or it are not included in another tree.
+   */
   constructor(children: BNode<K,V>[], keys?: K[]) {
     if (!keys) {
       keys = [];
@@ -1550,7 +1556,7 @@ class BNodeInternal<K,V> extends BNode<K,V> {
       return this;
     var nu = new BNodeInternal<K,V>(this.children.slice(0), this.keys.slice(0));
     for (var i = 0; i < nu.children.length; i++)
-      nu.children[i] = nu.children[i].greedyClone();
+      nu.children[i] = nu.children[i].greedyClone(force);
     return nu;
   }
 
@@ -1666,12 +1672,22 @@ class BNodeInternal<K,V> extends BNode<K,V> {
     }
   }
 
+  /** 
+   * Inserts `child` at index `i`.
+   * This does not mark `child` as shared, so it is the responsibility of the caller
+   * to ensure that either child is marked shared, or it is not included in another tree.
+   */
   insert(i: index, child: BNode<K,V>) {
     this.children.splice(i, 0, child);
     this.keys.splice(i, 0, child.maxKey());
   }
 
+  /**
+   * Split this node.
+   * Modifies this to remove the second half of the items, returning a separate node containing them.
+   */
   splitOffRightSide() {
+    // assert !this.isShared;
     var half = this.children.length >> 1;
     return new BNodeInternal<K,V>(this.children.splice(half), this.keys.splice(half));
   }
@@ -1765,11 +1781,26 @@ class BNodeInternal<K,V> extends BNode<K,V> {
     return false;
   }
 
+  /**
+   * Move children from `rhs` into this.
+   * `rhs` must be part of this tree, and be removed from it after this call
+   * (otherwise isShared for its children could be incorrect).
+   */
   mergeSibling(rhs: BNode<K,V>, maxNodeSize: number) {
     // assert !this.isShared;
     var oldLength = this.keys.length;
     this.keys.push.apply(this.keys, rhs.keys);
-    this.children.push.apply(this.children, (rhs as any as BNodeInternal<K,V>).children);
+    const rhsChildren = (rhs as any as BNodeInternal<K,V>).children;
+    this.children.push.apply(this.children, rhsChildren);
+
+    if (rhs.isShared) {
+      // Because rhs might continue to be used in another tree since it is shared,
+      // this is adding a parent to its children instead of just changing what their parent is.
+      // Thus they need to be marked as shared.
+      for (var i = 0; i < rhsChildren.length; i++)
+        rhsChildren[i].isShared = true;
+    }
+
     // If our children are themselves almost empty due to a mass-delete,
     // they may need to be merged too (but only the oldLength-1 and its
     // right sibling should need this).