From e52cf0cc1a165c54543ef55f1f4b164cbff1e1bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kasper=20Overg=C3=A5rd=20Nielsen?= Date: Sun, 1 Sep 2024 19:05:41 +0200 Subject: [PATCH 1/2] Expose bug: Set.add should never update existing item --- test/set_test.dart | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/set_test.dart b/test/set_test.dart index 31c4114..12cc86d 100644 --- a/test/set_test.dart +++ b/test/set_test.dart @@ -150,5 +150,11 @@ void main() { expect(it.current, 5); expect(it.moveNext(), isFalse); }); + + test("add don't update", () { + final s = TreapSet.of([(1, 2)], (a, b) => a.$1 - b.$1); + expect(s.add((1, 3)), isFalse); + expect(s.lookup((1, 0)), (1, 2)); + }); }); } From 26b2f0e48b245a893a3bfe2dd5bcf88322905327 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kasper=20Overg=C3=A5rd=20Nielsen?= Date: Sun, 1 Sep 2024 19:06:55 +0200 Subject: [PATCH 2/2] Fix++. Prep for mutable treaps --- lib/src/node.dart | 63 ++++++++++++++++++++++++++++----------- lib/src/treap_base.dart | 66 +++++++++++++++++++++++++---------------- lib/src/treap_map.dart | 2 +- lib/src/treap_set.dart | 2 +- test/treap_test.dart | 43 +++++++++++++++++---------- 5 files changed, 114 insertions(+), 62 deletions(-) diff --git a/lib/src/node.dart b/lib/src/node.dart index f984dc6..fbe6c69 100644 --- a/lib/src/node.dart +++ b/lib/src/node.dart @@ -2,27 +2,23 @@ // SPDX-License-Identifier: BSD-3-Clause import 'split.dart'; -class Node { +sealed class Node { final T item; final int priority; - final int _size; - final Node? left, right; + int get _size; // size is an extension method on Node? + Node? get left; + Node? get right; - Node(this.item, this.priority, {this.left, this.right}) - : _size = 1 + left.size + right.size { + Node(this.item, this.priority) { checkInvariant(); } - /// Replace the left child with [left]. - Node withLeft(Node? left) => - Node(item, priority, left: left, right: right); + Node withLeft(Node? left); - /// Replace the right child with [right]. - Node withRight(Node? right) => - Node(item, priority, left: left, right: right); + Node withRight(Node? right); - /// Create a copy of this node without children. - Node withoutChildren() => Node(item, priority); + /// x + Node withoutChildren(); /// The minimum item in the treap. T get min => left == null ? item : left!.min; @@ -42,6 +38,31 @@ class Node { } } +class PersistentNode extends Node { + @override + final PersistentNode? left, right; + @override + final int _size; + + PersistentNode(super.item, super.priority, {this.left, this.right}) + : _size = 1 + left.size + right.size; + + /// Create a copy with the left child set to [left]. + @override + PersistentNode withLeft(covariant PersistentNode? left) => + PersistentNode(item, priority, left: left, right: right); + + /// Create a copy with the right child set to [right]. + @override + PersistentNode withRight(covariant PersistentNode? right) => + PersistentNode(item, priority, left: left, right: right); + + /// Create a copy of this node without children. + /// @super + @override + PersistentNode withoutChildren() => PersistentNode(item, priority); +} + extension NodeEx on Node? { int get size => this?._size ?? 0; @@ -57,7 +78,8 @@ extension NodeEx on Node? { final s = self.right.split(pivot, compare); return s.withLow(self.withRight(s.low)); } - return Split((low: self.left, middle: self, high: self.right)) // order == 0 + // order == 0 + return Split((low: self.left, middle: self, high: self.right)) ..checkInvariant(compare); } @@ -73,11 +95,16 @@ extension NodeEx on Node? { return self ?? other; // zero or one } - Node add(Node child, Comparator compare) => - split(child.item, compare).withMiddle(child).join()!; + ({Node root, T? old}) upsert(Node child, Comparator compare) { + final item = child.item; + final s = split(item, compare); + return (root: s.withMiddle(child).join()!, old: s.middle?.item); + } - Node? erase(T dead, Comparator compare) => - split(dead, compare).withMiddle(null).join(); + ({Node? root, T? old}) erase(T dead, Comparator compare) { + final s = split(dead, compare); + return (root: s.withMiddle(null).join(), old: s.middle?.item); + } Node? union(Node? other, Comparator compare) { final self = this; // for type promotion diff --git a/lib/src/treap_base.dart b/lib/src/treap_base.dart index 3929c46..1e81050 100644 --- a/lib/src/treap_base.dart +++ b/lib/src/treap_base.dart @@ -41,6 +41,8 @@ class Treap { const Treap([Comparator? compare]) : this._(null, compare ?? Comparable.compare as Comparator); + Treap _new(Node? root) => root == _root ? this : Treap._(root, compare); + /// Build a treap containing the [items]. /// /// Constructs the treap by folding [add] over the [items], adding each one @@ -49,28 +51,42 @@ class Treap { /// This method is `O(N log(N))` in complexity. An `O(N)` algorithm exists if the /// items are sorted. However, this works in all cases. factory Treap.of(Iterable items, [Comparator? comparator]) => - items.fold(Treap(comparator), (acc, i) => acc.add(i)); + Treap(comparator).addAll(items); /// Adds an [item]. /// - /// Creates a new node with the [item] and a random priority. Returns a new treap - /// with the added node. - Treap add(T item) => Treap._( - _root.add( - Node(item, _rnd.nextInt(1 << 32)), - compare, - ), - compare); + /// If the [item] is already present in the treap, the original treap is returned. + /// Otherwise, a new treap is returned with the item added. + Treap add(T item) { + final (:root, :old) = _root.upsert(_createNode(item), compare); + return old == null ? _new(root) : this; + } + + /// Adds or updates an [item]. + /// + /// Returns a new treap, with [item] either added or updated. + Treap addOrUpdate(T item) { + final (:root, :old) = _root.upsert(_createNode(item), compare); + return _new(root); + } + + Node _createNode(T item) => PersistentNode(item, _rnd.nextInt(1 << 32)); /// Adds a range of [items]. /// - /// Returns a new treap with the added [items]. - Treap addRange(Iterable items) => union(Treap.of(items, compare)); + /// Returns a new treap with the added [items]. If all the [items] are already + /// present, the original treap is returned. + Treap addAll(Iterable items) => + items.fold(this, (acc, i) => acc.add(i)); /// Erases an [item] from the treap, if it exists. /// - /// Returns a new treap without the erased [item]. - Treap erase(T item) => Treap._(_root.erase(item, compare), compare); + /// Returns a new treap without the erased [item]. If the [item] was not present, + /// the original treap is returned. + Treap erase(T item) { + final (:root, :old) = _root.erase(item, compare); + return old != null ? _new(root) : this; + } /// Whether this treap is empty. bool get isEmpty => _root == null; @@ -85,8 +101,6 @@ class Treap { T? find(T item) => _root.find(item, compare)?.item; /// Whether an[item] exists in this treap. - /// - /// Returns `true` if `find` re bool has(T item) => find(item) != null; /// The rank of an [item]. @@ -132,9 +146,11 @@ class Treap { /// Returns a new treap with the first [n] items. /// /// Returns the original treap, if [n] is greater than the [size] of this treap. - Treap take(int n) => n < size - ? Treap._(_root.split(_root.select(n).item, compare).low, compare) - : this; + Treap take(int n) { + if (n == 0) return Treap(compare); // empty; + if (n >= size) return this; + return _new(_root.split(_root.select(n).item, compare).low); + } /// Skips the first [n] items and returns a new treap with the remaining items. /// @@ -143,23 +159,19 @@ class Treap { Treap skip(int n) { if (n == 0) return this; if (n >= size) return Treap(compare); // empty - return Treap._( - _root.split(_root.select(n - 1).item, compare).high, - compare, - ); + return _new(_root.split(_root.select(n - 1).item, compare).high); } /// Returns a new treap that is the union of this treap and the [other] treap. - Treap union(Treap other) => - Treap._(_root.union(other._root, compare), compare); + Treap union(Treap other) => _new(_root.union(other._root, compare)); /// Returns a new treap that is the intersection of this treap and the [other] treap. Treap intersection(Treap other) => - Treap._(_root.intersection(other._root, compare), compare); + _new(_root.intersection(other._root, compare)); /// Returns a new treap that is the difference of this treap minus the [other] treap. Treap difference(Treap other) => - Treap._(_root.difference(other._root, compare), compare); + _new(_root.difference(other._root, compare)); /// Operator overload for [add]ing an [item] to the treap. Treap operator +(T item) => add(item); @@ -176,3 +188,5 @@ class Treap { /// Operator overload for [select]ing an item in the treap by its [index]. T operator [](int index) => select(index); } + +class MutableTreap {} diff --git a/lib/src/treap_map.dart b/lib/src/treap_map.dart index 93cd22b..617315c 100644 --- a/lib/src/treap_map.dart +++ b/lib/src/treap_map.dart @@ -16,7 +16,7 @@ class TreapMap extends MapBase { @override void operator []=(K key, V value) => - _root = _root.add((key: key, value: value)); + _root = _root.addOrUpdate((key: key, value: value)); @override void clear() => _root = Treap(_root.compare); diff --git a/lib/src/treap_set.dart b/lib/src/treap_set.dart index 25ca426..7d18d12 100644 --- a/lib/src/treap_set.dart +++ b/lib/src/treap_set.dart @@ -22,7 +22,7 @@ class TreapSet extends SetBase { } @override - void addAll(Iterable elements) => _root = _root.addRange(elements); + void addAll(Iterable elements) => _root = _root.addAll(elements); @override bool contains(covariant T element) => lookup(element) != null; diff --git a/test/treap_test.dart b/test/treap_test.dart index f0f249a..3cb6b97 100644 --- a/test/treap_test.dart +++ b/test/treap_test.dart @@ -12,7 +12,9 @@ void main() { test('add, erase, build', () { final x = Treap() + 1; final y = x.add(1); - expect(x, isNot(y)); // add gives new Treap, even for existing items + final z = x.addOrUpdate(1); + expect(x, y); + expect(x, isNot(z)); // Be aware, that chaining with .. operator probably don't do what you want x @@ -34,6 +36,11 @@ void main() { expect(t.isEmpty, isTrue); expect(t.values, []); }); + + test('duplicates', () { + final t = Treap.of([1, 1, 1, 1, 1]); + expect(t.values, [1]); + }); }); group('retrieval', () { @@ -165,15 +172,17 @@ void main() { group('Node', () { final rnd = Random(42 ^ 42); - Node node(num value) => Node(value, rnd.nextInt(1 << 32)); + Node node(num value) => + PersistentNode(value, rnd.nextInt(1 << 32)); test('add, find, erase, inOrder', () { final first = node(1); - final again = first.add(node(1), Comparable.compare); - final second = first.add(node(2), Comparable.compare); - final third = second.add(node(3), Comparable.compare); - final another = third.add(node(3), Comparable.compare); - final forth = third.add(node(0), Comparable.compare); + final (root: again, old: _) = first.upsert(node(1), Comparable.compare); + final (root: second, old: _) = first.upsert(node(2), Comparable.compare); + final (root: third, old: _) = second.upsert(node(3), Comparable.compare); + final (root: another, old: _) = + second.upsert(node(3), Comparable.compare); + final (root: forth, old: _) = third.upsert(node(0), Comparable.compare); expect(first.inOrder().map((n) => n.item), [1]); expect(again.inOrder().map((n) => n.item), [1]); @@ -190,18 +199,19 @@ void main() { expect(second.find(1, Comparable.compare), isNotNull); expect(second.find(2, Comparable.compare), isNotNull); - final fifth = forth.erase(0, Comparable.compare); + final (root: fifth, old: _) = forth.erase(0, Comparable.compare); expect(fifth!.inOrder().map((n) => n.item), [1, 2, 3]); expect(identical(third, fifth), false); - expect(forth.erase(2, Comparable.compare)!.inOrder().map((n) => n.item), - [0, 1, 3]); + expect( + forth.erase(2, Comparable.compare).root!.inOrder().map((n) => n.item), + [0, 1, 3], + ); }); test('rank, select', () { - final top = [1, 2, 3, 4, 5, 6, 7, 8, 9] - .reversed - .fold(node(0), (acc, i) => acc.add(node(i), Comparable.compare)); + final top = [1, 2, 3, 4, 5, 6, 7, 8, 9].reversed.fold( + node(0), (acc, i) => acc.upsert(node(i), Comparable.compare).root); expect(top.inOrder().map((n) => n.item), [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]); expect(top.inOrder().map((n) => top.rank(n.item, Comparable.compare)), [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]); @@ -217,9 +227,10 @@ void main() { // Deterministic shaped treap despite shuffle final items = [5, 6, 3, 9, 1, 8, 2, 4, 7]; // evil order print(items); - final top = items.fold( - Node(0, 0), // will have same priority (5,0) - (acc, i) => acc.add(Node(i, 5 - i), Comparable.compare), + final top = items.fold>( + PersistentNode(0, 0), // will have same priority (5,0) + (acc, i) => + acc.upsert(PersistentNode(i, 5 - i), Comparable.compare).root, ); expect( top.inOrder().map((n) => n.item),