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

[Mark Proctor]

  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-CollectSetAccumulateFunction-should-probably-use-IdentityHashMap-internally-tp3774079p3781313.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