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

[Mark Proctor]

accumulate supports custom functions, just add your own version of 
collectSet that uses IdentityMap.

Mark
On 25/02/2012 19:28, SirMungus wrote:
> 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-CollectSetAccumulateFunction-should-probably-use-IdentityHashMap-internally-tp3774079p3775596.html
> Sent from the Drools: Developer (committer) mailing list mailing list archive at Nabble.com.
> _______________________________________________
> rules-dev mailing list
> rules-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/rules-dev