-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Backport 2.13.x Set/Map test and fix regression in HashMapBuilder #9229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport 2.13.x Set/Map test and fix regression in HashMapBuilder #9229
Conversation
@ignore tests that currently fail.
Skip immutable.HashMap
Make sure we update values both in `Map.+` and `Builder.++=`.
| if (merger eq null) { | ||
| if (this.value.asInstanceOf[AnyRef] eq value.asInstanceOf[AnyRef]) this | ||
| else new HashMap1(key, hash, value, kv) | ||
| else new HashMap1(this.key, hash, value, null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use kv if we can. I agree the previous was wrong, but how about
// preserve kv if we can. We know that kv matches key and value
// and we know if kv ne null that the value is eq, so if the key is also eq
// then we can retain the tuple
else new HashMap1(this.key, hash, value, if (this.key.asInstanceOf[AnyRef] eq key.asInstanceOf[AnyRef]) kv else null)`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it ought to be if (this.key eq key) kv else null.
| // 32-bit hash collision (rare, but not impossible) | ||
| new HashMapCollision1(hash, ListMap.empty.updated(this.key, this.value).updated(key, value)) | ||
| } else { | ||
| val nkv = if ((kv ne null) && (kv._1.asInstanceOf[AnyRef] eq this.key.asInstanceOf[AnyRef])) kv else null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change look wrong to me, and the original looked correct
This is the case where the hashes don't match (as I see it) so kv._1 is unrelated to this.key
As I see it this would cause a corruption in the tree, as the new value would be lost
so - conceptually
val previous = HashMap1(hash1, "a", "A")
val result = previous.updated("b", "B")
//which calls previous.updated(hash2, "b", "B", null, null)
this would give us a
HashTrieMap containing
previous and HashMap1(hash2, "a", "B")
and should be
previous and HashMap1(hash2, "b", "B")
I guess there isn't a test for that, or I am misreading the code
| elems = elems + elem | ||
| } else { | ||
| // assert(elems.size == 4) | ||
| elems.get(elem._1) match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change that you mane is better that the original ...but
It looks to me like the new and old code don't cope with a different value in the map for the same key, and convert to a hashMapBuilder when we could still use a Map4.
Also we could remove the creation of a Some via use of a getOrElse
something like
in object Map
private val flag = new AbstractFunction0[Any, Any] {
def apply() = flag
}
and here
val valueOrFlag = elems.getOrElse(elem._1, Map.flag).asInstanceOf[AnyRef]
if (valueOrFlag eq Map.flag) {
//its a new key
convertToHashMapBuilder()
hashMapBuilder += elem
} else if (valueOrFlag ne elem._2.asInstanceOf[AnyRef])
// its an existing key with a new value
elems = elems + elem
//else as key is == and value is eq, its no change
| * rules have explicit tests. | ||
| */ | ||
| @RunWith(classOf[JUnit4]) | ||
| class SetMapRulesTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
new tests - Looks like I have some reading and understanding to do!
2f82021 to
1409acb
Compare
Fix these properties:
```
checkAllValuesUpdated(gen, "++ (identical key)")(_.++(Seq((Value(1,1), Value(101,2)))).filter(_._1.id == 1))
checkAllValuesUpdated(gen, "++ (identical key)")(_.++(Seq((Value(1,1), Value(101,2)))).filter(_._1.id == 1))
```
Unskip immutable.HashMap
1409acb to
980e2e0
Compare
|
/rebuild |
980e2e0 to
b60fd93
Compare
…existing key instance
609487b to
5208677
Compare
mkeskells
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
In #8726, the optimization to HashMapBuilder fails to overwrite a value
if the existing value is
==to it. Map implementations should onlycall
==on keys and never on values.This PR does not go further to align other collections with 2.13.x
rules for preserving existing keys on updates.