[First post. I apologize in advance if I'm following the wrong protocol to report a bug.] I had a rule using the collectSet() accumulator that was giving me an NPE at line 123 of CollectSetAccumulateFunction.java, highlighted below: public void reverse(Serializable context, Object value) throws Exception { CollectListData data = (CollectListData) context; CollectListData.MutableInt counter = data.map.get( value ); */if( (--counter.value) == 0 ) {/* data.map.remove( value ); } } When looking in the debugger, I could see that the "value" passed in was indeed present within the "data.map". However, the "data.map.get(value)" was returning null. I thankfully realized this was because the object had changed, which changed its hashCode(). The stored hashCode() in the HashMap was different from its current hashCode(). I solved my problem by reimplementing my hashCode() as System.identityHashCode(this) and my equals as "this == other". However, I suspect the CollectSetAccumulateFunction should really use an IdentityHashMap in this situation. It's a really bad idea to allow mutable objects as keys in HashMap, and there is nothing about the collectSet() accumulator function that suggests that it should only collect immutable objects. Thanks. P.S. I'm loving Drools. Great tool. -- View this message in context: http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun... Sent from the Drools: Developer (committer) mailing list mailing list archive at Nabble.com. _______________________________________________ rules-dev mailing list rules-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/rules-dev

There are two types of facts - stated and justified. As explained in the TMS section. Justified facts are ALWAYS equality based, as it's the nature of TMS. The other part of engine for stated facts is customizable and may work in either equality mode or identity mode. Identity mode is default, as it's the most common way java developers expect to work, knowledge based engineers may prefer eqality mode. We do discuss this in the maual. If it's clear, then please do help beef up the docs on this. Fork the docs and submit a pull request, see: http://www.athico.com/Getting_Involved/gettinginvolved.html Mark On 25/02/2012 19:41, SirMungus wrote: > By the way, while checking into various scenarios prompted by Wolfgang's > response, I came up with a very interesting test program: > > rule "Initialize" > salience 100 > when > eval(true) > then > insert("one"); > insert(new String("one")); > insertLogical("two"); > insertLogical(new String("two")); > end > > rule "Find strings" > salience 50 > when > $s : String() > then > System.out.println("I found string: " + $s); > end > > rule "Gather strings" > salience 50 > when > accumulate( > $s : String(), > $sSet : collectSet( $s ), > $sList : collectList( $s ) ) > then > System.out.println("Here is the set of all strings: " + $sSet); > System.out.println("Here is the list of all strings: " +

> end > > What would you expect that to output? How many "I found string: xxx"

> would there be? How many entries in the "set of all strings"? How many > entries in the "list of all strings"? > > For my part, I figured there would be four "I found string: xxx" lines, > two > entries in the set, and four entries in the list. > > I was very surprised to find the following output: > > I found string: two > I found string: one > I found string: one > Here is the set of all strings: [two, one] > Here is the list of all strings: [one, one, two] > > This strongly suggests that "insert" objects are held in an > identity-preserving structure, but that "insertLogical" objects are held > in > an equals()-based structure. > > I am glad for the identity-based handling of "insert" objects, especially > considering that all of my inserted objects come from "outside" the > engine. > I would hate to see one of them fail to be considered by the rule engine. > > I can almost see some usefulness for the equals() behavior of > insertLogical > objects, but far more potential problems. > > I think it's slightly dangerous that these behaviors differ and that it

> not, AFAIK, documented in a way that is clear to users of Drools. > > -- > View this message in context: >

> Sent from the Drools: Developer (committer) mailing list mailing list > archive at Nabble.com. > _______________________________________________ > rules-dev mailing list > rules-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/rules-dev _______________________________________________ rules-dev mailing list rules-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/rules-dev

I'm not really trying to make an abstract point that all maps should be identity maps. The problem I encountered was very subtle (it took me a day to track it down), and it's difficult to see how to resolve it without resorting to identity semantics somewhere in the accumulator implementation. Consider a very basic rule using an accumulator (and a class called Fooble with an isRelevant() accessor): rule "Test sets" when accumulate( $o : Fooble(relevant) $oSet : collectSet( $o ) ) then ... end The general principle behind the rules engine is that whenever something known to the engine changes such that it affects one of the conditions in a rule, the rule will reevaluate. So, if there were three "relevant" Foobles, the set would contain them (possibly, I now confess, holding less than three objects, if any of them are equals() to another). If later, there were only two relevant Foobles, one would expect the set to contain those two. The great optimization of the rule engine is that the reevaluations are done for only those portions of the underlying RETE structure that are affected. The accumulator functions support this by the fact that they maintain state and have accumulate() and reverse() functions. But, because of the particular choices made in the implementation of CollectSetAccumulateFunction(), it failed, in my case, to reverse() an object that was previously relevant, because that object had a different hashCode()/equals() when the time came to reverse. And the failure was such that an NPE was involved, and ignoring the NPE (or ignoring the fact that the reversed object wasn't found in the set) would have caused the set to still contain the object it was supposed to remove. On the other hand, if an IdentityHashMap were used in place of the HashMap, it would have been able to reverse() the object, regardless of its equals(). There would be one other change required, mostly likely. The getResults() would probably have to change from... public Object getResult(Serializable context) throws Exception { CollectListData data = (CollectListData) context; return Collections.unmodifiableSet( data.map.keySet() ); } ...to... public Object getResult(Serializable context) throws Exception { CollectListData data = (CollectListData) context; return Collections.unmodifiableSet( new HashSet(data.map.keySet()) ); } That would take the identity-based keySet (being the accumulated objects) and collect them into an equals()-based Set. That would be, unfortunately, an O(N) operation inserted into a function that is otherwise an O(1). But, it would preserve the current functionality of collectSet() without failing in the case of objects that are not suitable as keys. Alternatively, the set returned by collectSet() could changed to be an identity-based Set, which would need to be called out in release notes, naturally. -- View this message in context: http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun... Sent from the Drools: Developer (committer) mailing list mailing list archive at Nabble.com. _______________________________________________ rules-dev mailing list rules-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/rules-dev

Mark Proctor wrote > > accumulate supports custom functions, just add your own version of > collectSet that uses IdentityMap. > I could do that, but please let me take one more stab, just in case I haven't been clear enough. 1) I am not suggesting that collectSet() return an identity-based set. I am content to have it return a normal set, though I had early confusion on that point. 2) The discussion above about stated vs. justified facts turns out to be a distraction to the main point of the bug that I believe to be in CollectSetAccumulateFunction. My failure to understand the distinction is not germane to the bug. 3) I am not disagreeing with the value behind implementing equals() to only take into account invariant fields, though I am stating that goes beyond the explicity documentation of equals()/hashCode() and is explicitly *not* followed by many classes in the JRE (most notably, collections). There is nothing about the purpose of collectSet() that logically suggests that it should only work with objects that have invariant equals()/hashCode() functions. If five objects match a rule, they should be able to be collected into a set (which may end up with fewer than five objects). If things change such that only three facts match the rule, they should still be able to be collected into a set. There's no compelling reason why that should only apply to objects with invariant equals(). And it seems to me that you rarely want to leave an NPE in code, rather than catching it and throwing a more intelligible exception (e.g., "Object to be removed from collectSet() was not found"). If the internal maintenance of state in CollectSetAccumulateFunction used identity semantics and applied the equality semantics only when you asked it for its results, it would maintain the same external behavior (returning a "normal" equality-based set) without failing to work for objects with variant equals. If you Google around for the proper implementation of equals(), you will find many serious sites that discuss it at length, yet provide examples like: public boolean equals(Object aThat){ if ( this == aThat ) return true; if ( !(aThat instanceof Car) ) return false; Car that = (Car)aThat; return EqualsUtil.areEqual(this.fName, that.fName) && EqualsUtil.areEqual(this.fNumDoors, that.fNumDoors) && EqualsUtil.areEqual(this.fGasMileage, that.fGasMileage) && EqualsUtil.areEqual(this.fColor, that.fColor) && Arrays.equals(this.fMaintenanceChecks, that.fMaintenanceChecks); //array! } Ironically, that is explicitly against Wolfgang's suggestion above! -- View this message in context: http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun... Sent from the Drools: Developer (committer) mailing list mailing list archive at Nabble.com. _______________________________________________ rules-dev mailing list rules-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/rules-dev

The getResults() would probably have to change from... public Object getResult(Serializable context) throws Exception { CollectListData data = (CollectListData) context; return Collections.unmodifiableSet( data.map.keySet() ); } ...to... public Object getResult(Serializable context) throws Exception { CollectListData data = (CollectListData) context; return Collections.unmodifiableSet( new HashSet(data.map.keySet()) ); } The proposed getResult() results in a full HashSet copy for each change of Rete, that performance hit would be unbearable for most people. I do agree that there is some merit to what you are asking, but we can't give that perf hit to everyone. It would be better to maintain two collections, one identity and one set based. And some how only change in set what was relevant to the change in identity, you might need to build a custom hash table for this - see AbstractHashTable in drools which can easily be extended for custom keys and custom add/remove methods. Mark On 27/02/2012 16:53, SirMungus wrote: > Mark Proctor wrote >> accumulate supports custom functions, just add your own version of >> collectSet that uses IdentityMap. >> > I could do that, but please let me take one more stab, just in case I > haven't been clear enough. > > 1) I am not suggesting that collectSet() return an identity-based > set. I am > content to have it return a normal set, though I had early confusion > on that > point. > > 2) The discussion above about stated vs. justified facts turns out to > be a > distraction to the main point of the bug that I believe to be in > CollectSetAccumulateFunction. My failure to understand the > distinction is > not germane to the bug. > > 3) I am not disagreeing with the value behind implementing equals() > to only > take into account invariant fields, though I am stating that goes > beyond the > explicity documentation of equals()/hashCode() and is explicitly *not* > followed by many classes in the JRE (most notably, collections). > > There is nothing about the purpose of collectSet() that logically > suggests > that it should only work with objects that have invariant > equals()/hashCode() functions. If five objects match a rule, they > should be > able to be collected into a set (which may end up with fewer than five > objects). If things change such that only three facts match the rule, > they > should still be able to be collected into a set. There's no compelling > reason why that should only apply to objects with invariant equals(). > And it > seems to me that you rarely want to leave an NPE in code, rather than > catching it and throwing a more intelligible exception (e.g., "Object > to be > removed from collectSet() was not found"). > > If the internal maintenance of state in CollectSetAccumulateFunction > used > identity semantics and applied the equality semantics only when you > asked it > for its results, it would maintain the same external behavior > (returning a > "normal" equality-based set) without failing to work for objects with > variant equals. > > If you Google around for the proper implementation of equals(), you will > find many serious sites that discuss it at length, yet provide examples > like: > > public boolean equals(Object aThat){ > if ( this == aThat ) return true; > if ( !(aThat instanceof Car) ) return false; > Car that = (Car)aThat; > return > EqualsUtil.areEqual(this.fName, that.fName)&& > EqualsUtil.areEqual(this.fNumDoors, that.fNumDoors)&& > EqualsUtil.areEqual(this.fGasMileage, that.fGasMileage)&& > EqualsUtil.areEqual(this.fColor, that.fColor)&& > Arrays.equals(this.fMaintenanceChecks, that.fMaintenanceChecks); > //array! > } > > Ironically, that is explicitly against Wolfgang's suggestion above! > > > -- > View this message in context: > http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun... > Sent from the Drools: Developer (committer) mailing list mailing list > archive at Nabble.com. > _______________________________________________ > rules-dev mailing list > rules-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/rules-dev

How do you find the List for removal by identity when the key has already been changed? -W On 28/02/2012, Mark Proctor <mproctor(a)codehaus.org&gt; wrote: > Actually this can be done without two collections. > > Just use a HashMap and mainta a List of the "clashed" identities. To > operate on the actualy identity, such as remove. Lookup the List, via > normal map.get( key ) then iterate the list to remove the identity. If > the list is empty, remove the key. reverse removes the entity, makes it > change, and then adds it back in. Adding is the process of looking for > an existing key and adding to it's clash list, or creating a new key > with an entry of one. > > This would provide identity safe update/removal/reverse will providing a > set view of the world. I suspect overhead would be < 15% if done right. > > It should be done as a custom accumulate function for now, and we should > profile the performance characteristics of the two - as there are some > optimizations that can be made, such as only lazy storying the value as > a list only when there is more than one entry. If you are willing to > code this up, we can give you pointers on irc: > http://www.jboss.org/drools/irc > http://www.athico.com/Getting_Involved/gettinginvolved.html > > Mark > On 28/02/2012 07:25, Mark Proctor wrote: >> The getResults() >> would probably have to change from... >> >> public Object getResult(Serializable context) throws Exception { >> CollectListData data = (CollectListData) context; >> return Collections.unmodifiableSet( data.map.keySet() ); >> } >> >> ...to... >> >> public Object getResult(Serializable context) throws Exception { >> CollectListData data = (CollectListData) context; >> return Collections.unmodifiableSet( new >> HashSet(data.map.keySet()) >> ); >> } >> >> The proposed getResult() results in a full HashSet copy for each >> change of Rete, that performance hit would be unbearable for most people. >> >> I do agree that there is some merit to what you are asking, but we >> can't give that perf hit to everyone. It would be better to maintain >> two collections, one identity and one set based. And some how only >> change in set what was relevant to the change in identity, you might >> need to build a custom hash table for this - see AbstractHashTable in >> drools which can easily be extended for custom keys and custom >> add/remove methods. >> >> Mark >> >> >> On 27/02/2012 16:53, SirMungus wrote: >>> Mark Proctor wrote >>>> accumulate supports custom functions, just add your own version of >>>> collectSet that uses IdentityMap. >>>> >>> I could do that, but please let me take one more stab, just in case I >>> haven't been clear enough. >>> >>> 1) I am not suggesting that collectSet() return an identity-based >>> set. I am >>> content to have it return a normal set, though I had early confusion >>> on that >>> point. >>> >>> 2) The discussion above about stated vs. justified facts turns out to >>> be a >>> distraction to the main point of the bug that I believe to be in >>> CollectSetAccumulateFunction. My failure to understand the >>> distinction is >>> not germane to the bug. >>> >>> 3) I am not disagreeing with the value behind implementing equals() >>> to only >>> take into account invariant fields, though I am stating that goes >>> beyond the >>> explicity documentation of equals()/hashCode() and is explicitly *not* >>> followed by many classes in the JRE (most notably, collections). >>> >>> There is nothing about the purpose of collectSet() that logically >>> suggests >>> that it should only work with objects that have invariant >>> equals()/hashCode() functions. If five objects match a rule, they >>> should be >>> able to be collected into a set (which may end up with fewer than five >>> objects). If things change such that only three facts match the rule, >>> they >>> should still be able to be collected into a set. There's no compelling >>> reason why that should only apply to objects with invariant equals(). >>> And it >>> seems to me that you rarely want to leave an NPE in code, rather than >>> catching it and throwing a more intelligible exception (e.g., "Object >>> to be >>> removed from collectSet() was not found"). >>> >>> If the internal maintenance of state in CollectSetAccumulateFunction >>> used >>> identity semantics and applied the equality semantics only when you >>> asked it >>> for its results, it would maintain the same external behavior >>> (returning a >>> "normal" equality-based set) without failing to work for objects with >>> variant equals. >>> >>> If you Google around for the proper implementation of equals(), you will >>> find many serious sites that discuss it at length, yet provide examples >>> like: >>> >>> public boolean equals(Object aThat){ >>> if ( this == aThat ) return true; >>> if ( !(aThat instanceof Car) ) return false; >>> Car that = (Car)aThat; >>> return >>> EqualsUtil.areEqual(this.fName, that.fName)&& >>> EqualsUtil.areEqual(this.fNumDoors, that.fNumDoors)&& >>> EqualsUtil.areEqual(this.fGasMileage, that.fGasMileage)&& >>> EqualsUtil.areEqual(this.fColor, that.fColor)&& >>> Arrays.equals(this.fMaintenanceChecks, that.fMaintenanceChecks); >>> //array! >>> } >>> >>> Ironically, that is explicitly against Wolfgang's suggestion above! >>> >>> >>> -- >>> View this message in context: >>> http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun... >>> Sent from the Drools: Developer (committer) mailing list mailing list >>> archive at Nabble.com. >>> _______________________________________________ >>> rules-dev mailing list >>> rules-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/rules-dev >> > > _______________________________________________ > rules-dev mailing list > rules-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/rules-dev > _______________________________________________ rules-dev mailing list rules-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/rules-dev

Edson Tirelli-4 wrote > > a) Please note that clashes objects was taken into account in the design > of the solution, but there is a bug in there. > I eventually surmised that from seeing the use of MutableInt within it. Edson Tirelli-4 wrote > > b) If you look at [1] you will see that the value that is being collected > is already cached by the engine by fact handle ID and used on reverse. So > that takes care of expressions, for instance, where a new object is > created every time. For instance: > > collectSet( $firstName + $lastName ) > > In that case, every time the expression is evaluated, it will create a new > immutable String object that we tract by fact ID, cache it, and properly > manage with equality semantics. > * * * > d) The problem is that we are collecting *sets* of objects, and because of > that, we have to use equality semantics to check for duplicates. I was > using a java Map to track that, and that works find for the use case (b) > above, but obviously breaks on use case (c). Moving to identity does not > seem correct to me as it would work for use case (c), but no longer work > for (b). > > I don't have a solution yet, as I am involved in another large complex > task, but I will re-evaluate this as soon as possible. Meanwhile, if > someone has a suggestion that supports both cases above, please let us > know. > Your scenario (b) is a very interesting point. I'm getting a bit beyond my depth here, but when the existing node in the RETE tree changes such that it realizes it will no longer contribute to the rule, does it re-evaluate the "$firstname + $lastname" and then try to remove it, or does it use a cached FactHandle from its prior evaluation. If it re-evaluates, I agree that it is a problem. If it uses the FactHandle from before, then the IdentityHashMap implementation should be fine, because the equals() semantics will be applied in the getResults(). Your text above reads to me as though the latter is the case. So, aren't we fine here? -- View this message in context: http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun... Sent from the Drools: Developer (committer) mailing list mailing list archive at Nabble.com. _______________________________________________ rules-dev mailing list rules-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/rules-dev

Edson Tirelli-4 wrote > > As you noticed already, the function uses the resulting object as a key > in the map because the goal is indeed to match equals() objects and filter > them to return a set, no duplicates. Of course, because in case of (b) the > hashcode changes, java no longer finds the key to remove it from the map. > > The problem of using a straight identity map is that it will no longer > collect sets of equals() object, but sets of identical objects, what is > obviously not the intent of the function. For instance, in case (a), if > two > objects have the same first and last name, there will be 2 instances of > the > resulting object created, even if they are equals(). That will break the > function. > Thanks for the attention you've been able to give this, Edson. The solution I've proposed doesn't break the equals() contact of the resulting set; it just applies it at getResult() time, rather than during accumulate()/reverse(). So, the main question would be whether the creation of a new HashSet in getResult() is too costly, which I've discussed elsewhere on this thread. Are there existing stress tests that could test that empirically? -- View this message in context: http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun... Sent from the Drools: Developer (committer) mailing list mailing list archive at Nabble.com. _______________________________________________ rules-dev mailing list rules-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/rules-dev

Edson Tirelli-4 wrote > > 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. > > 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. > Edson, thanks for the offer. I'll see what I can do. Since I work at a financial services company, I practically have to get Obama's permission to do anything official on an open source project. :) -- View this message in context: http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun... Sent from the Drools: Developer (committer) mailing list mailing list archive at Nabble.com. _______________________________________________ rules-dev mailing list rules-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/rules-dev

I have cloned it and running benchmarks on it. Might not be able to complete it tonight. -W On 28 February 2012 19:55, Edson Tirelli <ed.tirelli(a)gmail.com&gt; wrote: > > >    I am sure he would not deny that... :) you are helping to make the world a better place... :) > >    In any case, please open a JIRA so that this does not get lost in the e-mails. It is a low hanging fruit, very isolated and simple to fix. So anyone looking for a starting work to contribute to the project can do it. Otherwise I will do it when I get the time. > >    Thanks, >        Edson > > On Tue, Feb 28, 2012 at 1:45 PM, SirMungus <Patrick_Rusk(a)ssga.com&gt; wrote: >> >> >> Edson Tirelli-4 wrote >> > >> > 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. >> > >> >    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. >> > >> Edson, thanks for the offer.  I'll see what I can do.  Since I work at a >> financial services company, I practically have to get Obama's permission to >> do anything official on an open source project. :) >> >> -- >> View this message in context: http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun... >> Sent from the Drools: Developer (committer) mailing list mailing list archive at Nabble.com. >> _______________________________________________ >> rules-dev mailing list >> rules-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/rules-dev > > > > > -- >   Edson Tirelli >   JBoss Drools Core Development >   JBoss by Red Hat @ www.jboss.com > > _______________________________________________ > rules-dev mailing list > rules-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/rules-dev >

So, there would undoubtedly be a performance impact, even not taking into account the "new HashSet()" in the getResults() call. Whether it would be significant enough in real world scenarios to warrant a much more complex implementation requires more knowledge of Drools's user base than I possess, for sure. My guess is that Wolfgang's second scenario above is generally unrealistic. But, if it is routine for rules with collectSet() to end up with sets involving thousands of objects like the /second/ scenario, that could certainly be an issue.

laune wrote > Here are some first results from comparing collectSet (as in 5.3.0) to > a similar accumulate function using an IdentitiyHashMap&lt;Object,Void&gt; > in its context and returns a true Set as a of the key set as its > result. The reported times are the elapsed times of firing a single > rule doing an accumulate into a Set from all String objects in WM. > Out of curiosity, I did a quick timing test of repeated accumulate() calls (without getResults() calls) using IdentityHashMap vs. HashMap using your "s##" strings. While there should have been no particular reason for the performance to differ between the two (indeed, one would think identity would be faster because the identity hash function is natively implemented and presumably much faster), I consistently found the IdentityHashMap to take twice the time for its put() calls. Looking at the source for the two classes, I was surprised to see that they have completely separate implementations. That's pretty astonishing. I would have figured that HashMap would have protected methods like computeHash(key) and testEquals(key1,key2) and that those would be overridden in an IdentityHashMap to use System.identityHashCode() and ==, rather than hashCode() and equals(). My best guess is that someone has spent more time optimizing HashMap than IdentityHashMap. So, there would undoubtedly be a performance impact, even not taking into account the "new HashSet()" in the getResults() call. Whether it would be significant enough in real world scenarios to warrant a much more complex implementation requires more knowledge of Drools's user base than I possess, for sure. My guess is that Wolfgang's second scenario above is generally unrealistic. But, if it is routine for rules with collectSet() to end up with sets involving thousands of objects like the second scenario, that could certainly be an issue. I will try to get the requested JIRA in soon, but I won't be able to give much attention to this until maybe a week from now. I've fallen a bit behind because of all this. :) -- View this message in context: http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun... Sent from the Drools: Developer (committer) mailing list mailing list archive at Nabble.com. _______________________________________________ rules-dev mailing list rules-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/rules-dev