Core: Add portable Roaring bitmap for row positions #11372

aokolnychyi · 2024-10-21T20:40:12Z

This PR adds a Roaring bitmap implementation that follows the official "portable" serialization format from the library for row positions in preparation for V3 position deletes. We are still discussing the blob layout for DVs on the dev list but we agree we should serialize the bitmap according to the portable serialization spec.

We cannot rely on the Roaring64Bitmap from the library as it has undefined serialization behavior:

  /**
   * {@link Roaring64NavigableMap} are serializable. However, contrary to RoaringBitmap, the
   * serialization format is not well-defined: for now, it is strongly coupled with Java standard
   * serialization. Just like the serialization may be incompatible between various Java versions,
   * {@link Roaring64NavigableMap} are subject to incompatibilities. Moreover, even on a given Java
   * versions, the serialization format may change from one RoaringBitmap version to another
   */

Here are benchmarks comparing this implementation to the one from the library we use today.

Benchmark                                                         Mode  Cnt  Score   Error  Units
RoaringPositionBitmapBenchmark.addAndCheckPositionsIcebergBitmap    ss    5  0.535 ± 0.032   s/op
RoaringPositionBitmapBenchmark.addAndCheckPositionsLibraryBitmap    ss    5  1.124 ± 0.013   s/op
RoaringPositionBitmapBenchmark.addOrderedPositionsIcebergBitmap     ss    5  0.023 ± 0.003   s/op
RoaringPositionBitmapBenchmark.addOrderedPositionsLibraryBitmap     ss    5  0.165 ± 0.028   s/op
RoaringPositionBitmapBenchmark.addShuffledPositionsIcebergBitmap    ss    5  0.320 ± 0.006   s/op
RoaringPositionBitmapBenchmark.addShuffledPositionsLibraryBitmap    ss    5  0.338 ± 0.011   s/op

This is not a spec change but part of #11122.

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

aokolnychyi · 2024-10-21T20:55:39Z

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

+   * @param posStartInclusive the start position of the range (inclusive)
+   * @param posEndExclusive the end position of the range (exclusive)
+   */
+  public void addRange(long posStartInclusive, long posEndExclusive) {


This can be optimized in the future but we have no use case in the repo beyond tests.

Should we make this package-private then? This has implications for run encoding in the bitmaps.

Our public PositionDeleteIndex where we will eventually consume this class actually supports marking ranges as removed. Therefore, it is needed, but we shouldn't be concerned optimizing this right away.

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

aokolnychyi · 2024-10-21T21:10:50Z

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

+import org.roaringbitmap.RoaringBitmap;
+
+/**
+ * A bitmap that supports positive 64-bit positions, but is optimized for cases where most positions


Credit to @rdblue for most of this description copied from the spec PRs for DVs.

LICENSE

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

rdblue · 2024-10-21T21:17:20Z

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

+   * @see <a href="https://github.com/RoaringBitmap/RoaringFormatSpe">Roaring bitmap spec</a>
+   */
+  public void serialize(ByteBuffer buffer) {
+    validateByteOrder(buffer);


Why not duplicate the ByteBuffer, set its endianness, and then update the buffer's position to match the duplicate after serialization?

I do similar stuff in the caller, it is more like a sanity check at this point, which also follows the Roaring lib API.

RussellSpitzer · 2024-10-21T21:21:39Z

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

+  }
+
+  // extracts high 32 bits from a 64-bit position
+  private static int highBytes(long pos) {


I tend to agree here, since you need to read the javadoc to know that these are the key bytes, so might be worth renaming the method to indicate that

Or just key?

The problem is that input in many methods is called position and represents 64-bit positions. If we want to rename this method to key (which I agree makes sense), we will have to consider renaming lowBytes too, which I don't have a very good alternative for.

Let me think.

I renamed to "key" and "value". Please, take a look, @RussellSpitzer @nastra @amogh-jahagirdar.

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

RussellSpitzer · 2024-10-21T21:23:53Z

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

+  }
+
+  /**
+   * Adds a position to the bitmap.


I wonder if we should use "add", because in my mind this is more of a "set"

I used add/contains from the Roaring lib but open to any names.
What would be contains equivalent if we go with set?

I don't have super strong opinions on this, but I always kind of think of "add" for Sets and "set" for Bitmaps. I know we are kind of doing the same things but at least mentally in my head it's not as if we are actually "adding" more like we are just flipping a bit.

I think contains would still probably be fine?

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

RussellSpitzer · 2024-10-21T21:29:42Z

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

+   *
+   * @param that the other bitmap
+   */
+  public void or(RoaringPositionBitmap that) {


Why not return this for chaining? I'm not sure what our standard is for this sort of API

To highlight it modifies the bitmap in place, not a strong opinion. What do you think?

Either way is ok with me, I know in most of our public APIs we use the functional approach even for modify in place.

RussellSpitzer · 2024-10-21T21:32:53Z

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

+   *
+   * @return whether the bitmap was compressed
+   */
+  public boolean runOptimize() {


I'm not sure if we want to expose Roaring bitmap apis like this?

I use this while serializing BitmapPositionDeleteIndex in the main PR.

Would it be better to just call this compress or runLengthEncode? I'm OK either way, i see roaring bit map calls it runOptimize, just feels like too broad of a name

I do like compress, I'll rename.

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

core/src/test/java/org/apache/iceberg/deletes/TestRoaringPositionBitmap.java

rdblue · 2024-10-21T21:42:12Z

core/src/test/java/org/apache/iceberg/deletes/TestRoaringPositionBitmap.java

+        position(1 /* bitmap */, 2 /* container */, 1L),
+        position(1 /* bitmap */, 2 /* container */, CONTAINER_OFFSET - 1));
+
+    assertThat(bitmap.runOptimize()).as("Bitmap must be RLE encoded").isTrue();


Should optimize be run as part of serialization?

I call that in PositionDeleteIndex, not sure about the bitmap itself.

Actually, we can't do that. The caller may have called serializedSizeInBytes() to estimate the size prior to the serialization.

rdblue

The implementation looks correct overall to me. I have a few suggestions for minor things, but it looks like this is about ready.

RussellSpitzer · 2024-10-21T21:46:48Z

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

+
+  private void allocateBitmapsIfNeeded(int requiredLength) {
+    if (bitmaps.length < requiredLength) {
+      if (bitmaps.length == 0 && requiredLength == 1) {


Why is there a special case for this?

Simply optimizes the most frequent use case.

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

RussellSpitzer · 2024-10-21T21:53:12Z

core/src/test/java/org/apache/iceberg/deletes/TestRoaringPositionBitmap.java

+import org.apache.iceberg.relocated.com.google.common.io.Resources;
+import org.junit.jupiter.api.Test;
+
+public class TestRoaringPositionBitmap {


I think this would be a good place to do some property based testing since we already have a class that matches the behavior we want ("HashSet"). This way we can hit more than a select few positions and verify the behaviors pretty simply I think?

Will add some fuzz testing.

I added some fuzz tests and included seed information in the test arguments. If anything fails, we will have a way to reproduce the issue reliably.

core/src/test/java/org/apache/iceberg/deletes/TestRoaringPositionBitmap.java

nastra · 2024-10-22T10:03:35Z

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

+  private static int readKey(ByteBuffer buffer, int lastKey) {
+    int key = buffer.getInt();
+    Preconditions.checkArgument(key >= 0, "Invalid unsigned key: %s", key);
+    Preconditions.checkArgument(key <= Integer.MAX_VALUE - 1, "Key is too large: %s", key);


looks like this and the precondition right below it isn't examined in the tests

Covering this would be fairly involved as this logic is part of deserialization and we don't produce such files.

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

danielcweeks

I think there are a couple good outstanding ideas for clarifying method names, but I'm ok with whatever is decided.

aokolnychyi · 2024-10-23T18:27:31Z

core/src/test/java/org/apache/iceberg/deletes/TestRoaringPositionBitmap.java

+  }
+
+  @TestTemplate
+  public void testRandomSparseBitmap() {


@RussellSpitzer, here we have 3 tests with random values that are compared against Set<Long>.

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

RussellSpitzer · 2024-10-24T17:26:04Z

core/src/test/java/org/apache/iceberg/deletes/TestRoaringPositionBitmap.java

+    return Resources.toByteArray(resource);
+  }
+
+  private static void assertEqual(RoaringPositionBitmap bitmap, Set<Long> positions) {


this is really 3 asserts
assertEqual()
assertEqualAfterSerDE()
assertEqualAfterCompress()

, but it's probably fine to combine them since the debugger will point to which block we are in

I added a common method for the checks.

aokolnychyi · 2024-10-28T18:03:34Z

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java

+   *
+   * @param pos the position
+   */
+  public void set(long pos) {


@RussellSpitzer, I switched to set instead of add. It matches BitSet in Java.

aokolnychyi · 2024-10-28T21:04:30Z

I am going to merge this one as it was thoroughly reviewed. There is no need to include it in 1.7, though (no harm too).

Thanks for reviewing, @rdblue @RussellSpitzer @amogh-jahagirdar @nastra @danielcweeks!

github-actions bot added the core label Oct 21, 2024

aokolnychyi commented Oct 21, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java Show resolved Hide resolved

Core: Add portable Roaring bitmap for row positions

dfbba84

aokolnychyi force-pushed the serializeble-bitmap branch from f168c3a to dfbba84 Compare October 21, 2024 20:51

aokolnychyi commented Oct 21, 2024

View reviewed changes

rdblue reviewed Oct 21, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java Outdated Show resolved Hide resolved

aokolnychyi commented Oct 21, 2024

View reviewed changes

rdblue reviewed Oct 21, 2024

View reviewed changes

LICENSE Outdated Show resolved Hide resolved

RussellSpitzer reviewed Oct 21, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java Outdated Show resolved Hide resolved

rdblue reviewed Oct 21, 2024

View reviewed changes

RussellSpitzer reviewed Oct 21, 2024

View reviewed changes

rdblue reviewed Oct 21, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Oct 21, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Oct 21, 2024

View reviewed changes

rdblue reviewed Oct 21, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java Show resolved Hide resolved

RussellSpitzer reviewed Oct 21, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java Show resolved Hide resolved

RussellSpitzer reviewed Oct 21, 2024

View reviewed changes

rdblue reviewed Oct 21, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java Outdated Show resolved Hide resolved

rdblue reviewed Oct 21, 2024

View reviewed changes

core/src/test/java/org/apache/iceberg/deletes/TestRoaringPositionBitmap.java Outdated Show resolved Hide resolved

rdblue reviewed Oct 21, 2024

View reviewed changes

core/src/test/java/org/apache/iceberg/deletes/TestRoaringPositionBitmap.java Show resolved Hide resolved

rdblue reviewed Oct 21, 2024

View reviewed changes

core/src/test/java/org/apache/iceberg/deletes/TestRoaringPositionBitmap.java Outdated Show resolved Hide resolved

rdblue reviewed Oct 21, 2024

View reviewed changes

core/src/test/java/org/apache/iceberg/deletes/TestRoaringPositionBitmap.java Outdated Show resolved Hide resolved

rdblue reviewed Oct 21, 2024

View reviewed changes

rdblue approved these changes Oct 21, 2024

View reviewed changes

RussellSpitzer reviewed Oct 21, 2024

View reviewed changes

danielcweeks reviewed Oct 21, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Oct 21, 2024

View reviewed changes

danielcweeks reviewed Oct 21, 2024

View reviewed changes

core/src/test/java/org/apache/iceberg/deletes/TestRoaringPositionBitmap.java Show resolved Hide resolved

danielcweeks reviewed Oct 21, 2024

View reviewed changes

core/src/test/java/org/apache/iceberg/deletes/TestRoaringPositionBitmap.java Outdated Show resolved Hide resolved

aokolnychyi added 3 commits October 21, 2024 15:28

Feedback

41c769f

More tests and feedback

5112ebb

addAll

7961648

nastra reviewed Oct 22, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java Show resolved Hide resolved

danielcweeks approved these changes Oct 22, 2024

View reviewed changes

aokolnychyi added 2 commits October 23, 2024 16:18

Basic fuzz tests

830b1b0

More renames

9f236c9

aokolnychyi commented Oct 23, 2024

View reviewed changes

More renames

4bbaf7d

nastra approved these changes Oct 24, 2024

View reviewed changes

RussellSpitzer reviewed Oct 24, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/deletes/RoaringPositionBitmap.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Oct 24, 2024

View reviewed changes

Polish

2d1b546

aokolnychyi commented Oct 28, 2024

View reviewed changes

RussellSpitzer approved these changes Oct 28, 2024

View reviewed changes

Minor private method rename

8a6729f

aokolnychyi merged commit 1e3ee1e into apache:main Oct 28, 2024
49 checks passed

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

Core: Add portable Roaring bitmap for row positions (apache#11372)

b3723a6

Core: Add portable Roaring bitmap for row positions #11372

Core: Add portable Roaring bitmap for row positions #11372

Conversation

aokolnychyi commented Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aokolnychyi Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aokolnychyi Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

RussellSpitzer Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielcweeks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aokolnychyi commented Oct 28, 2024

aokolnychyi commented Oct 21, 2024 •

edited

Loading

aokolnychyi Oct 23, 2024 •

edited

Loading

RussellSpitzer Oct 21, 2024 •

edited

Loading

aokolnychyi Oct 21, 2024 •

edited

Loading

RussellSpitzer Oct 21, 2024 •

edited

Loading