Skip to content

Commit 29bbf63

Browse files
committed
fix: stop assuming HybridBitmap::Large having higher cardinality than 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.
1 parent 33fabbd commit 29bbf63

1 file changed

Lines changed: 5 additions & 7 deletions

File tree

src/common/io/src/bitmap.rs

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -947,16 +947,14 @@ pub fn bitmap_has_all(lhs: &[u8], rhs: &[u8]) -> Result<bool> {
947947
return Ok(lhs.has_all_with(&rhs));
948948
}
949949

950-
// Fast path: HybridSmall lhs cannot contain HybridLarge rhs
951-
if as_roaring(lhs).is_none() && is_hybrid_large(rhs) {
952-
return Ok(false);
953-
}
954-
955-
// Fast path: HybridSmall lhs, Legacy rhs — cardinality check
950+
// Fast path: HybridSmall lhs, HybridLarge or Legacy rhs cardinality check
951+
// We need to check cardinality here because: (1) it might be a legacy tree,
952+
// and (2) we don't always demote HybridBitmap after reducing its cardinality immediately.
956953
if as_roaring(lhs).is_none() && as_roaring(rhs).is_some() {
957954
let lhs_small = SmallReader::new(lhs)?;
955+
let rhs_roaring = as_roaring(rhs).unwrap();
958956
// Fast path: rhs has more values than lhs, impossible to contain
959-
if reader::bitmap_len_above(rhs, lhs_small.len())? {
957+
if reader::bitmap_len_above(rhs_roaring, lhs_small.len())? {
960958
return Ok(false);
961959
}
962960
// rhs has <= lhs_small.len() values, but still need to check containment

0 commit comments

Comments
 (0)