@codex review

@codex review

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 02454bf569

Hi @forsaken628, could you review this PR? It adds zero-deserialization fast paths for bitmap_contains/min/max/has_any/has_all, operating directly on serialized HybridLarge buffers via a TreemapReader instead of full deserialize + method call. The else branch (HybridSmall + Legacy) still uses deserialize_bitmap fallback. I'd appreciate your perspective on the reader.rs zero-deserialization approach and the bitmap.rs dispatch design.

I do not object to reimplementing these operations instead of relying on roaring-rs, but the current implementation is too fragmented. Although TreemapReader and BitmapReader already exist, container-level behavior is still spread across many free functions operating on raw byte slices. A dedicated zero-copy serialized container type should encapsulate parsing, validation, representation dispatch, and operations such as contains, min, max, intersection, and superset checks.

The repeated treemap/container merge traversal should also be centralized, with operation-specific logic supplied through a visitor or handler. ab59ebef (RoaringBitmap/roaring-rs@ab59ebe) provides a useful reference for separating traversal from operation handlers. It is designed around roaring-rs’s materialized containers, so Databend does not need to copy it directly; since both operands here are serialized buffers, we can design a simpler zero-copy visitor around our own serialized container abstraction.

The current tests are also insufficient for maintaining an independent implementation of Roaring semantics. There are some fixed differential checks for has_any and has_all, but equivalent comparisons against RoaringTreemap should cover all newly implemented operations. Property-based tests are also needed, with structured coverage of array/bitmap combinations, multiple prefix buckets, empty inputs, and cardinality boundaries such as 4095/4096/4097.

I do not object to reimplementing these operations instead of relying on roaring-rs, but the current implementation is too fragmented. Although TreemapReader and BitmapReader already exist, container-level behavior is still spread across many free functions operating on raw byte slices. A dedicated zero-copy serialized container type should encapsulate parsing, validation, representation dispatch, and operations such as contains, min, max, intersection, and superset checks.

The repeated treemap/container merge traversal should also be centralized, with operation-specific logic supplied through a visitor or handler. ab59ebef (RoaringBitmap/roaring-rs@ab59ebe) provides a useful reference for separating traversal from operation handlers. It is designed around roaring-rs’s materialized containers, so Databend does not need to copy it directly; since both operands here are serialized buffers, we can design a simpler zero-copy visitor around our own serialized container abstraction.

The current tests are also insufficient for maintaining an independent implementation of Roaring semantics. There are some fixed differential checks for has_any and has_all, but equivalent comparisons against RoaringTreemap should cover all newly implemented operations. Property-based tests are also needed, with structured coverage of array/bitmap combinations, multiple prefix buckets, empty inputs, and cardinality boundaries such as 4095/4096/4097.

Thank you for the detailed review. I fully agree with your feedback:

1. Current implementation is indeed too fragmented. A dedicated zero-copy serialized container type should make it more maintainable.
2. Centralizing the repeated merge traversal logic through visitor pattern is a good idea.
3. Tests comparing against RoaringTreemap and property-based tests will be added.

I'll work on refactoring the implementation and expanding the test coverage accordingly.

@codex review

That was a make lint error. Resolved.

@codex review

@codex review

@codex review

@codex review

I pushed three patches, with the following changes:

1. ContainerReader and SmallReader: encapsulates container/HybridSmall operations
2. Visitor pattern: now the merge traversal logic is centralized with bitmap logics decoupled
3. Structured unit tests: all tests share same fixtures comparing against Roaring(Bit|Tree)map, with combinations of: format, prefix, container key, container type, lhs/rhs relationships, boundary and emptiness
4. Property-based tests: with arbitrary tree generating strategy and verifying against RoaringTreemap, caught 2 bugs, no new one emerges in last ~2000 test runs(64000 random cases for each function)

These should address all three points from the last review.

Regarding the later codex comments: codex wants the fast paths behave exactly same like deserialize_bitmap on corrupted input. For example, this, codex complains bitmap_contains({0:{1}, 0:{2}}, 1) behave differently -- 0:{1} means prefix 0 bucket with value 1, deserialize_bitmap will normalize it into {0:{2}} and return false, new path will take the first and return true. Another example, another, codex complains bitmap_has_any(corrupted, empty) will error in deserialize_bitmap path(because it deserialize both at first), with new path return false (without deserialize and check rhs).

After some thought, I‘d prefer to not to address those. Because they are all corruption related: (1) the bottom line of fast paths should be: correctness, safety and we have introduced random walking proptests for that; (2) even if we deserialize the whole buffer like deserialize_bitmap did, we could not catch all corruption. The corruption should be considered, but not in this pr; (3) this issue is mainly about speedup, and it should not be slowed down by adding extra checks for corruption.

So I think it's ready for another review.

@forsaken628 would you like to take another look?