Add a new BF strategy. The old one is subtly broken. For more information, see: https://code.google.com/p/guava-libraries/issues/detail?id=1119

-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=63970075

Author: kluever

guava-tests/test/com/google/common/hash/BloomFilterTest.java

@@ -38,7 +38,6 @@
  * @author Dimitris Andreou
  */
 public class BloomFilterTest extends TestCase {
-
   public void testLargeBloomFilterDoesntOverflow() {
     long numBits = Integer.MAX_VALUE;
     numBits++;
@@ -93,6 +92,47 @@ public void testCreateAndCheckMitz32BloomFilterWithKnownFalsePositives() {
     assertEquals(actualFpp, expectedFpp, 0.00015);
   }
 
+  public void testCreateAndCheckBloomFilterWithKnownFalsePositives64() {
+    int numInsertions = 1000000;
+    BloomFilter<CharSequence> bf = BloomFilter.create(
+        Funnels.unencodedCharsFunnel(), numInsertions, 0.03,
+        BloomFilterStrategies.MURMUR128_MITZ_64);
+
+    // Insert "numInsertions" even numbers into the BF.
+    for (int i = 0; i < numInsertions * 2; i += 2) {
+      bf.put(Integer.toString(i));
+    }
+
+    // Assert that the BF "might" have all of the even numbers.
+    for (int i = 0; i < numInsertions * 2; i += 2) {
+      assertTrue(bf.mightContain(Integer.toString(i)));
+    }
+
+    // Now we check for known false positives using a set of known false positives.
+    // (These are all of the false positives under 900.)
+    ImmutableSet<Integer> falsePositives = ImmutableSet.of(
+        1, 143, 231, 287, 311, 319, 331, 421, 457, 547, 599, 659, 723);
+    for (int i = 1; i < 900; i += 2) {
+      if (!falsePositives.contains(i)) {
+        assertFalse("BF should not contain " + i, bf.mightContain(Integer.toString(i)));
+      }
+    }
+
+    // Check that there are exactly 29651 false positives for this BF.
+    int knownNumberOfFalsePositives = 29651;
+    int numFpp = 0;
+    for (int i = 1; i < numInsertions * 2; i += 2) {
+      if (bf.mightContain(Integer.toString(i))) {
+        numFpp++;
+      }
+    }
+    assertEquals(knownNumberOfFalsePositives, numFpp);
+    double actualFpp = (double) knownNumberOfFalsePositives / numInsertions;
+    double expectedFpp = bf.expectedFpp();
+    // The normal order of (expected, actual) is reversed here on purpose.
+    assertEquals(actualFpp, expectedFpp, 0.00033);
+  }
+
   /**
    * Sanity checking with many combinations of false positive rates and expected insertions
    */
@@ -361,7 +401,8 @@ public void testJavaSerialization() {
    * Only appending a new constant is allowed.
    */
   public void testBloomFilterStrategies() {
-    assertEquals(1, BloomFilterStrategies.values().length);
+    assertEquals(2, BloomFilterStrategies.values().length);
     assertEquals(BloomFilterStrategies.MURMUR128_MITZ_32, BloomFilterStrategies.values()[0]);
+    assertEquals(BloomFilterStrategies.MURMUR128_MITZ_64, BloomFilterStrategies.values()[1]);
   }
 }

guava/src/com/google/common/hash/BloomFilterStrategies.java

@@ -18,6 +18,7 @@
 
 import com.google.common.math.LongMath;
 import com.google.common.primitives.Ints;
+import com.google.common.primitives.Longs;
 
 import java.math.RoundingMode;
 import java.util.Arrays;
@@ -32,6 +33,7 @@
  * on their ordinal for BloomFilter serialization.
  *
  * @author Dimitris Andreou
+ * @author Kurt Alfred Kluever
  */
 enum BloomFilterStrategies implements BloomFilter.Strategy {
   /**
@@ -42,40 +44,100 @@ enum BloomFilterStrategies implements BloomFilter.Strategy {
   MURMUR128_MITZ_32() {
     @Override public <T> boolean put(T object, Funnel<? super T> funnel,
         int numHashFunctions, BitArray bits) {
+      long bitSize = bits.bitSize();
       long hash64 = Hashing.murmur3_128().hashObject(object, funnel).asLong();
       int hash1 = (int) hash64;
       int hash2 = (int) (hash64 >>> 32);
+
       boolean bitsChanged = false;
       for (int i = 1; i <= numHashFunctions; i++) {
-        int nextHash = hash1 + i * hash2;
-        if (nextHash < 0) {
-          nextHash = ~nextHash;
+        int combinedHash = hash1 + (i * hash2);
+        // Flip all the bits if it's negative (guaranteed positive number)
+        if (combinedHash < 0) {
+          combinedHash = ~combinedHash;
         }
-        bitsChanged |= bits.set(nextHash % bits.bitSize());
+        bitsChanged |= bits.set(combinedHash % bitSize);
       }
       return bitsChanged;
     }
 
     @Override public <T> boolean mightContain(T object, Funnel<? super T> funnel,
         int numHashFunctions, BitArray bits) {
+      long bitSize = bits.bitSize();
       long hash64 = Hashing.murmur3_128().hashObject(object, funnel).asLong();
       int hash1 = (int) hash64;
       int hash2 = (int) (hash64 >>> 32);
+
       for (int i = 1; i <= numHashFunctions; i++) {
-        int nextHash = hash1 + i * hash2;
-        if (nextHash < 0) {
-          nextHash = ~nextHash;
+        int combinedHash = hash1 + (i * hash2);
+        // Flip all the bits if it's negative (guaranteed positive number)
+        if (combinedHash < 0) {
+          combinedHash = ~combinedHash;
         }
-        if (!bits.get(nextHash % bits.bitSize())) {
+        if (!bits.get(combinedHash % bitSize)) {
           return false;
         }
       }
       return true;
     }
+  },
+  /**
+   * This strategy uses all 128 bits of {@link Hashing#murmur3_128} when hashing. It looks
+   * different than the implementation in MURMUR128_MITZ_32 because we're avoiding the
+   * multiplication in the loop and doing a (much simpler) += hash2. We're also changing the
+   * index to a positive number by AND'ing with Long.MAX_VALUE instead of flipping the bits.
+   */
+  MURMUR128_MITZ_64() {
+    @Override
+    public <T> boolean put(T object, Funnel<? super T> funnel,
+        int numHashFunctions, BitArray bits) {
+      long bitSize = bits.bitSize();
+      byte[] bytes = Hashing.murmur3_128().hashObject(object, funnel).getBytesInternal();
+      long hash1 = lowerEight(bytes);
+      long hash2 = upperEight(bytes);
+
+      boolean bitsChanged = false;
+      long combinedHash = hash1 + hash2;
+      for (int i = 0; i < numHashFunctions; i++) {
+        // Make the combined hash positive and indexable
+        bitsChanged |= bits.set((combinedHash & Long.MAX_VALUE) % bitSize);
+        combinedHash += hash2;
+      }
+      return bitsChanged;
+    }
+
+    @Override
+    public <T> boolean mightContain(T object, Funnel<? super T> funnel,
+        int numHashFunctions, BitArray bits) {
+      long bitSize = bits.bitSize();
+      byte[] bytes = Hashing.murmur3_128().hashObject(object, funnel).getBytesInternal();
+      long hash1 = lowerEight(bytes);
+      long hash2 = upperEight(bytes);
+
+      long combinedHash = hash1 + hash2;
+      for (int i = 0; i < numHashFunctions; i++) {
+        // Make the combined hash positive and indexable
+        if (!bits.get((combinedHash & Long.MAX_VALUE) % bitSize)) {
+          return false;
+        }
+        combinedHash += hash2;
+      }
+      return true;
+    }
+
+    private /* static */ long lowerEight(byte[] bytes) {
+      return Longs.fromBytes(
+          bytes[7], bytes[6], bytes[5], bytes[4], bytes[3], bytes[2], bytes[1], bytes[0]);
+    }
+
+    private /* static */ long upperEight(byte[] bytes) {
+      return Longs.fromBytes(
+          bytes[15], bytes[14], bytes[13], bytes[12], bytes[11], bytes[10], bytes[9], bytes[8]);
+    }
   };
 
   // Note: We use this instead of java.util.BitSet because we need access to the long[] data field
-  static class BitArray {
+  static final class BitArray {
     final long[] data;
     long bitCount;