From 4fe84ca7370f05059817fd333f72719d33fb4652 Mon Sep 17 00:00:00 2001 From: Gerard Murphy Date: Fri, 26 Apr 2024 15:20:49 +0100 Subject: [PATCH 1/4] Added test to reproduce bug described in https://github.com/scala/scala-collection-contrib/issues/239. Test fails, as expected... --- .../decorators/MapDecoratorTest.scala | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/src/test/scala/scala/collection/decorators/MapDecoratorTest.scala b/src/test/scala/scala/collection/decorators/MapDecoratorTest.scala index 8d8e474..d7e9be4 100644 --- a/src/test/scala/scala/collection/decorators/MapDecoratorTest.scala +++ b/src/test/scala/scala/collection/decorators/MapDecoratorTest.scala @@ -74,4 +74,92 @@ class MapDecoratorTest { // Assert.assertEquals(expected, zipped2) } + @Test + def mergingByKeyPerformsFullOuterJoin(): Unit = { + val arthur = "arthur.txt" + + val tyson = "tyson.txt" + + val sandra = "sandra.txt" + + val allKeys = Set(arthur, tyson, sandra) + + val sharedValue = 1 + + val ourChanges = Map( + ( + arthur, + sharedValue + ), + ( + tyson, + 2 + ) + ) + + { + // In this test case, none of the associated values collide across keys... + + val theirChanges = Map( + ( + arthur, + sharedValue + ), + ( + sandra, + 3 + ) + ) + + Assert.assertEquals("Expect the same keys to appear in the join taken either way around.", ourChanges.mergeByKey(theirChanges).keySet, theirChanges + .mergeByKey(ourChanges) + .keys) + + Assert.assertTrue("Expect the same associated values to appear in the join taken either way around, albeit swapped around and not necessarily in the same key order.", + ourChanges + .mergeByKey(theirChanges) + .values + .map(_.swap) + .toList + .sorted + .sameElements(theirChanges.mergeByKey(ourChanges).values.toList.sorted)) + + Assert.assertEquals("Expect all the keys to appear in an outer join.", ourChanges.mergeByKey(theirChanges).keys, allKeys) + + Assert.assertEquals("Expect all the keys to appear in an outer join.", theirChanges.mergeByKey(ourChanges).keys, allKeys) + } + + { + // In this test case, associated values collide across keys... + + val theirChangesRedux = Map( + ( + arthur, + sharedValue + ), + ( + sandra, + sharedValue + ) + ) + + Assert.assertEquals("Expect the same keys to appear in the join taken either way around.", ourChanges.mergeByKey(theirChangesRedux).keySet, theirChangesRedux + .mergeByKey(ourChanges) + .keys) + + Assert.assertTrue("Expect the same associated values to appear in the join taken either way around, albeit swapped around and not necessarily in the same key order.", + ourChanges + .mergeByKey(theirChangesRedux) + .values + .map(_.swap) + .toList + .sorted + .sameElements(theirChangesRedux.mergeByKey(ourChanges).values.toList.sorted)) + + Assert.assertEquals("Expect all the keys to appear in an outer join.", ourChanges.mergeByKey(theirChangesRedux).keys, allKeys) + + Assert.assertEquals("Expect all the keys to appear in an outer join.", theirChangesRedux.mergeByKey(ourChanges).keys, allKeys) + } + } + } From 8c8d67bd323f7a9bf2b786b5b55c3349fa52f7f8 Mon Sep 17 00:00:00 2001 From: Gerard Murphy Date: Fri, 26 Apr 2024 15:30:56 +0100 Subject: [PATCH 2/4] Fix outstanding test failure. --- .../scala/scala/collection/decorators/MapDecorator.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/scala/scala/collection/decorators/MapDecorator.scala b/src/main/scala/scala/collection/decorators/MapDecorator.scala index bb7a90e..e3c7aa3 100644 --- a/src/main/scala/scala/collection/decorators/MapDecorator.scala +++ b/src/main/scala/scala/collection/decorators/MapDecorator.scala @@ -59,16 +59,16 @@ class MapDecorator[C, M <: IsMap[C]](coll: C)(implicit val map: M) { */ def mergeByKeyWith[W, X, That](other: Map[map.K, W])(f: PartialFunction[(Option[map.V], Option[W]), X])(implicit bf: BuildFrom[C, (map.K, X), That]): That = { val b = bf.newBuilder(coll) - val traversed = mutable.Set.empty[W] + val traversed = mutable.Set.empty[map.K] val pf = f.lift for { (k, v) <- map(coll) - x <- pf(other.get(k).fold[(Option[map.V], Option[W])]((Some(v), None)){ w => traversed += w; (Some(v), Some(w)) }) + x <- pf(other.get(k).fold[(Option[map.V], Option[W])]((Some(v), None)){ w => traversed += k; (Some(v), Some(w)) }) } { b += k -> x } for { - (k, w) <- other if !traversed(w) + (k, w) <- other if !traversed(k) x <- pf((None, Some(w))) } { b += k -> x From ab3637e0cc921a2e13d4bfcbced12658bc4fe748 Mon Sep 17 00:00:00 2001 From: Gerard Murphy Date: Mon, 29 Apr 2024 08:46:18 +0100 Subject: [PATCH 3/4] Conform to existing code style and use `locally` to emphasize blocks used to control scope. --- .../scala/scala/collection/decorators/MapDecoratorTest.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/scala/scala/collection/decorators/MapDecoratorTest.scala b/src/test/scala/scala/collection/decorators/MapDecoratorTest.scala index d7e9be4..ef7fc61 100644 --- a/src/test/scala/scala/collection/decorators/MapDecoratorTest.scala +++ b/src/test/scala/scala/collection/decorators/MapDecoratorTest.scala @@ -97,7 +97,7 @@ class MapDecoratorTest { ) ) - { + locally { // In this test case, none of the associated values collide across keys... val theirChanges = Map( @@ -129,7 +129,7 @@ class MapDecoratorTest { Assert.assertEquals("Expect all the keys to appear in an outer join.", theirChanges.mergeByKey(ourChanges).keys, allKeys) } - { + locally { // In this test case, associated values collide across keys... val theirChangesRedux = Map( From 350cd7538a81401ff0f3a7c8359d8205b5d64447 Mon Sep 17 00:00:00 2001 From: Gerard Murphy Date: Sat, 25 May 2024 10:59:05 +0100 Subject: [PATCH 4/4] Address code review comment (https://github.com/scala/scala-collection-contrib/pull/244#discussion_r1614326507): be consistent about using `.keySet` to express a map's keys. --- .../collection/decorators/MapDecoratorTest.scala | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/test/scala/scala/collection/decorators/MapDecoratorTest.scala b/src/test/scala/scala/collection/decorators/MapDecoratorTest.scala index ef7fc61..3aae2fa 100644 --- a/src/test/scala/scala/collection/decorators/MapDecoratorTest.scala +++ b/src/test/scala/scala/collection/decorators/MapDecoratorTest.scala @@ -113,7 +113,7 @@ class MapDecoratorTest { Assert.assertEquals("Expect the same keys to appear in the join taken either way around.", ourChanges.mergeByKey(theirChanges).keySet, theirChanges .mergeByKey(ourChanges) - .keys) + .keySet) Assert.assertTrue("Expect the same associated values to appear in the join taken either way around, albeit swapped around and not necessarily in the same key order.", ourChanges @@ -124,9 +124,9 @@ class MapDecoratorTest { .sorted .sameElements(theirChanges.mergeByKey(ourChanges).values.toList.sorted)) - Assert.assertEquals("Expect all the keys to appear in an outer join.", ourChanges.mergeByKey(theirChanges).keys, allKeys) + Assert.assertEquals("Expect all the keys to appear in an outer join.", ourChanges.mergeByKey(theirChanges).keySet, allKeys) - Assert.assertEquals("Expect all the keys to appear in an outer join.", theirChanges.mergeByKey(ourChanges).keys, allKeys) + Assert.assertEquals("Expect all the keys to appear in an outer join.", theirChanges.mergeByKey(ourChanges).keySet, allKeys) } locally { @@ -145,7 +145,7 @@ class MapDecoratorTest { Assert.assertEquals("Expect the same keys to appear in the join taken either way around.", ourChanges.mergeByKey(theirChangesRedux).keySet, theirChangesRedux .mergeByKey(ourChanges) - .keys) + .keySet) Assert.assertTrue("Expect the same associated values to appear in the join taken either way around, albeit swapped around and not necessarily in the same key order.", ourChanges @@ -156,9 +156,9 @@ class MapDecoratorTest { .sorted .sameElements(theirChangesRedux.mergeByKey(ourChanges).values.toList.sorted)) - Assert.assertEquals("Expect all the keys to appear in an outer join.", ourChanges.mergeByKey(theirChangesRedux).keys, allKeys) + Assert.assertEquals("Expect all the keys to appear in an outer join.", ourChanges.mergeByKey(theirChangesRedux).keySet, allKeys) - Assert.assertEquals("Expect all the keys to appear in an outer join.", theirChangesRedux.mergeByKey(ourChanges).keys, allKeys) + Assert.assertEquals("Expect all the keys to appear in an outer join.", theirChangesRedux.mergeByKey(ourChanges).keySet, allKeys) } }