address comments and polish test

Author: wgtmac

src/iceberg/deletes/roaring_position_bitmap.cc

@@ -41,12 +41,12 @@ constexpr size_t kBitmapKeySizeBytes = 4;
 int32_t Key(int64_t pos) { return static_cast<int32_t>(pos >> 32); }
 
 // Extracts low 32 bits from a 64-bit position.
-uint32_t Pos32Bits(int64_t pos) { return static_cast<uint32_t>(pos); }
+uint32_t Pos32Bits(int64_t pos) { return static_cast<uint32_t>(0xFFFFFFFF & pos); }
 
 // Combines key (high 32 bits) and pos32 (low 32 bits) into a 64-bit
 // position. The low 32 bits are zero-extended to avoid sign extension.
 int64_t ToPosition(int32_t key, uint32_t pos32) {
-  return (static_cast<int64_t>(key) << 32) | static_cast<int64_t>(pos32);
+  return (int64_t{key} << 32) | int64_t{pos32};
 }
 
 void WriteLE64(char* buf, int64_t value) {
@@ -95,6 +95,20 @@ RoaringPositionBitmap::RoaringPositionBitmap() : impl_(std::make_unique<Impl>())
 
 RoaringPositionBitmap::~RoaringPositionBitmap() = default;
 
+RoaringPositionBitmap::RoaringPositionBitmap(const RoaringPositionBitmap& other)
+    : impl_(other.impl_ != nullptr ? std::make_unique<Impl>(*other.impl_)
+                                   : std::make_unique<Impl>()) {}
+
+RoaringPositionBitmap& RoaringPositionBitmap::operator=(
+    const RoaringPositionBitmap& other) {
+  if (this == &other) {
+    return *this;
+  }
+  impl_ = other.impl_ != nullptr ? std::make_unique<Impl>(*other.impl_)
+                                 : std::make_unique<Impl>();
+  return *this;
+}
+
 RoaringPositionBitmap::RoaringPositionBitmap(RoaringPositionBitmap&&) noexcept = default;
 
 RoaringPositionBitmap& RoaringPositionBitmap::operator=(
@@ -167,6 +181,8 @@ size_t RoaringPositionBitmap::SerializedSizeInBytes() const {
   return size;
 }
 
+// Serializes using the portable format (little-endian).
+// See https://iceberg.apache.org/puffin-spec/#deletion-vector-v1-blob-type
 Result<std::string> RoaringPositionBitmap::Serialize() const {
   size_t size = SerializedSizeInBytes();
   std::string result(size, '\0');
@@ -243,7 +259,7 @@ Result<RoaringPositionBitmap> RoaringPositionBitmap::Deserialize(std::string_vie
     buf += bitmap_size;
     remaining -= bitmap_size;
 
-    impl->bitmaps.push_back(std::move(bitmap));
+    impl->bitmaps.emplace_back(std::move(bitmap));
     last_key = key;
     --remaining_count;
   }

src/iceberg/deletes/roaring_position_bitmap.h

@@ -46,7 +46,7 @@ namespace iceberg {
 /// this class.
 class ICEBERG_EXPORT RoaringPositionBitmap {
  public:
-  /// \brief Maximum supported position.
+  /// \brief Maximum supported position (aligned with the Java implementation).
   static constexpr int64_t kMaxPosition = 0x7FFFFFFE80000000LL;
 
   RoaringPositionBitmap();
@@ -55,8 +55,8 @@ class ICEBERG_EXPORT RoaringPositionBitmap {
   RoaringPositionBitmap(RoaringPositionBitmap&& other) noexcept;
   RoaringPositionBitmap& operator=(RoaringPositionBitmap&& other) noexcept;
 
-  RoaringPositionBitmap(const RoaringPositionBitmap&) = delete;
-  RoaringPositionBitmap& operator=(const RoaringPositionBitmap&) = delete;
+  RoaringPositionBitmap(const RoaringPositionBitmap& other);
+  RoaringPositionBitmap& operator=(const RoaringPositionBitmap& other);
 
   /// \brief Sets a position in the bitmap.
   /// \param pos the position (must be >= 0 and <= kMaxPosition)

src/iceberg/test/CMakeLists.txt

@@ -115,6 +115,7 @@ add_iceberg_test(util_test
                  endian_test.cc
                  formatter_test.cc
                  location_util_test.cc
+                 roaring_position_bitmap_test.cc
                  string_util_test.cc
                  transform_util_test.cc
                  truncate_util_test.cc
@@ -124,8 +125,6 @@ add_iceberg_test(util_test
 
 add_iceberg_test(roaring_test SOURCES roaring_test.cc)
 
-add_iceberg_test(roaring_position_bitmap_test SOURCES roaring_position_bitmap_test.cc)
-
 if(ICEBERG_BUILD_BUNDLE)
   add_iceberg_test(avro_test
                    USE_BUNDLE

src/iceberg/test/meson.build

@@ -90,6 +90,7 @@ iceberg_tests = {
             'endian_test.cc',
             'formatter_test.cc',
             'location_util_test.cc',
+            'roaring_position_bitmap_test.cc',
             'string_util_test.cc',
             'transform_util_test.cc',
             'truncate_util_test.cc',