[rdblue]

This adds a MutableBitmap interface to expose a set method for modifying a bitmap and a mutable implementation of Mumbling.

The mutable bitmap implementation is based on a read-only bitmap. The mutable class keeps a map of "overlay" Container objects that track the contents of containers that have been modified, either as a sparse ordered list of local positions or as a dense 32-byte bitmap. When serializing the bitmap, unchanged regions are copied from the original base bitmap and overlay containers are written in between.

This also uses and updates ByteBuffers methods. When using the unsigned little-endian methods, I found that they depend on the byte order of the buffer. This assumption works in the Variant package where these were first added, but not in general.

Test plan:

• Validates pass-through to original bitmap
• Validate overlay works for set bits
  • set bits override overlay
  • set bits show up in isSet
  • setting bits in reverse order works if the byte representation is negative (i.e. 254)
• Serialization
  • changing an existing container
  • adding containers past the original bitmap end
  • updating an empty bitmap
  • sparse containers
  • dense containers
  • serialized container in the middle of a ByteBuffer
  • serialization from a base bitmap in the middle of a ByteBuffer
• Correct cardinality with all cases
• Sparse overlay container
• Dense overlay container
• Conversion from sparse to dense
• Empty mutable bitmap
• Random test

This also includes but does not run a size test similar to the original Rust implementation's that simulates binary pages of bitmaps to check compression and correctness.

[stevenzwu]

I found the naming a bit off. ManifestBitmap is a very specific bitmap for manifest DV. The MutableBitmap sounds like a generic bitmap interface. How about MutableManifestBitmap?

[amogh-jahagirdar]

Commented above, with a builder I think we wouldn't need a public MutableBitmap interface. The builder can hold the mutable state and build() just returns a ManifestBitmap, so the mutable type stays internal to org.apache.iceberg.mumbling.

[rdblue]

MutableManifestBitmap seems a bit too long. I see wanting to make it mirror the other interface and considered adding it as ManifestBitmap.Mutable, but I thought that was long and more awkward. The builder gets around this, but I think we should choose to use a builder or a mutable bitmap independent of the naming decision.

[amogh-jahagirdar]

I'm good with the direction here, I think @stevenzwu pointed out some valid gaps between implementation and spec.

Two things I wanted to think through:

1. The current approach of making the bitmap mutable is fine, though we've used builders in a lot of places in the project and I would've thought we'd do the same here. Something like MumblingBitmaps.builder().buildFrom(originalBitmap).set(...).build() would also return an immutable ManifestBitmap, so callers only ever handle the read type.

2. On the copy-on-write implementation, I'm good as it is but I'm curious about the rationale. CoW is advantageous if we generally expect to modify a few containers and this optimization is well worth it for the general case, since that's when we avoid decoding the untouched ones. Though for a manifest with tens of thousands of entries and a 1% delete rate spread across the manifest, a lot of the containers will be touched anyway. The container section is bounded at exactly 8,192 containers × 32 bytes = 262,144 bytes (256 KiB), so decoding every container upfront has a pretty small worst case, and decoding everything eagerly seems like a much simpler implementation.

Is the assumption that we'll typically only modify a few containers and the optimization is well worth it for that case?

[amogh-jahagirdar]

Minor: Should we just track if the container is dense or sparse from the descriptor rather than looking at the cardinality? Right now the read path uses the descriptor to decide, and this uses the cardinality, which is two separate sources of truth for the same thing.

[rdblue]

The descriptor is passed as length. In the current spec, the descriptor is the length of the container. That's also the cardinality for sparse containers, but dense containers may have a higher cardinality. That's what's happening here: we use length to copy the bytes into a 32-byte buffer (always correct) and if the container is sparse, we set the cardinality from it as well.

[rdblue]

The current approach of making the bitmap mutable is fine, though we've used builders in a lot of places in the project and I would've thought we'd do the same here.

I didn't use a builder because I thought that we would want a mutable structure that can also function as a bitmap for reusing this later. As long as the implementation is almost entirely the same, it makes sense to me to use a bitmap API (set/isSet/cardinality) because it can be reused more. With a builder API it would be odd to check a builder if a particular bit is set, and setting a bit should return the builder for chaining instead of whether the bit was set (or was already set).

CoW is advantageous if we generally expect to modify a few containers and this optimization is well worth it for the general case, since that's when we avoid decoding the untouched ones.

I think this matches our expectation that deletes will generally be sparse. The worst case is random deletes, where you're not targeting files that are clustered together. If you are targeting files that are clustered (say by date partition from writes) then deletes will cluster together and the copy-on-write strategy works better. But even for random deletes, a 1% delete rate means about 1 in 100 files will be affected, which touches about 1 container in 3.

For a 50,000 entry file, there are about 200 containers. If a third of those containers is affected, that is 70 containers or about 2.2k allocated in overlay buffers. The alternative of maintaining a dense bitset is about 49k of memory. Plus you'd have to translate the existing entries by calling set a few thousand times. Maybe it isn't a big deal, but this cost adds up with the number of manifests as well. 100 manifests would cost about 5 MB, rather than 220k.

[stevenzwu]

The new tests lock little-endian reads on a big-endian buffer. writeLittleEndianUnsigned has the same change and no coverage here.

[stevenzwu]

I believe these 4 lines check the positions right before the dense bytes. similar to line 371, I believe the next 3 positions meant 279, 295, 311.

line-381-384 covers the positions right after the dense bytes.

please correct me if I misunderstood the intention.

[stevenzwu]

should we assert the bitmap state before any bit mutations to verify that relocated byte buffer doesn't cause any problem?

[stevenzwu]

Tests cover an invalid version, container count, and cardinality, but not an invalid descriptor. Fixtures and the writer only emit descriptors 0–32, so these cases never appear:

1. A dense descriptor with the low bits set (33–63). The spec says:

The two most significant bits are reserved for future use and implementations must ignore the least-significant bits for a dense container if they are set.

A dense container is 32 bytes. The reader accepts only 32, so 33 is rejected with "Invalid descriptor, not sparse or dense".

2. A descriptor that is neither sparse nor dense (64 or above). 96 is worth including so a check of only bit 5 does not treat it as dense.

These need hand-built buffers.