Adjust for review

Author: AntoinePrv

cpp/src/arrow/util/bit_util.h

@@ -183,8 +183,8 @@ template <typename Uint>
 struct CopyBitsParams {
   Uint src = {};
   Uint dst = {};
-  Uint start = {};
-  Uint end = {};
+  int start = {};
+  int end = {};
 };
 
 /// Copy a contiguous span of bits from src into dst.
@@ -195,19 +195,20 @@ struct CopyBitsParams {
 /// guarantee that the range of bits to copy does not cover the whole range.
 template <typename Uint, bool kAllowFullCopy = true>
 [[nodiscard]] constexpr Uint CopyBitsInInteger(const CopyBitsParams<Uint>& params) {
-  constexpr auto kUintSizeBits = static_cast<Uint>(sizeof(Uint) * 8);
+  constexpr auto kUintSizeBits = static_cast<int>(sizeof(Uint) * 8);
   assert(params.start <= params.end);
   assert(params.start < kUintSizeBits);
   assert(params.end <= kUintSizeBits);
 
-  const auto length = static_cast<Uint>(params.end - params.start);
+  const int length = params.end - params.start;
   if constexpr (kAllowFullCopy) {
     if (length == kUintSizeBits) {
       return params.src;
     }
   }
   assert(length < kUintSizeBits);
-  const Uint mask = LeastSignificantBitMask<Uint, false>(length) << params.start;
+  const Uint mask =
+      static_cast<Uint>(LeastSignificantBitMask<Uint, false>(length) << params.start);
   return (~mask & params.dst) | (mask & params.src);
 }
 

cpp/src/arrow/util/rle_bitmap_internal.h

@@ -98,24 +98,24 @@ class RleRunToBitmapDecoder {
   /// Return how much the decoder would advance if asked to.
   ///
   /// Does not modify input.
-  rle_size_t AdvanceCapacity(rle_size_t batch_size) const noexcept {
+  rle_size_t GetAdvanceCapacity(rle_size_t batch_size) const noexcept {
     const auto n_vals = std::min(batch_size, remaining());
     return n_vals;
   }
 
   /// Advance by as many values as provided or until exhaustion of the decoder.
   /// Return the number of values skipped.
   [[nodiscard]] rle_size_t Advance(rle_size_t batch_size) {
-    const auto n_vals = AdvanceCapacity(batch_size);
+    const auto n_vals = GetAdvanceCapacity(batch_size);
     values_left_ -= n_vals;
     ARROW_DCHECK_GE(remaining(), 0);
     return n_vals;
   }
 
-  /// Get the next value and return false if there are no more.
+  /// Read the next value into `out` and return false if there are no more.
   [[nodiscard]] bool Get(BitmapSpanMut out) { return GetBatch(out, 1) == 1; }
 
-  /// Get a batch of values return the number of decoded elements.
+  /// Get a batch of values into `out` and return the number of decoded elements.
   ///
   /// May write fewer elements to the output than requested if there are not
   /// enough values left.
@@ -140,7 +140,7 @@ class RleRunToBitmapDecoder {
 
     // TRAILER: Writing inside the last byte if caller asked for non multiple of 8 values
     const auto n_last_vals = std::min(batch_size - n_vals, remaining());
-    if (ARROW_PREDICT_FALSE(n_last_vals > 0)) {
+    if (n_last_vals > 0) {
       ARROW_DCHECK_LT(n_last_vals, 8);
       n_vals += GetBatchInByte(out.NewStartingAt(n_vals), n_last_vals);
     }
@@ -215,15 +215,15 @@ class BitPackedRunToBitmapDecoder {
   /// Return how much the decoder would advance if asked to.
   ///
   /// Does not modify input.
-  rle_size_t AdvanceCapacity(rle_size_t batch_size) const noexcept {
+  rle_size_t GetAdvanceCapacity(rle_size_t batch_size) const noexcept {
     const auto n_vals = std::min(batch_size, remaining());
     return n_vals;
   }
 
   /// Advance by as many values as provided or until exhaustion of the decoder.
   /// Return the number of values skipped.
   [[nodiscard]] rle_size_t Advance(rle_size_t batch_size) {
-    const auto n_vals = AdvanceCapacity(batch_size);
+    const auto n_vals = GetAdvanceCapacity(batch_size);
     values_read_ += n_vals;
     ARROW_DCHECK_GE(remaining(), 0);
     return n_vals;
@@ -236,7 +236,7 @@ class BitPackedRunToBitmapDecoder {
   /// May write fewer elements to the output than requested if there are not enough values
   /// left.
   [[nodiscard]] rle_size_t GetBatch(BitmapSpanMut out, rle_size_t batch_size) {
-    auto n_vals = AdvanceCapacity(batch_size);
+    auto n_vals = GetAdvanceCapacity(batch_size);
     arrow::internal::CopyBitmap(unread_values_ptr(), unread_values_bit_offset(), n_vals,
                                 out.data(), out.bit_start());
     return Advance(n_vals);

cpp/src/arrow/util/rle_bitmap_test.cc

@@ -315,7 +315,7 @@ TEST_P(RleRunToBitmapDecoderTest, Decode) {
 
 INSTANTIATE_TEST_SUITE_P(  //
     RleBitmap, RleRunToBitmapDecoderTest,
-    ::testing::Values(0, 1, 3, 8, 9, 13, 64, 100, 100, 1000));
+    ::testing::Values(0, 1, 3, 8, 9, 13, 64, 100, 177));
 
 /*********************************
  *  BitPackedRunToBitmapDecoder  *
@@ -454,7 +454,7 @@ void CheckRleBitPackedToBitmap(const std::vector<uint8_t>& bytes,
   ASSERT_GT(n_vals, 0);
   for (const rle_size_t chunk_size :
        {rle_size_t{1}, rle_size_t{3}, rle_size_t{7}, rle_size_t{8}, rle_size_t{9},
-        rle_size_t{33}, n_vals}) {
+        rle_size_t{33}, n_vals, n_vals + 1}) {
     CheckRleBitPackedDecode(bytes, expected, chunk_size);
     // A non-zero output offset forces the first run to start at a non-byte
     // aligned output position.