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

[SirMungus]

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.