feat: avoid deserialization in bitmap_contains/min/max/has_any/has_all - #20210
feat: avoid deserialization in bitmap_contains/min/max/has_any/has_all#20210harry-hao wants to merge 10 commits into
Conversation
harry-hao
changed the title
github-actions
Bot
added
the
pr-feature
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aab60db4a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
2aab60d to
02454bf
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02454bf569
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
02454bf to
cd08039
Compare
Added a `bitmap_scalar` bench group covering: - bitmap_contains - bitmap_count - bitmap_min - bitmap_max - bitmap_and - bitmap_has_any - bitmap_has_all
For large hybrid bitmaps, bitmap_contains now reads the serialized buffer directly via binary search on container descriptions instead of deserializing the entire bitmap into memory. This reduces bitmap_contains_large from ~12.1µs to ~2.1µs (6x speedup).
For large hybrid bitmaps, bitmap_min now reads the minimum value directly from the serialized buffer (first element of the first container) instead of deserializing the entire roaring treemap. This reduces bitmap_min_large from ~10.3µs to ~562ns (18x speedup).
For large hybrid bitmaps, bitmap_max now reads the maximum value directly from the serialized buffer (last element of the last container) instead of deserializing the entire roaring treemap. This reduces bitmap_max_large from ~10.7µs to ~531ns (20x speedup).
For large hybrid bitmaps, bitmap_has_any now checks intersection directly on the serialized buffers using container-level binary search instead of deserializing both bitmaps and computing full intersection. This reduces bitmap_has_any_large_large from ~26µs to ~2.5µs (10x), and disjoint cases from ~20µs to ~963ns (21x).
For large hybrid bitmaps, bitmap_has_all now checks superset relationships directly on serialized buffers using container-level comparison instead of deserializing both bitmaps. This reduces bitmap_has_all_large_large from ~39µs to ~11µs (3.5x), and disjoint cases from ~20µs to ~932ns (22x).
9bafe89 to
4ba28b9
Compare
|
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. |
Thank you for the detailed review. I fully agree with your feedback:
I'll work on refactoring the implementation and expanding the test coverage accordingly. |
Refactor the 5 bitmap operation functions to improve maintainability: - bitmap_contains - bitmap_min - bitmap_max - bitmap_has_any - bitmap_has_all Key changes: - ContainerReader: read and operate on a roaring container - SmallReader: same as ContainerReader but for HybridBitmap::Small - ContainerVisitor: merge traversal of two serialized treemaps - ContainerHandler: trait abstract container handle logic, used by ContainerVisitor - HasAnyHandler/HasAllHandler: impl ContainerHandler for bitmap_has_any/all - Tests restructured to compare against RoaringTreemap as ground truth, sharing fixtures across all tests, covering combinations of: format, container type, set relationship, and key alignment.
This commit introduced [proptests](https://docs.rs/proptest) for newly introduced: - bitmap_contains - bitmap_min - bitmap_max - bitmap_has_any - bitmap_has_all Each compares the result against RoaringTreemap as ground truth. Strategy design inspired by roaring as: - Store (random bits) - Container (50% Array, 50% Bitmap) - Bitmap (0 to 16 containers) - Tree (0 to 16 Bitmaps) - Serialization (80% Hybrid, 10% Legacy, 10% Empty)
6369790 to
4ae69c5
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ae69c5513
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… Small Previously, when bitmap_has_all got a Small lhs and a Large rhs, it simply return false, since a set with fewer cardinality can't has_all another one with higher cardinality. But due to we don't always demote Large to Small after cardinality reducing operations, that's not true anymore. As a defense, we removed this branch, and the doubting rhs will fall into branch below, where rhs cardinality is actually checked before returning false.
4651515 to
2cf4a7e
Compare
🤖 CI Job Analysis (Retry 1)
📊 Summary
❌ NO RETRY NEEDEDAll failures appear to be code/test issues requiring manual fixes. 🔍 Job Details
🤖 AboutAutomated analysis using job annotations to distinguish infrastructure issues (auto-retried) from code/test issues (manual fixes needed). |
|
That was a make lint error. Resolved. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cf4a7e834
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if as_roaring(lhs).is_none() && as_roaring(rhs).is_some() { | ||
| let lhs_small = SmallReader::new(lhs)?; | ||
| // Fast path: rhs has more values than lhs, impossible to contain | ||
| if reader::bitmap_len_above(rhs, lhs_small.len())? { |
There was a problem hiding this comment.
The follow-up still passes the full Hybrid buffer here, including the HB header, but reader::bitmap_len_above expects a RoaringTreemap payload like the slice returned by as_roaring(rhs). In the HybridSmall-lhs/HybridLarge-rhs path, common inputs such as bitmap_has_all(build_bitmap(0..31), build_bitmap(0..50000)) start decoding from the header bytes and return a BadBytes error before the fallback deserialization, instead of returning false (or checking a small HybridLarge rhs accurately). Use the bound Roaring slice for this length check.
Useful? React with 👍 / 👎.
| fn new(buf: &'a [u8]) -> Result<Self> { | ||
| let payload = &buf[HYBRID_HEADER_LEN..]; | ||
| let (len, bytes) = decode_small_payload(payload)?; | ||
| Ok(Self { len, values: bytes }) |
There was a problem hiding this comment.
This reader keeps the raw HybridSmall payload order, while deserialize_bitmap accepts the same payload by inserting each value into a sorted, de-duplicated small set. For persisted HybridSmall bytes with values like [5, 1] or duplicates, the new bitmap_min/bitmap_max/bitmap_contains/set predicates now use first/last/binary-search/two-pointer logic on non-canonical bytes and can return results that differ from the previous deserialize-based implementation. Either validate sorted unique payloads before these fast paths or decode through the same normalization used by decode_small_values.
Useful? React with 👍 / 👎.
…ice to bitmap_len_above SmallReader previously used decode_small_payload which returns raw that may contain duplicates or be unsorted. Switch to decode_small_values to deduplicate and sort, matching the deserialize path. Pass the roaring slice correctly to reader::bitmap_len_above. The previous code passed the HybridLarge buffer with its HB header, causing a BadBytes error.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70fd795f4c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if is_hybrid_large(buf) { | ||
| Some(&buf[HYBRID_HEADER_LEN..]) | ||
| } else if !is_hybrid(buf) { | ||
| Some(buf) // Legacy |
There was a problem hiding this comment.
When this legacy path sends every non-Hybrid buffer into reader::*, valid RoaringTreemap payloads encoded with the standard SERIAL_COOKIE form now fail because BitmapReader::decode rejects that cookie as does not support run containers. These legacy bytes were previously accepted by deserialize_bitmap, so bitmap_contains/bitmap_min/bitmap_max/set predicates regress for imported or persisted standard Roaring data; fall back to deserialization or add support for that cookie before taking this path.
Useful? React with 👍 / 👎.
| /// Different from [`validate_serialized_bitmap`], this one does not check payload to be | ||
| /// lightweight. |
There was a problem hiding this comment.
Only checking the 8-byte treemap prefix lets non-empty Large/legacy buffers with truncated payloads bypass the old deserialize_bitmap validation: for example, bytes equal to 1u64.to_le_bytes() declare one bucket but have no bucket data, yet the new readers treat them as empty, so bitmap_has_all(valid, corrupt_rhs) returns true and bitmap_min(corrupt) reports an empty bitmap instead of BadBytes. Please validate the Roaring payload or make the reader consume the declared bucket count before returning fast-path results.
Useful? React with 👍 / 👎.
2 participants
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Background
Issue [#19019](#19019) identified deserialization as the bottleneck for
BITMAP_INTERSECT. A series of PRs addressed this progressively:HybridBitmap(Small/Large dual-mode, <=16 elements on stack), improvingbitmap_intersect~3x.small_union/small_symmetric_differencein-place, improvingbitmap_union~27%.bitmap_countandbitmap_intersectfor the Large variant, relying on upstream [roaring-rs #281](Direct intersections with serialized RoaringBitmaps RoaringBitmap/roaring-rs#281).operate_bufpasses serialized RHS directly), improvingbitmap_intersect_empty81%,bitmap_xor_agg~68%.These PRs laid the foundation for computing on serialized bytes, follow-up work left as a good first issue tracked under [#19101](#19101). This PR picks up that work.
Summary
Implement zero-deserialization fast paths for five scalar bitmap functions on the
HybridBitmap::Largevariant (bitmap_contains,bitmap_min,bitmap_max,bitmap_has_any,bitmap_has_all). These functions now compute results by reading RoaringFormatSpec serialized bytes directly, avoiding full deserialization.Zero-deserialization operations
bitmap_contains: intersects a single-elementRoaringTreemapwith the serialized bufferbitmap_min/bitmap_max: reads the first/last value from the first/last containerbitmap_has_any/bitmap_has_all: container-level merge-join between two serialized bitmaps if both are large, if one side is large then deserialize and iterate the smaller side, then probe the larger side, fallback to deserialize both side as last sort.Fast-path
Before entering the costly intersection path, several checks reject impossible results at zero cost:
bitmap_has_any(empty, _) => falsebitmap_has_any(lhs, rhs) => falseiflhs.max < rhs.min || lhs.min > rhs.maxbitmap_has_all(lhs, rhs) => falseiflhs.min > rhs.min || lhs.max < rhs.maxbitmap_has_all(lhs, rhs) => falseiflhs.len < rhs.lenSupporting infrastructure
BitmapStats: a struct holding{len, min, max}computed once from serialized bytes, reused bybitmap_has_any/bitmap_has_allfor range and cardinality rejectionTests & benchmarks
bitmap.rs:test_bitmap_contains,test_bitmap_min,test_bitmap_max,test_bitmap_has_any,test_bitmap_stats,test_bitmap_has_all-- covering HybridLarge, HybridSmall, Legacy, and empty-bitmap cases.bitmap_scalargroup:bitmap_contains(large + small),bitmap_min(large + small),bitmap_max(large + small),bitmap_has_any(5 variants including disjoint),bitmap_has_all(5 variants including disjoint).Performance
Benchmarks collected from bench (
cargo bench -p databend-common-functions --bench bench -- bitmap_scalar), taking median values for compare:Remaining work
bitmap_or/bitmap_xor/bitmap_notLarge x SerializedLarge path: blocked on upstream [roaring-rs PR #343](Union with serialized bitmap RoaringBitmap/roaring-rs#343) (union_with_serialized_unchecked). The PR is open and awaiting further refactoring per maintainer feedback.Tests
Type of change
This change is