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

[GoogleCodeExporter]

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-eq
uality

Original issue reported on code.google.com by arg...@gmail.com on 7 Aug 2012 at 11:03

[GoogleCodeExporter]

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)?

Original comment by kak@google.com on 8 Aug 2012 at 4:16

• Added labels: Package-Hash, Type-Defect

[GoogleCodeExporter]

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-id
entity

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.

Original comment by arg...@gmail.com on 8 Aug 2012 at 5:16

[GoogleCodeExporter]

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)

Original comment by andr...@google.com on 8 Aug 2012 at 5:24

[GoogleCodeExporter]

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

Original comment by arg...@gmail.com on 8 Aug 2012 at 6:40

[GoogleCodeExporter]

Stateless class I suppose, right?

Original comment by andr...@google.com on 8 Aug 2012 at 9:22

[GoogleCodeExporter]

Yes, stateless class.

Original comment by arg...@gmail.com on 9 Aug 2012 at 4:48

[GoogleCodeExporter]

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);
}

Original comment by kak@google.com on 9 Aug 2012 at 4:58

[GoogleCodeExporter]

Yes just the instanceof check.

Original comment by arg...@gmail.com on 9 Aug 2012 at 5:11

[GoogleCodeExporter]

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.

Original comment by kak@google.com on 9 Aug 2012 at 5:32

[GoogleCodeExporter]

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).

Original comment by kak@google.com on 9 Aug 2012 at 5:42

[GoogleCodeExporter]

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.

Original comment by kevinb@google.com on 9 Aug 2012 at 5:44

• Changed title: BloomFilter.equals() should use Funnel.equals()
• Changed state: Accepted

[GoogleCodeExporter]

Appears fixed by 
https://code.google.com/p/guava-libraries/source/detail?r=4135d369da714b34a293f5
2bbe81ca28eaab3423 .

Original comment by wasserman.louis on 18 Aug 2012 at 4:20

• Changed state: Fixed

[GoogleCodeExporter]

Appears fixed by 
https://code.google.com/p/guava-libraries/source/detail?r=4135d369da714b34a293f5
2bbe81ca28eaab3423 .

Original comment by wasserman.louis on 18 Aug 2012 at 4:20

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:13

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:08

• Added labels: Restrict-AddIssueComment-Commit