<div><br></div> I see what you are saying and I think it works. Although, it will be heavier, it would cover scenarios not covered by the current implementation.<div><br></div><div> My suggestion is if you can, please create a test case for the problem, change the collectSet accumulate function to do what you propose and send a pull request. I will review it and apply. I think in this case, making user's life easier by not requiring immutability might trump performance, but we need to evaluate some tests just in case.</div>
<div><br></div><div> Edson<br><br><div class="gmail_quote">On Tue, Feb 28, 2012 at 1:01 PM, SirMungus <span dir="ltr"><<a href="mailto:Patrick_Rusk@ssga.com">Patrick_Rusk@ssga.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Edson Tirelli-4 wrote<br>
<div class="im">><br>
> As you noticed already, the function uses the resulting object as a key<br>
> in the map because the goal is indeed to match equals() objects and filter<br>
> them to return a set, no duplicates. Of course, because in case of (b) the<br>
> hashcode changes, java no longer finds the key to remove it from the map.<br>
><br>
> The problem of using a straight identity map is that it will no longer<br>
> collect sets of equals() object, but sets of identical objects, what is<br>
> obviously not the intent of the function. For instance, in case (a), if<br>
> two<br>
> objects have the same first and last name, there will be 2 instances of<br>
> the<br>
> resulting object created, even if they are equals(). That will break the<br>
> function.<br>
><br>
</div>Thanks for the attention you've been able to give this, Edson.<br>
The solution I've proposed doesn't break the equals() contact of the<br>
resulting set; it just applies it at getResult() time, rather than during<br>
accumulate()/reverse(). So, the main question would be whether the creation<br>
of a new HashSet in getResult() is too costly, which I've discussed<br>
elsewhere on this thread.<br>
Are there existing stress tests that could test that empirically?<br>
<br>
--<br>
View this message in context: <a href="http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFunction-should-probably-use-IdentityHashMap-internally-tp3774079p3784789.html" target="_blank">http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFunction-should-probably-use-IdentityHashMap-internally-tp3774079p3784789.html</a><br>
<div class="HOEnZb"><div class="h5">Sent from the Drools: Developer (committer) mailing list mailing list archive at Nabble.com.<br>
_______________________________________________<br>
rules-dev mailing list<br>
<a href="mailto:rules-dev@lists.jboss.org">rules-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/rules-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/rules-dev</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br> Edson Tirelli<br> JBoss Drools Core Development<br> JBoss by Red Hat @ <a href="http://www.jboss.com">www.jboss.com</a><br>
</div>