SPARK-18252: Using RoaringBitmap for bloom filters #15917

ponkin · 2016-11-17T09:10:41Z

What changes were proposed in this pull request?

Bloom filter is now using RoaringBitmap as bit vector - to save memory usage
Bloom filter is using Murmur 128 bit hash

How was this patch tested?

All previous tests are valid
Added more accurate test for Byte type

Please review https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark before opening a pull request.

srowen · 2016-11-17T11:25:50Z

common/sketch/src/main/java/org/apache/spark/util/sketch/Murmur3_128.java

+ * Best perfomance is on x86_64 platform
+ * Based on implementation https://github.com/aappleby/smhasher/blob/master/src/MurmurHash3.cpp
+ */
+final class Murmur3_128 {


Could you explain why we need to add a custom murmurhash implementation? it seems like conceptually we're just swapping bitmaps for bitmapss. What's the license of this code?

This decision came from fact that RoaringBitmap supports only int indexes(and can have only Int.MaxValue bits), since we use RoaringBitmap[] to represent vector way larger than Int.MaxValue, we need good hash function to generate long hash values. This implementation is based on smhasher and solr implementation which are both freely available I suppose. Also this implementation must be way faster on 64 bit platform than Murmur3_x86_32

Rather than make a new implementation, use Guava's Hashing? Scala and Spark itself already have murmur hash impls but they seem 32-bit only

Well Guava`s implementation is allocating a lot of temp objects(works only with ByteBuffer source). So I decided to do standalone implementation - all tests for Hasher are taken from Guava lib.

OK, maybe that makes sense. If bothering with a custom impl, why not 64 bits instead of 128? it would avoid needing a long[2] to take the result?

In fact the is Murmur hash version which produces 64 - it is called Murmur 2 Wiki, but it is previous version of Murmur, current is Murmur 3 which is faster and give better distribution(Murmur2 has some flaws). Since in bloom filter we always need 1 or more hash functions, creating 128 bit hash(two longs) is pretty good I suppose.

srowen · 2016-11-17T11:26:53Z

common/sketch/src/test/java/org/apache/spark/util/sketch/Murmur3_128Suite.java

@@ -0,0 +1,30 @@
+package org.apache.spark.util.sketch;


Missing a license

srowen · 2016-11-17T11:27:50Z

common/sketch/src/main/java/org/apache/spark/util/sketch/RoaringBitmapArray.java

+import java.io.IOException;
+import java.util.Arrays;
+
+class RoaringBitmapArray {


Probably a dumb question but why do we need an array of bitmaps? I thought RoaringBitmap was a BitArray counterpart?

Unfortunately, RoaringBitmap supports only int indexes javadoc. Since we need to support bloom filters with size more than Int.MaxValue(BitArray supported 64*IntMaxValue) I decided to partition bloom filter with RoaringBitmap[]. Now Bloom filter can be Long.MaxValue large in size(number of bits).

Hm, I see: RoaringBitmap/RoaringBitmap#109
I wonder if it's worth using RoaringBitmap then, but, of course making the improvements we want from RB would take work too, and, removing code helps.

well, the main point of this improvement is to save memory - now even filters with few elements acquire space needed for full bloom filter. With this improvement bloom filter size will increase as we insert values in it.

It's certainly worth documenting this in the scaladoc BTW, why the class has to exist.

srowen

CC @cloud-fan for an initial look before we get further

srowen · 2016-11-17T13:52:48Z

common/sketch/src/main/java/org/apache/spark/util/sketch/RoaringBitmapArray.java

+import java.io.IOException;
+import java.util.Arrays;
+
+class RoaringBitmapArray {


It's certainly worth documenting this in the scaladoc BTW, why the class has to exist.

srowen · 2016-11-17T13:53:25Z

common/sketch/src/main/java/org/apache/spark/util/sketch/RoaringBitmapArray.java

+        return (int) Math.ceil(numBits / (double)Integer.MAX_VALUE);
+    }
+
+    private static RoaringBitmap[] initialVector(int numOfBuckets){


Small style things: space before braces, and after casts, as in a few lines above. Spaces around elements of a for loop, as below.

srowen · 2016-11-17T13:53:52Z

common/sketch/src/main/java/org/apache/spark/util/sketch/RoaringBitmapArray.java

+
+    /** Number of bits */
+    long bitSize() {
+        return numBits;


2 space indent everywhere

srowen · 2016-11-17T13:55:40Z

common/sketch/src/main/java/org/apache/spark/util/sketch/RoaringBitmapArray.java

+
+    @Override
+    public int hashCode() {
+        return Arrays.hashCode(data);


Is it worth incorporating bitCount, numBits? it's not wrong to omit them, but might be didier.

srowen · 2016-11-17T13:55:46Z

common/sketch/src/main/java/org/apache/spark/util/sketch/RoaringBitmapArray.java

+    @Override
+    public boolean equals(Object other) {
+        if (this == other) return true;
+        if (other == null || !(other instanceof RoaringBitmapArray)) return false;


Use braces, newline for return

srowen · 2016-11-17T13:55:57Z

common/sketch/src/main/java/org/apache/spark/util/sketch/RoaringBitmapArray.java

+        if (other == null || !(other instanceof RoaringBitmapArray)) return false;
+        RoaringBitmapArray that = (RoaringBitmapArray) other;
+        return (this.bitCount == that.bitCount) &&
+                (this.numBits == that.numBits) &&


4-space continuation indents

srowen · 2016-11-17T13:56:55Z

common/sketch/src/main/java/org/apache/spark/util/sketch/RoaringBitmapArray.java

+class RoaringBitmapArray {
+
+    private final RoaringBitmap[] data;
+    private long bitCount; // number of 1`s in bitset


Is it worth maintaining this? it slows down paths like set; is it called frequently enough to cache?

it is only need to calculate expectedFpp (false positive rate) which is not very frequent operation, I agree. will remove

srowen · 2016-11-17T14:01:42Z

common/sketch/src/main/java/org/apache/spark/util/sketch/BloomFilter.java


    return new BloomFilterImpl(optimalNumOfHashFunctions(expectedNumItems, numBits), numBits);
  }
+


Go ahead and back out this spurious change; there's another newline at the end of the next file

srowen · 2016-11-17T14:05:55Z

common/sketch/src/main/java/org/apache/spark/util/sketch/Murmur3_128.java

+ * Best perfomance is on x86_64 platform
+ * Based on implementation https://github.com/aappleby/smhasher/blob/master/src/MurmurHash3.cpp
+ */
+final class Murmur3_128 {


OK, maybe that makes sense. If bothering with a custom impl, why not 64 bits instead of 128? it would avoid needing a long[2] to take the result?

srowen · 2016-11-17T14:06:40Z

Jenkins add to whitelist

SparkQA · 2016-11-17T14:08:40Z

Test build #68782 has finished for PR 15917 at commit 2cd92c3.

This patch fails RAT tests.
This patch merges cleanly.
This patch adds no public classes.

rxin · 2016-11-17T19:04:13Z

Please see my comment on the jira ticket. I'm not sure this is a good idea.

Also the current implementation ignores versioning.

ponkin · 2016-11-17T20:34:03Z

@srowen It seems like I have fixed all your comments.

SparkQA · 2016-11-17T20:39:37Z

Test build #68796 has finished for PR 15917 at commit 66f58d1.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

rxin · 2016-11-17T21:10:36Z

Also cc @lemire

Can you take a look at the discussion on the JIRA ticket and chime in? https://issues.apache.org/jira/browse/SPARK-18252

lemire · 2016-11-17T22:01:09Z

One of the core Roaring authors (@gssiyankai) worked specifically on Roaring and Bloom filters. Any thoughts Gregory?

SparkQA · 2016-11-17T23:07:16Z

Test build #68797 has finished for PR 15917 at commit 487da8f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-11-18T10:53:12Z

Yes all good points, this is more complex than I had imagined, especially with the good point about needing to support older serialized forms. This may not be worth it, yes.

It would be nice to have just one implementation one way or the other, rather than 2. At the least, it may be fine to improve the existing implementation and punt on the rest.

rxin · 2016-11-18T19:22:49Z

@ponkin mind closing the pull request given your findings on https://issues.apache.org/jira/browse/SPARK-18252 ?

Closes apache#15689 Closes apache#14640 Closes apache#15917 Closes apache#16188 Closes apache#16206

SPARK-18252: Using RoaringBitmap for bloom filters

2cd92c3

srowen reviewed Nov 17, 2016

View reviewed changes

srowen requested changes Nov 17, 2016

View reviewed changes

SPARK-18252: Fixing review comments

66f58d1

SPARK-18252: Fixing RAT style errors

487da8f

HyukjinKwon mentioned this pull request Dec 8, 2016

[BUILD] Closing some stale/inappropriate PRs #16207

Closed

asfgit closed this in 330fda8 Dec 8, 2016

guevara mentioned this pull request May 6, 2025

如何设计高性能支持64位的去重服务 guevara/read-it-later#12148

Open

zifeif2 pushed a commit to zifeif2/spark that referenced this pull request Nov 22, 2025

Close stale pull requests.

0de30bb

Closes apache#15689 Closes apache#14640 Closes apache#15917 Closes apache#16188 Closes apache#16206


		return new BloomFilterImpl(optimalNumOfHashFunctions(expectedNumItems, numBits), numBits);
		}

SPARK-18252: Using RoaringBitmap for bloom filters #15917

SPARK-18252: Using RoaringBitmap for bloom filters #15917

Uh oh!

Conversation

ponkin commented Nov 17, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ponkin Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen commented Nov 17, 2016

Uh oh!

SparkQA commented Nov 17, 2016

Uh oh!

rxin commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ponkin commented Nov 17, 2016

Uh oh!

SparkQA commented Nov 17, 2016

Uh oh!

rxin commented Nov 17, 2016

Uh oh!

lemire commented Nov 17, 2016

Uh oh!

SparkQA commented Nov 17, 2016

Uh oh!

srowen commented Nov 18, 2016

Uh oh!

rxin commented Nov 18, 2016

Uh oh!

Reviewers

Assignees

ponkin Nov 17, 2016 •

edited

Loading

rxin commented Nov 17, 2016 •

edited

Loading