BloomFilter.equals() should use Funnel.equals()

[gissuebot]

Original issue created by argaul on 2012-08-07 at 11:03 PM

---

After serializing and deserializing a BloomFilter I was surprised that equals against the original object returned false. This is due to Guava using reference equality instead of object equality in BloomFilter.equals. I pushed a fix to GitHub:

https://github.com/andrewgaul/guava-libraries/tree/bloom-filter-funnel-object-equality

[gissuebot]

Original comment posted by kak@google.com on 2012-08-08 at 04:16 AM

---

I dug up the original (internal) code review where BloomFilter.equals() method was added.

The original author used .equals() for the funnels, but Kevin commented and said: This is problematic. No one's ever going to want to implement an equals() method for their funnel. If they have it, it'll probably be because they had an object serving some other purpose that they decided to just make a funnel as well. If we want to call equals(), we probably have to specify equals() behavior in Funnel.java like we do in Function.java. Or we could document this equals method to say it uses reference equality for the funnel, and people who want equals to work right would have to make sure they use the same constant funnel.

And Dimitri followed up with: I agree with this, let's make it a reference equality, mention this, and add somewhere a footnote that we do recommend implementing funnels as enums (so that we know that deserializing a byte stream twice will create two bloom filters that are equal).

@argaul: Is there any reason you can't have a constant funnel (or implement it as a enum)?

---

Labels: Package-Hash, Type-Defect CC: kevinb@google.com, andreou@google.com

[gissuebot]

Original comment posted by argaul on 2012-08-08 at 05:16 AM

---

While I can change my implementation to an enum, using a constant funnel does not work after serialization:

http://stackoverflow.com/questions/1351706/does-serialization-preserve-object-identity

I object to BloomFilter.equals deviating from the Object.equals contract. While using an enum is a clever implementation detail for Funnel helpers, the Javadoc does not document this requirement and this behavior surprised me when adding a new unit test. If you want to require an enum, please enforce funnel.getClass().isEnum() in BloomFilter.create.

[gissuebot]

Original comment posted by andreou@google.com on 2012-08-08 at 05:24 PM

---

To be fair, the javadoc of BloomFilter#equals does document this: "This implementation uses reference equality to compare funnels".

Just curious, what was you implementation of Funnel#equals() originally? (When you first encountered this behavior)

[gissuebot]

Original comment posted by argaul on 2012-08-08 at 06:40 PM

---

My original Funnel implementation was a class and only implemented funnel. I implemented equals and hashCode after encountering unexpected equals results.

[gissuebot]

Original comment posted by andreou@google.com on 2012-08-08 at 09:22 PM

---

Stateless class I suppose, right?

[gissuebot]

Original comment posted by argaul on 2012-08-09 at 04:48 AM

---

Yes, stateless class.

[gissuebot]

Original comment posted by kak@google.com on 2012-08-09 at 04:58 AM

---

What did your equals() method on your Funnel look like then if it was stateless?

@Override public boolean equals(@Nullable Object object) {   return (object instanceof MyFunnel); }

[gissuebot]

Original comment posted by argaul on 2012-08-09 at 05:11 AM

---

Yes just the instanceof check.

[gissuebot]

Original comment posted by kak@google.com on 2012-08-09 at 05:32 AM

---

Hmm, an equals() method that only has an instanceof check is a little suspect...

If you implemented your funnel as a constant, everything would work out:   private static final Funnel<Long> ANON_FUNNEL =       new Funnel<Long>() {         @Override         public void funnel(Long value, PrimitiveSink into) {           into.putLong(value);         }       };   public void testEqualsAnonymousClassFunnel() {     int size = 100;     BloomFilter<Long> bf1 = BloomFilter.create(ANON_FUNNEL, size);     BloomFilter<Long> bf2 = BloomFilter.create(ANON_FUNNEL, size);     assertEquals(bf1, bf2);   }

Or if your Funnel had a private ctor and you only exposed a single static singleton. Seems preferable to adding a questionable equals/hashcode method to your stateless class.

[gissuebot]

Original comment posted by kak@google.com on 2012-08-09 at 05:42 AM

---

Hmm, I forgot to take into account your comment about serialization (the test I gave does indeed pass, but it doesn't attempt to serialize/de-serialize).

[gissuebot]

Original comment posted by kevinb@google.com on 2012-08-09 at 05:44 PM

---

argaul, thanks for bringing this up. We've gone over all our old reasoning on this, and realized that, well, you're right!

For one thing, I hadn't thought about the serialization issue at all. What I was concerned about was that using Funnel.equals() would require us to add bloated Funnel.equals() javadoc which would in turn make Funnel look more complex than it should. However, I now realize that even without Funnel.equals() javadoc, plain common sense leads you to try what you tried and it should have worked.

We'll fix this.

---

Status: Accepted Owner: kurt.kluever

[gissuebot]

Original comment posted by wasserman.louis on 2012-08-18 at 04:20 PM

---

Appears fixed by https://github.com/google/guava/commit/4135d369da714b34a293f52bbe81ca28eaab3423 .

---

Status: Fixed

[gissuebot]

Original comment posted by wasserman.louis on 2012-08-18 at 04:20 PM

---

Appears fixed by https://github.com/google/guava/commit/4135d369da714b34a293f52bbe81ca28eaab3423 .