[TreeDictionary] Adjust combining API

Spin off two separate combining behaviors for values that are present in both dictionaries, based on whether or not the values are equal. Before this change, unequal values were always merged, which isn’t necessarily what we want.
apple · Apr 11, 2023 · f825243 · f825243
1 parent 3698314
commit f825243
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 21 deletions.
diff --git a/Sources/HashTreeCollections/HashNode/_Node+Structural combining.swift b/Sources/HashTreeCollections/HashNode/_Node+Structural combining.swift
@@ -38,7 +38,7 @@ extension _HashNode {
     by strategy: some TreeDictionaryCombiningStrategy<Key, Value>
   ) throws -> Builder {
     if left.raw.storage === right.raw.storage {
-      switch strategy.commonBehavior {
+      switch strategy.equalValuesInBoth {
       case .include:
         return .node(level, left)
       case .discard:
@@ -245,7 +245,7 @@ extension _HashNode {
     left: _HashNode,
     by strategy: some TreeDictionaryCombiningStrategy<Key, Value>
   ) throws -> Builder {
-    switch strategy.removeBehavior {
+    switch strategy.valuesOnlyInFirst {
     case .include:
       return .node(level, left)
     case .discard:
@@ -263,7 +263,7 @@ extension _HashNode {
     right: _HashNode,
     by strategy: some TreeDictionaryCombiningStrategy<Key, Value>
   ) throws -> Builder {
-    switch strategy.addBehavior {
+    switch strategy.valuesOnlyInSecond {
     case .include:
       return .node(level, right)
     case .discard:
@@ -283,7 +283,7 @@ extension _HashNode {
     by strategy: some TreeDictionaryCombiningStrategy<Key, Value>
   ) throws -> Builder {
     let hash = _Hash(left.pointee.key)
-    switch strategy.addBehavior {
+    switch strategy.valuesOnlyInSecond {
     case .include:
       if var t = right.removing(level, left.pointee.key, hash) {
         guard let item = try strategy._processCommon(left, &t.removed) else {
@@ -347,7 +347,7 @@ extension _HashNode {
     by strategy: some TreeDictionaryCombiningStrategy<Key, Value>
   ) throws -> Builder {
     let hash = _Hash(right.pointee.key)
-    switch strategy.removeBehavior {
+    switch strategy.valuesOnlyInFirst {
     case .include:
       if var t = left.removing(level, right.pointee.key, hash) {
         guard let item = try strategy._processCommon(&t.removed, right) else {
@@ -534,7 +534,7 @@ extension _HashNode {
     by strategy: some TreeDictionaryCombiningStrategy<Key, Value>
   ) throws -> Builder {
     assert(left.isCollisionNode)
-    switch strategy.removeBehavior {
+    switch strategy.valuesOnlyInFirst {
     case .include:
       return .node(level, left)
     case .discard:
@@ -553,7 +553,7 @@ extension _HashNode {
     by strategy: some TreeDictionaryCombiningStrategy<Key, Value>
   ) throws -> Builder {
     assert(right.isCollisionNode)
-    switch strategy.addBehavior {
+    switch strategy.valuesOnlyInSecond {
     case .include:
       return .node(level, right)
     case .discard:
@@ -717,24 +717,31 @@ extension TreeDictionaryCombiningStrategy {
     _ item2: Element
   ) throws -> Element? {
     assert(item1.key == item2.key)
-    let b = commonBehavior
-    if
-      b == .merge
-        || !areEquivalentValues(item1.value, item2.value)
-    {
+    let equals = self.equalValuesInBoth
+    let unequals = self.unequalValuesInBoth
+
+    let result: CombiningBehavior
+
+    if equals == unequals || areEquivalentValues(item1.value, item2.value) {
+      result = equals
+    } else {
+      result = unequals
+    }
+    switch result {
+    case .include:
+      return item1
+    case .discard:
+      return nil
+    case .merge:
       let v = try merge(item1.key, item1.value, item2.value)
       guard let v = v else { return nil }
       return (item1.key, v)
     }
-    if b == .include {
-      return item1
-    }
-    return nil
   }
 
   @inlinable
   internal func _processRemove(_ item: Element) throws -> Element? {
-    switch addBehavior {
+    switch valuesOnlyInFirst {
     case .include:
       return item
     case .discard:
@@ -748,7 +755,7 @@ extension TreeDictionaryCombiningStrategy {
 
   @inlinable
   internal func _processAdd(_ item: Element) throws -> Element? {
-    switch removeBehavior {
+    switch valuesOnlyInSecond {
     case .include:
       return item
     case .discard:

diff --git a/Sources/HashTreeCollections/TreeDictionary/TreeDictionary+Combine.swift b/Sources/HashTreeCollections/TreeDictionary/TreeDictionary+Combine.swift
@@ -21,11 +21,13 @@ public protocol TreeDictionaryCombiningStrategy<Key, Value> {
   associatedtype Key: Hashable
   associatedtype Value
 
-  var commonBehavior: CombiningBehavior { get }
-  var addBehavior: CombiningBehavior { get }
-  var removeBehavior: CombiningBehavior { get }
+  var valuesOnlyInFirst: CombiningBehavior { get }
+  var valuesOnlyInSecond: CombiningBehavior { get }
+  var equalValuesInBoth: CombiningBehavior { get }
+  var unequalValuesInBoth: CombiningBehavior { get }
 
   func areEquivalentValues(_ a: Value, _ b: Value) -> Bool
+
   func merge(_ key: Key, _ value1: Value?, _ value2: Value?) throws -> Value?
 }
 

diff --git a/Tests/HashTreeCollectionsTests/TreeDictionary Smoke Tests.swift b/Tests/HashTreeCollectionsTests/TreeDictionary Smoke Tests.swift
@@ -625,4 +625,60 @@ final class TreeDictionarySmokeTests: CollectionTestCase {
 
     expectTrue(expectedPositions.isEmpty)
   }
+
+  func test_combine() {
+    var d1 = TreeDictionary<Int, String>(uniqueKeysWithValues: (0 ..< 10000).map { ($0, "1") })
+    d1[1] = "1"
+    var d2 = d1
+//    for i in 10 ..< 20 {
+//      d1[i] = nil
+//    }
+//    for i in 20 ..< 30 {
+//      d2[i] = nil
+//    }
+    for i in 40 ..< 50 {
+      d2[i] = "2"
+    }
+
+    class TestStrategy: TreeDictionaryCombiningStrategy {
+      typealias Key = Int
+      typealias Value = String
+
+      var _equalCounter = 0
+      var _mergeCounter = 0
+
+      var valuesOnlyInFirst: CombiningBehavior { .merge }
+      var valuesOnlyInSecond: CombiningBehavior { .merge }
+      var equalValuesInBoth: CombiningBehavior { .discard }
+      var unequalValuesInBoth: CombiningBehavior { .merge }
+
+      func areEquivalentValues(_ a: Value, _ b: Value) -> Bool {
+        _equalCounter += 1
+        return a == b
+      }
+
+      func merge(
+        _ key: Key, _ value1: Value?, _ value2: Value?
+      ) throws -> Value? {
+        _mergeCounter += 1
+
+        let s1 = value1 ?? "nil"
+        let s2 = value2 ?? "nil"
+        print("key: \(key), value1: \(s1), value2: \(s2)")
+
+        switch (value1, value2) {
+        case (nil, nil): return "00"
+        case (_?, nil): return "10"
+        case (nil, _?): return "01"
+        case (_?, _?): return "11"
+        }
+      }
+    }
+
+    let strategy = TestStrategy()
+    let d = try! d1.combining(d2, by: strategy)
+    print(d.map { ($0.key, $0.value) }.sorted(by: { $0.0 < $1.0 }))
+    print("Merge count: \(strategy._mergeCounter)")
+    print("isEqual count: \(strategy._equalCounter)")
+  }
 }
diff --git a/Utils/swift-collections.xcworkspace/xcshareddata/xcschemes/HashTreeCollections.xcscheme b/Utils/swift-collections.xcworkspace/xcshareddata/xcschemes/HashTreeCollections.xcscheme
@@ -20,6 +20,20 @@
                ReferencedContainer = "container:..">
             </BuildableReference>
          </BuildActionEntry>
+         <BuildActionEntry
+            buildForTesting = "YES"
+            buildForRunning = "YES"
+            buildForProfiling = "NO"
+            buildForArchiving = "NO"
+            buildForAnalyzing = "NO">
+            <BuildableReference
+               BuildableIdentifier = "primary"
+               BlueprintIdentifier = "HashTreeCollectionsTests"
+               BuildableName = "HashTreeCollectionsTests"
+               BlueprintName = "HashTreeCollectionsTests"
+               ReferencedContainer = "container:..">
+            </BuildableReference>
+         </BuildActionEntry>
       </BuildActionEntries>
    </BuildAction>
    <TestAction