Re: [rules-dev] BUG: [5.3.0.Final] CollectSetAccumulateFunction should probably use IdentityHashMap internally

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