From 002ccd92e35127766d0da5a72e78a4d3b6722f1c Mon Sep 17 00:00:00 2001 From: xuanyili Date: Sat, 5 Sep 2026 21:15:39 +0000 Subject: [PATCH] fix: support all non-negative int64 deletion vector positions --- src/iceberg/deletes/dv_writer.cc | 9 +- src/iceberg/deletes/position_delete_index.cc | 19 ++- src/iceberg/deletes/position_delete_index.h | 8 + .../deletes/position_delete_range_consumer.cc | 10 +- .../deletes/roaring_position_bitmap.cc | 73 ++++----- src/iceberg/deletes/roaring_position_bitmap.h | 23 +-- src/iceberg/test/dv_writer_test.cc | 10 +- .../test/position_delete_index_test.cc | 22 +++ .../position_delete_range_consumer_test.cc | 15 +- .../test/roaring_position_bitmap_test.cc | 151 +++++++++++------- 10 files changed, 196 insertions(+), 144 deletions(-) diff --git a/src/iceberg/deletes/dv_writer.cc b/src/iceberg/deletes/dv_writer.cc index a51407fd3..b9c7fb9b9 100644 --- a/src/iceberg/deletes/dv_writer.cc +++ b/src/iceberg/deletes/dv_writer.cc @@ -31,7 +31,6 @@ #include "iceberg/deletes/dv_util_internal.h" #include "iceberg/deletes/position_delete_index.h" -#include "iceberg/deletes/roaring_position_bitmap.h" #include "iceberg/file_format.h" #include "iceberg/file_io.h" // IWYU pragma: keep #include "iceberg/manifest/manifest_entry.h" @@ -79,9 +78,7 @@ class DVWriter::Impl { ICEBERG_PRECHECK(!referenced_data_file.empty(), "Deletion vector requires a non-empty referenced data file"); ICEBERG_PRECHECK(spec != nullptr, "Deletion vector requires a partition spec"); - ICEBERG_PRECHECK(pos >= 0 && pos <= RoaringPositionBitmap::kMaxPosition, - "Deletion vector position out of range [0, {}]: {}", - RoaringPositionBitmap::kMaxPosition, pos); + ICEBERG_PRECHECK(pos >= 0, "Deletion vector position must be non-negative: {}", pos); DeletesFor(referenced_data_file, spec, partition).positions.Delete(pos); return {}; } @@ -112,6 +109,10 @@ class DVWriter::Impl { ICEBERG_RETURN_UNEXPECTED(LoadPreviousDeletes(path, deletes)); } + for (auto& [_, deletes] : deletes_by_path_) { + ICEBERG_RETURN_UNEXPECTED(deletes.positions.PrepareForSerialization()); + } + ICEBERG_ASSIGN_OR_RAISE(auto output_file, options_.io->NewOutputFile(options_.path)); const std::string output_path(options_.path); ICEBERG_ASSIGN_OR_RAISE( diff --git a/src/iceberg/deletes/position_delete_index.cc b/src/iceberg/deletes/position_delete_index.cc index 53e33e635..30cabfb62 100644 --- a/src/iceberg/deletes/position_delete_index.cc +++ b/src/iceberg/deletes/position_delete_index.cc @@ -49,6 +49,7 @@ constexpr std::array kMagic = {0xD1, 0xD3, 0x39, 0x64}; constexpr int32_t kLengthPrefixBytes = 4; constexpr int32_t kMagicBytes = 4; constexpr int32_t kCrcBytes = 4; +constexpr size_t kMaxSerializedLength = std::numeric_limits::max(); uint32_t ComputeCrc32(std::span bytes) { uLong crc = crc32(0L, Z_NULL, 0); @@ -142,16 +143,22 @@ void PositionDeleteIndex::Merge(const PositionDeleteIndex& other) { other.delete_files_.end()); } -Result> PositionDeleteIndex::Serialize() { +Result PositionDeleteIndex::PrepareForSerialization() { bitmap_.Optimize(); // run-length encode before serializing - std::vector blob(kLengthPrefixBytes); - blob.insert(blob.end(), kMagic.begin(), kMagic.end()); - ICEBERG_ASSIGN_OR_RAISE(const auto vector_size, bitmap_.SerializeTo(blob)); - + const size_t vector_size = bitmap_.SerializedSizeInBytes(); const size_t magic_and_vector_size = kMagicBytes + vector_size; - ICEBERG_PRECHECK(magic_and_vector_size <= std::numeric_limits::max(), + ICEBERG_PRECHECK(magic_and_vector_size <= kMaxSerializedLength, "Deletion vector is too large to serialize: {} bytes", magic_and_vector_size); + return magic_and_vector_size; +} + +Result> PositionDeleteIndex::Serialize() { + ICEBERG_ASSIGN_OR_RAISE(const auto magic_and_vector_size, PrepareForSerialization()); + + std::vector blob(kLengthPrefixBytes); + blob.insert(blob.end(), kMagic.begin(), kMagic.end()); + ICEBERG_RETURN_UNEXPECTED(bitmap_.SerializeTo(blob)); WriteBigEndian(static_cast(magic_and_vector_size), blob.data()); const auto crc_offset = blob.size(); diff --git a/src/iceberg/deletes/position_delete_index.h b/src/iceberg/deletes/position_delete_index.h index 6f301210e..04690a4d3 100644 --- a/src/iceberg/deletes/position_delete_index.h +++ b/src/iceberg/deletes/position_delete_index.h @@ -22,6 +22,7 @@ /// \file iceberg/deletes/position_delete_index.h /// Index of deleted row positions for a data file. +#include #include #include #include @@ -34,6 +35,8 @@ namespace iceberg { +class DVWriter; + /// \brief Tracks deleted row positions using a bitmap. /// /// This class provides a domain-specific API for position deletes @@ -53,6 +56,8 @@ class ICEBERG_EXPORT PositionDeleteIndex { /// \brief Mark a range of positions as deleted [pos_start, pos_end). /// \param pos_start Start position (inclusive) /// \param pos_end End position (exclusive) + /// \note Because pos_end is an int64_t exclusive endpoint, this method cannot + /// include INT64_MAX. Call Delete(INT64_MAX) separately. void Delete(int64_t pos_start, int64_t pos_end); /// \brief Check if a position is deleted. @@ -97,6 +102,8 @@ class ICEBERG_EXPORT PositionDeleteIndex { private: explicit PositionDeleteIndex(RoaringPositionBitmap bitmap); + Result PrepareForSerialization(); + // Bulk-add positions sharing high-32-bit `key`. Private hook for // `ForEachPositionDelete`'s bulk path; keeps `Delete` the sole public // mutation surface. @@ -105,6 +112,7 @@ class ICEBERG_EXPORT PositionDeleteIndex { friend void ICEBERG_EXPORT ForEachPositionDelete(std::span positions, PositionDeleteIndex& target, std::vector& scratch); + friend class DVWriter; RoaringPositionBitmap bitmap_; std::vector> delete_files_; diff --git a/src/iceberg/deletes/position_delete_range_consumer.cc b/src/iceberg/deletes/position_delete_range_consumer.cc index f7cf258c3..6362a4371 100644 --- a/src/iceberg/deletes/position_delete_range_consumer.cc +++ b/src/iceberg/deletes/position_delete_range_consumer.cc @@ -31,9 +31,7 @@ namespace iceberg { namespace { -bool IsValidPosition(int64_t pos) { - return pos >= 0 && pos <= RoaringPositionBitmap::kMaxPosition; -} +bool IsValidPosition(int64_t pos) { return pos >= 0; } // Unsigned subtraction so negative or wrap-around input can't // false-positive via signed overflow. @@ -45,11 +43,13 @@ bool IsAdjacent(int64_t prev, int64_t next) { // bulk path groups by this key before flushing via `BulkAddForKey`. int32_t HighKeyFromPosition(int64_t pos) { return static_cast(pos >> 32); } -// Emit `[range_start, last_position]`, collapsing singletons. Callers -// pre-filter via `IsValidPosition`, so `last_position + 1` cannot overflow. +// Emit `[range_start, last_position]`, collapsing singletons. void EmitRange(PositionDeleteIndex& target, int64_t range_start, int64_t last_position) { if (range_start == last_position) { target.Delete(range_start); + } else if (last_position == RoaringPositionBitmap::kMaxPosition) { + target.Delete(range_start, last_position); + target.Delete(last_position); } else { target.Delete(range_start, last_position + 1); } diff --git a/src/iceberg/deletes/roaring_position_bitmap.cc b/src/iceberg/deletes/roaring_position_bitmap.cc index a2827d4bb..b05bb20c0 100644 --- a/src/iceberg/deletes/roaring_position_bitmap.cc +++ b/src/iceberg/deletes/roaring_position_bitmap.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -51,34 +52,21 @@ int64_t ToPosition(int32_t key, uint32_t pos32) { return (int64_t{key} << 32) | int64_t{pos32}; } -Status ValidatePosition(int64_t pos) { - if (pos < 0 || pos > RoaringPositionBitmap::kMaxPosition) { - return InvalidArgument("Bitmap supports positions that are >= 0 and <= {}: {}", - RoaringPositionBitmap::kMaxPosition, pos); - } - return {}; -} - -void WriteBitmaps(const std::vector& bitmaps, uint8_t* buf) { +void WriteBitmaps(const std::map& bitmaps, uint8_t* buf) { WriteLittleEndian(static_cast(bitmaps.size()), buf); buf += kBitmapCountSizeBytes; - for (int32_t key = 0; std::cmp_less(key, bitmaps.size()); ++key) { + for (const auto& [key, bitmap] : bitmaps) { WriteLittleEndian(key, buf); buf += kBitmapKeySizeBytes; - buf += bitmaps[key].write(reinterpret_cast(buf), /*portable=*/true); + buf += bitmap.write(reinterpret_cast(buf), /*portable=*/true); } } } // namespace struct RoaringPositionBitmap::Impl { - std::vector bitmaps; - - void AllocateBitmapsIfNeeded(int32_t required_length) { - if (std::cmp_less(bitmaps.size(), required_length)) { - bitmaps.resize(static_cast(required_length)); - } - } + // Empty buckets are never retained. + std::map bitmaps; }; RoaringPositionBitmap::RoaringPositionBitmap() : impl_(std::make_unique()) {} @@ -108,24 +96,24 @@ RoaringPositionBitmap::RoaringPositionBitmap(std::unique_ptr impl) : impl_(std::move(impl)) {} void RoaringPositionBitmap::Add(int64_t pos) { - if (pos < 0 || pos > kMaxPosition) { + if (pos < 0) { return; // Silently ignore invalid positions } int32_t key = Key(pos); uint32_t pos32 = Pos32Bits(pos); - impl_->AllocateBitmapsIfNeeded(key + 1); impl_->bitmaps[key].add(pos32); } void RoaringPositionBitmap::AddManyForKey(int32_t key, std::span positions) { - impl_->AllocateBitmapsIfNeeded(key + 1); + if (key < 0 || positions.empty()) { + return; + } impl_->bitmaps[key].addMany(positions.size(), positions.data()); } void RoaringPositionBitmap::AddRange(int64_t pos_start, int64_t pos_end) { pos_start = std::max(pos_start, int64_t{0}); - pos_end = std::min(pos_end, kMaxPosition + 1); if (pos_start >= pos_end) { return; } @@ -133,61 +121,60 @@ void RoaringPositionBitmap::AddRange(int64_t pos_start, int64_t pos_end) { int64_t pos_last = pos_end - 1; int32_t start_key = Key(pos_start); int32_t end_key = Key(pos_last); - impl_->AllocateBitmapsIfNeeded(end_key + 1); - for (int32_t key = start_key; key <= end_key; ++key) { + for (int64_t key = start_key; key <= end_key; ++key) { uint64_t low_start = (key == start_key) ? Pos32Bits(pos_start) : uint64_t{0}; uint64_t low_end = (key == end_key) ? static_cast(Pos32Bits(pos_last)) + 1 : (uint64_t{1} << 32); - impl_->bitmaps[key].addRange(low_start, low_end); + impl_->bitmaps[static_cast(key)].addRange(low_start, low_end); } } bool RoaringPositionBitmap::Contains(int64_t pos) const { - if (pos < 0 || pos > kMaxPosition) { + if (pos < 0) { return false; // Invalid positions are not contained } int32_t key = Key(pos); uint32_t pos32 = Pos32Bits(pos); - return std::cmp_less(key, impl_->bitmaps.size()) && impl_->bitmaps[key].contains(pos32); + auto it = impl_->bitmaps.find(key); + return it != impl_->bitmaps.end() && it->second.contains(pos32); } -bool RoaringPositionBitmap::IsEmpty() const { return Cardinality() == 0; } +bool RoaringPositionBitmap::IsEmpty() const { return impl_->bitmaps.empty(); } size_t RoaringPositionBitmap::Cardinality() const { size_t total = 0; - for (const auto& bitmap : impl_->bitmaps) { + for (const auto& [_, bitmap] : impl_->bitmaps) { total += bitmap.cardinality(); } return total; } void RoaringPositionBitmap::Or(const RoaringPositionBitmap& other) { - impl_->AllocateBitmapsIfNeeded(static_cast(other.impl_->bitmaps.size())); - for (size_t key = 0; key < other.impl_->bitmaps.size(); ++key) { - impl_->bitmaps[key] |= other.impl_->bitmaps[key]; + for (const auto& [key, bitmap] : other.impl_->bitmaps) { + impl_->bitmaps[key] |= bitmap; } } bool RoaringPositionBitmap::Optimize() { bool changed = false; - for (auto& bitmap : impl_->bitmaps) { + for (auto& [_, bitmap] : impl_->bitmaps) { changed |= bitmap.runOptimize(); } return changed; } void RoaringPositionBitmap::ForEach(const std::function& fn) const { - for (size_t key = 0; key < impl_->bitmaps.size(); ++key) { - for (uint32_t pos32 : impl_->bitmaps[key]) { - fn(ToPosition(static_cast(key), pos32)); + for (const auto& [key, bitmap] : impl_->bitmaps) { + for (uint32_t pos32 : bitmap) { + fn(ToPosition(key, pos32)); } } } size_t RoaringPositionBitmap::SerializedSizeInBytes() const { size_t size = kBitmapCountSizeBytes; - for (const auto& bitmap : impl_->bitmaps) { + for (const auto& [_, bitmap] : impl_->bitmaps) { size += kBitmapKeySizeBytes + bitmap.getSizeInBytes(/*portable=*/true); } return size; @@ -238,18 +225,10 @@ Result RoaringPositionBitmap::Deserialize(std::string_vie remaining -= kBitmapKeySizeBytes; ICEBERG_PRECHECK(key >= 0, "Invalid unsigned key: {}", key); - ICEBERG_PRECHECK(key < std::numeric_limits::max(), "Key is too large: {}", - key); ICEBERG_PRECHECK(key > last_key, "Keys must be sorted in ascending order, got key {} after {}", key, last_key); - // Fill gaps with empty bitmaps - while (last_key < key - 1) { - impl->bitmaps.emplace_back(); - ++last_key; - } - // Read bitmap using portable safe deserialization. // CRoaring's readSafe may throw on corrupted data. roaring::Roaring bitmap; @@ -266,7 +245,9 @@ Result RoaringPositionBitmap::Deserialize(std::string_vie buf += bitmap_size; remaining -= bitmap_size; - impl->bitmaps.emplace_back(std::move(bitmap)); + if (!bitmap.isEmpty()) { + impl->bitmaps.emplace(key, std::move(bitmap)); + } last_key = key; --remaining_count; } diff --git a/src/iceberg/deletes/roaring_position_bitmap.h b/src/iceberg/deletes/roaring_position_bitmap.h index 119387661..e3aee0b35 100644 --- a/src/iceberg/deletes/roaring_position_bitmap.h +++ b/src/iceberg/deletes/roaring_position_bitmap.h @@ -20,10 +20,11 @@ #pragma once /// \file iceberg/deletes/roaring_position_bitmap.h -/// A 64-bit position bitmap using an array of 32-bit Roaring bitmaps. +/// A 64-bit position bitmap using sparse 32-bit Roaring bitmaps. #include #include +#include #include #include #include @@ -37,7 +38,7 @@ namespace iceberg { class PositionDeleteIndex; -/// \brief A bitmap that supports positive 64-bit positions, optimized +/// \brief A bitmap that supports non-negative 64-bit positions, optimized /// for cases where most positions fit in 32 bits. /// /// Incoming 64-bit positions are divided into a 32-bit "key" using the @@ -51,8 +52,8 @@ class PositionDeleteIndex; /// for `deletion-vector-v1` persistence. class ICEBERG_EXPORT RoaringPositionBitmap { public: - /// \brief Maximum supported position (aligned with the Java implementation). - static constexpr int64_t kMaxPosition = 0x7FFFFFFE80000000LL; + /// \brief Maximum supported position. + static constexpr int64_t kMaxPosition = std::numeric_limits::max(); RoaringPositionBitmap(); ~RoaringPositionBitmap(); @@ -64,21 +65,21 @@ class ICEBERG_EXPORT RoaringPositionBitmap { RoaringPositionBitmap& operator=(const RoaringPositionBitmap& other); /// \brief Sets a position in the bitmap. - /// \param pos the position (must be >= 0 and <= kMaxPosition) - /// \note Invalid positions are silently ignored + /// \param pos the position (must be non-negative) + /// \note Negative positions are silently ignored. void Add(int64_t pos); /// \brief Sets a range of positions [pos_start, pos_end). /// \param pos_start the start of the range (inclusive), clamped to 0 - /// \param pos_end the end of the range (exclusive), clamped to kMaxPosition + 1 - /// \note If pos_start > pos_end, the call is silently ignored. - /// If pos_start == pos_end, this method does nothing. - /// Positions outside [0, kMaxPosition] are silently ignored. + /// \param pos_end the end of the range (exclusive) + /// \note Empty and reversed ranges are silently ignored. + /// \note Because pos_end is an int64_t exclusive endpoint, this method cannot + /// include kMaxPosition. Call Add(kMaxPosition) separately. void AddRange(int64_t pos_start, int64_t pos_end); /// \brief Checks if a position is set in the bitmap. /// \param pos the position to check - /// \return true if the position is set, false otherwise (including invalid positions) + /// \return true if the position is set, false otherwise (including negative positions) bool Contains(int64_t pos) const; /// \brief Returns true if the bitmap has no positions set. diff --git a/src/iceberg/test/dv_writer_test.cc b/src/iceberg/test/dv_writer_test.cc index 8eb4b2444..ac23c2973 100644 --- a/src/iceberg/test/dv_writer_test.cc +++ b/src/iceberg/test/dv_writer_test.cc @@ -332,18 +332,16 @@ TEST(DVWriterTest, DeleteRejectsEmptyReferencedFile) { IsError(ErrorKind::kInvalidArgument)); } -TEST(DVWriterTest, DeleteRejectsOutOfRangePosition) { +TEST(DVWriterTest, DeleteRejectsNegativeAndAcceptsMaximumPosition) { auto io = std::make_shared(); auto spec = UnpartitionedSpec(); ICEBERG_UNWRAP_OR_FAIL( auto writer, DVWriter::Make(MakeDVWriterOptions(io, "memory://invalid.puffin"))); - // Negative and out-of-range positions are rejected rather than silently - // dropped by the underlying bitmap. EXPECT_THAT(writer->Delete("data.parquet", -1, spec, PartitionValues{}), IsError(ErrorKind::kInvalidArgument)); - EXPECT_THAT(writer->Delete("data.parquet", RoaringPositionBitmap::kMaxPosition + 1, - spec, PartitionValues{}), - IsError(ErrorKind::kInvalidArgument)); + EXPECT_THAT(writer->Delete("data.parquet", RoaringPositionBitmap::kMaxPosition, spec, + PartitionValues{}), + IsOk()); } // Close propagates a load_previous_deletes failure and returns no metadata. diff --git a/src/iceberg/test/position_delete_index_test.cc b/src/iceberg/test/position_delete_index_test.cc index 1075ee6fa..92230c354 100644 --- a/src/iceberg/test/position_delete_index_test.cc +++ b/src/iceberg/test/position_delete_index_test.cc @@ -20,6 +20,7 @@ #include "iceberg/deletes/position_delete_index.h" #include +#include #include #include @@ -185,6 +186,27 @@ TEST(PositionDeleteIndexTest, TestLargePositions) { ASSERT_FALSE(index.IsDeleted(large_pos + 1)); } +TEST(PositionDeleteIndexTest, TestSparseDistantPositionsRoundTrip) { + PositionDeleteIndex index; + const std::vector positions = { + 1, + (int64_t{1} << 32) + 2, + std::numeric_limits::max(), + }; + for (auto pos : positions) { + index.Delete(pos); + } + + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + EXPECT_LT(blob.size(), 1024); + ICEBERG_UNWRAP_OR_FAIL(auto restored, PositionDeleteIndex::Deserialize( + blob, DeleteFileFor(blob, positions.size()))); + EXPECT_EQ(restored.Cardinality(), positions.size()); + for (auto pos : positions) { + EXPECT_TRUE(restored.IsDeleted(pos)); + } +} + TEST(PositionDeleteIndexTest, TestOverlappingRanges) { PositionDeleteIndex index; diff --git a/src/iceberg/test/position_delete_range_consumer_test.cc b/src/iceberg/test/position_delete_range_consumer_test.cc index 5a58fa5a2..59dccf22d 100644 --- a/src/iceberg/test/position_delete_range_consumer_test.cc +++ b/src/iceberg/test/position_delete_range_consumer_test.cc @@ -28,7 +28,6 @@ #include #include "iceberg/deletes/position_delete_index.h" -#include "iceberg/deletes/roaring_position_bitmap.h" namespace iceberg { @@ -38,7 +37,7 @@ namespace { std::set ExpectedValidSet(const std::vector& positions) { std::set expected; for (int64_t pos : positions) { - if (pos >= 0 && pos <= RoaringPositionBitmap::kMaxPosition) { + if (pos >= 0) { expected.insert(pos); } } @@ -95,12 +94,9 @@ TEST(PositionDeleteRangeConsumerTest, DuplicatesAreIdempotent) { TEST(PositionDeleteRangeConsumerTest, InvalidPositionsSilentlySkipped) { // Invalids at the edges, mid-run, and mixed with valid contiguous runs - // must all be dropped without breaking coalescing around them. We stay - // well below `kMaxPosition` to avoid forcing the bitmap to resize its - // backing vector to ~2^31 empty containers. + // must all be dropped without breaking coalescing around them. AssertMatchesBaseline({std::numeric_limits::min(), -5, -4, 10, 11, -999, 12, - 13, RoaringPositionBitmap::kMaxPosition + 1, - std::numeric_limits::max()}); + 13, std::numeric_limits::max()}); } TEST(PositionDeleteRangeConsumerTest, ContiguousRunAcrossKeyBoundary) { @@ -114,6 +110,11 @@ TEST(PositionDeleteRangeConsumerTest, ContiguousRunAcrossKeyBoundary) { AssertMatchesBaseline(positions); } +TEST(PositionDeleteRangeConsumerTest, ContiguousRunEndingAtMaximumPosition) { + const int64_t max = std::numeric_limits::max(); + AssertMatchesBaseline({max - 2, max - 1, max}); +} + TEST(PositionDeleteRangeConsumerTest, DispatcherAgreesAtBothDensities) { // Above the sniff threshold at densities below and above the 10% // cutoff. We can't observe the choice directly; agreement with the diff --git a/src/iceberg/test/roaring_position_bitmap_test.cc b/src/iceberg/test/roaring_position_bitmap_test.cc index 51f401b18..bf5b057ff 100644 --- a/src/iceberg/test/roaring_position_bitmap_test.cc +++ b/src/iceberg/test/roaring_position_bitmap_test.cc @@ -23,20 +23,25 @@ #include #include #include +#include #include #include #include #include #include +#include #include "iceberg/test/matchers.h" #include "iceberg/test/test_config.h" +#include "iceberg/util/endian.h" namespace iceberg { namespace { +constexpr size_t kBitmapCountSizeBytes = 8; +constexpr size_t kBitmapKeySizeBytes = 4; constexpr int64_t kBitmapSize = 0xFFFFFFFFL; constexpr int64_t kBitmapOffset = kBitmapSize + 1L; constexpr int64_t kContainerSize = 0xFFFF; // Character.MAX_VALUE @@ -174,13 +179,12 @@ TEST(RoaringPositionBitmapTest, TestAddRangeClampNegativeStart) { ASSERT_FALSE(bitmap.Contains(-1)); } -TEST(RoaringPositionBitmapTest, TestAddRangeClampBeyondMaxPosition) { +TEST(RoaringPositionBitmapTest, TestAddRangeWithMaximumExclusiveEnd) { RoaringPositionBitmap bitmap; - // Range entirely beyond kMaxPosition: after clamping both endpoints the range - // becomes empty, so no allocation or insertion happens. - bitmap.AddRange(RoaringPositionBitmap::kMaxPosition + 1, - RoaringPositionBitmap::kMaxPosition + 10); - ASSERT_TRUE(bitmap.IsEmpty()); + bitmap.AddRange(RoaringPositionBitmap::kMaxPosition - 1, + RoaringPositionBitmap::kMaxPosition); + ASSERT_TRUE(bitmap.Contains(RoaringPositionBitmap::kMaxPosition - 1)); + ASSERT_FALSE(bitmap.Contains(RoaringPositionBitmap::kMaxPosition)); } struct AddRangeNoOpParams { @@ -271,45 +275,21 @@ INSTANTIATE_TEST_SUITE_P( }), [](const ::testing::TestParamInfo& info) { return info.param.name; }); -enum class InteropBitmapShape { - kEmpty, - kOnly32BitPositions, - kSpreadAcrossKeys, -}; - struct InteropCase { const char* file_name; - InteropBitmapShape expected_shape; + size_t expected_cardinality; + std::vector expected_positions; + std::vector absent_positions; }; -void AssertInteropBitmapShape(const RoaringPositionBitmap& bitmap, - InteropBitmapShape expected_shape) { - bool saw_pos_lt_32_bit = false; - bool saw_pos_ge_32_bit = false; - - bitmap.ForEach([&](int64_t pos) { - if (pos < (int64_t{1} << 32)) { - saw_pos_lt_32_bit = true; - } else { - saw_pos_ge_32_bit = true; - } - }); - - switch (expected_shape) { - case InteropBitmapShape::kEmpty: - ASSERT_TRUE(bitmap.IsEmpty()); - ASSERT_EQ(bitmap.Cardinality(), 0u); - break; - case InteropBitmapShape::kOnly32BitPositions: - ASSERT_GT(bitmap.Cardinality(), 0u); - ASSERT_TRUE(saw_pos_lt_32_bit); - ASSERT_FALSE(saw_pos_ge_32_bit); - break; - case InteropBitmapShape::kSpreadAcrossKeys: - ASSERT_GT(bitmap.Cardinality(), 0u); - ASSERT_TRUE(saw_pos_lt_32_bit); - ASSERT_TRUE(saw_pos_ge_32_bit); - break; +void AssertInteropCase(const RoaringPositionBitmap& bitmap, + const InteropCase& test_case) { + ASSERT_EQ(bitmap.Cardinality(), test_case.expected_cardinality); + for (int64_t pos : test_case.expected_positions) { + ASSERT_TRUE(bitmap.Contains(pos)) << "Missing position: " << pos; + } + for (int64_t pos : test_case.absent_positions) { + ASSERT_FALSE(bitmap.Contains(pos)) << "Unexpected position: " << pos; } } @@ -342,7 +322,7 @@ TEST(RoaringPositionBitmapTest, TestAddPositionsRequiringMultipleBitmaps) { bitmap.Add(pos4); AssertEqualContent(bitmap, {pos1, pos2, pos3, pos4}); - ASSERT_EQ(bitmap.SerializedSizeInBytes(), 1260); + ASSERT_LT(bitmap.SerializedSizeInBytes(), 128); } TEST(RoaringPositionBitmapTest, TestAddEmptyRange) { @@ -424,6 +404,59 @@ TEST(RoaringPositionBitmapTest, TestSerializeDeserializeEmpty) { ASSERT_EQ(copy.Cardinality(), 0); } +TEST(RoaringPositionBitmapTest, TestSerializeOmitsEmptyBuckets) { + roaring::Roaring empty_bucket; + std::string bytes(kBitmapCountSizeBytes + sizeof(int32_t) + + empty_bucket.getSizeInBytes(/*portable=*/true), + '\0'); + WriteLittleEndian(int64_t{1}, bytes.data()); + WriteLittleEndian(int32_t{7}, bytes.data() + kBitmapCountSizeBytes); + empty_bucket.write(bytes.data() + kBitmapCountSizeBytes + sizeof(int32_t), + /*portable=*/true); + + ICEBERG_UNWRAP_OR_FAIL(auto bitmap, RoaringPositionBitmap::Deserialize(bytes)); + ICEBERG_UNWRAP_OR_FAIL(auto serialized, bitmap.Serialize()); + ASSERT_EQ(serialized.size(), kBitmapCountSizeBytes); + ASSERT_EQ(ReadLittleEndian(serialized.data()), 0); +} + +TEST(RoaringPositionBitmapTest, TestSerializeOrdersKeysAscending) { + RoaringPositionBitmap bitmap; + bitmap.Add(std::numeric_limits::max()); + bitmap.Add((int64_t{100} << 32) | 9); + bitmap.Add(std::numeric_limits::max()); + bitmap.Add((int64_t{1} << 32) | 7); + bitmap.Add(3); + + ICEBERG_UNWRAP_OR_FAIL(auto serialized, bitmap.Serialize()); + const char* cursor = serialized.data(); + size_t remaining = serialized.size(); + ASSERT_GE(remaining, kBitmapCountSizeBytes); + ASSERT_EQ(ReadLittleEndian(cursor), 4); + cursor += kBitmapCountSizeBytes; + remaining -= kBitmapCountSizeBytes; + + std::vector keys; + const std::vector expected_cardinalities = {2, 1, 1, 1}; + while (keys.size() < 4) { + ASSERT_GE(remaining, kBitmapKeySizeBytes); + keys.push_back(ReadLittleEndian(cursor)); + cursor += kBitmapKeySizeBytes; + remaining -= kBitmapKeySizeBytes; + + auto bucket = roaring::Roaring::readSafe(cursor, remaining); + ASSERT_EQ(bucket.cardinality(), expected_cardinalities[keys.size() - 1]); + const size_t bucket_size = bucket.getSizeInBytes(/*portable=*/true); + ASSERT_LE(bucket_size, remaining); + cursor += bucket_size; + remaining -= bucket_size; + } + + EXPECT_EQ(keys, (std::vector{0, 1, 100, std::numeric_limits::max()})); + EXPECT_EQ(keys.back(), std::numeric_limits::max()); + EXPECT_EQ(remaining, 0u); +} + TEST(RoaringPositionBitmapTest, TestSerializeDeserializeAllContainerBitmap) { RoaringPositionBitmap bitmap; @@ -509,21 +542,18 @@ TEST(RoaringPositionBitmapTest, TestOptimize) { AssertEqualContent(copy, expected_positions); } -TEST(RoaringPositionBitmapTest, TestUnsupportedPositions) { +TEST(RoaringPositionBitmapTest, TestPositionBounds) { RoaringPositionBitmap bitmap; // Negative position bitmap.Add(-1L); ASSERT_FALSE(bitmap.Contains(-1L)); - // Contains with negative position - - // Position exceeding MAX_POSITION - should be silently ignored - bitmap.Add(RoaringPositionBitmap::kMaxPosition + 1L); - ASSERT_FALSE(bitmap.Contains(RoaringPositionBitmap::kMaxPosition + 1L)); + bitmap.Add(RoaringPositionBitmap::kMaxPosition); + ASSERT_TRUE(bitmap.Contains(RoaringPositionBitmap::kMaxPosition)); - // Contains with position exceeding MAX_POSITION - should return false - ASSERT_FALSE(bitmap.Contains(RoaringPositionBitmap::kMaxPosition + 1L)); + auto copy = RoundTripSerialize(bitmap); + ASSERT_TRUE(copy.Contains(RoaringPositionBitmap::kMaxPosition)); } TEST(RoaringPositionBitmapTest, TestRandomSparseBitmap) { @@ -612,10 +642,17 @@ TEST(RoaringPositionBitmapInteropTest, TestDeserializeSupportedRoaringExamples) // roaring position bitmap interoperability test resources. static const std::vector kCases = { {.file_name = "64map32bitvals.bin", - .expected_shape = InteropBitmapShape::kOnly32BitPositions}, - {.file_name = "64mapempty.bin", .expected_shape = InteropBitmapShape::kEmpty}, + .expected_cardinality = 10, + .expected_positions = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}, + .absent_positions = {10}}, + {.file_name = "64mapempty.bin", + .expected_cardinality = 0, + .expected_positions = {}, + .absent_positions = {0}}, {.file_name = "64mapspreadvals.bin", - .expected_shape = InteropBitmapShape::kSpreadAcrossKeys}, + .expected_cardinality = 100, + .expected_positions = {0, (int64_t{3} << 32) | 7, (int64_t{9} << 32) | 9}, + .absent_positions = {int64_t{10} << 32}}, }; for (const auto& test_case : kCases) { @@ -625,14 +662,10 @@ TEST(RoaringPositionBitmapInteropTest, TestDeserializeSupportedRoaringExamples) ASSERT_THAT(result, IsOk()); const auto& bitmap = result.value(); - AssertInteropBitmapShape(bitmap, test_case.expected_shape); - - std::set positions; - bitmap.ForEach([&](int64_t pos) { positions.insert(pos); }); - AssertEqualContent(bitmap, positions); + AssertInteropCase(bitmap, test_case); auto copy = RoundTripSerialize(bitmap); - AssertEqualContent(copy, positions); + AssertInteropCase(copy, test_case); } }