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 9 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. |
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".
4651515 to
2cf4a7e
Compare
|
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".
|
@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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a42ce85e70
ℹ️ 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".
a42ce85 to
da16bd4
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da16bd4ccf
ℹ️ 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".
| let header = BitmapReader::decode(probe)?; | ||
| probe = &probe[header.buf.len()..]; |
There was a problem hiding this comment.
If a legacy/HybridLarge payload contains out-of-order or duplicate treemap bucket prefixes, this loop only decodes each bucket and preserves the raw order, while the old deserialize_bitmap path normalized prefixes through RoaringTreemap's BTreeMap semantics. For example, a payload with two prefix-0 buckets {1} then {2} used to behave as {2}, but the new bitmap_contains returns from the first bucket and reports that 1 is present; min/max and the set predicates have analogous mismatches. Please reject or normalize non-canonical prefix order before taking the zero-copy fast paths.
Useful? React with 👍 / 👎.
da16bd4 to
4c47c52
Compare
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)
… 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.
4c47c52 to
29bbf63
Compare
|
I pushed three patches, with the following changes:
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? |
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