From 079cf8ee61e16483550abb825c44fffa8fc5edae Mon Sep 17 00:00:00 2001 From: AntoinePrv Date: Mon, 3 Aug 2026 15:07:50 +0200 Subject: [PATCH 1/7] Align copy bitmap writer --- cpp/src/arrow/util/bitmap_ops.cc | 51 +++++++++++++++++++------------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/cpp/src/arrow/util/bitmap_ops.cc b/cpp/src/arrow/util/bitmap_ops.cc index 33a95150d53b..81b6d38fda39 100644 --- a/cpp/src/arrow/util/bitmap_ops.cc +++ b/cpp/src/arrow/util/bitmap_ops.cc @@ -17,6 +17,7 @@ #include "arrow/util/bitmap_ops.h" +#include #include #include #include @@ -32,8 +33,7 @@ #include "arrow/util/bitmap_writer.h" #include "arrow/util/logging_internal.h" -namespace arrow { -namespace internal { +namespace arrow::internal { int64_t CountSetBits(const uint8_t* data, int64_t bit_offset, int64_t length) { constexpr int64_t pop_len = sizeof(uint64_t) * 8; @@ -123,28 +123,40 @@ uint8_t GetReversedBlock(uint8_t block_left, uint8_t block_right, uint8_t length return ReverseUint8(((block_right << 8) + block_left) >> length); } +template +void TransferReaderWriter(auto&& reader, auto&& writer) { + auto nwords = reader.words(); + while (nwords--) { + auto word = reader.NextWord(); + writer.PutNextWord(mode == TransferMode::Invert ? ~word : word); + } + auto nbytes = reader.trailing_bytes(); + while (nbytes--) { + int valid_bits; + auto byte = reader.NextTrailingByte(valid_bits); + writer.PutNextTrailingByte(mode == TransferMode::Invert ? ~byte : byte, valid_bits); + } +} + template void TransferBitmap(const uint8_t* data, int64_t offset, int64_t length, int64_t dest_offset, uint8_t* dest) { int64_t bit_offset = offset % 8; int64_t dest_bit_offset = dest_offset % 8; - if (bit_offset || dest_bit_offset) { + if (length == 0) { + return; + } else if (dest_bit_offset) { + const auto count = std::min(8 - dest_bit_offset, length); + auto reader = internal::BitmapWordReader(data, offset, count); + auto writer = internal::BitmapWordWriter(dest, dest_offset, count); + TransferReaderWriter(reader, writer); + TransferBitmap(data, offset + count, length - count, dest_offset + count, dest); + } else if (bit_offset) { auto reader = internal::BitmapWordReader(data, offset, length); - auto writer = internal::BitmapWordWriter(dest, dest_offset, length); - - auto nwords = reader.words(); - while (nwords--) { - auto word = reader.NextWord(); - writer.PutNextWord(mode == TransferMode::Invert ? ~word : word); - } - auto nbytes = reader.trailing_bytes(); - while (nbytes--) { - int valid_bits; - auto byte = reader.NextTrailingByte(valid_bits); - writer.PutNextTrailingByte(mode == TransferMode::Invert ? ~byte : byte, valid_bits); - } - } else if (length) { + auto writer = internal::BitmapWordWriter(dest, dest_offset, length); + TransferReaderWriter(reader, writer); + } else { int64_t num_bytes = bit_util::BytesForBits(length); // Shift by its byte offset @@ -159,7 +171,7 @@ void TransferBitmap(const uint8_t* data, int64_t offset, int64_t length, uint8_t trail_mask = (1U << (8 - trailing_bits)) - 1; uint8_t last_data; - if (mode == TransferMode::Invert) { + if constexpr (mode == TransferMode::Invert) { for (int64_t i = 0; i < num_bytes - 1; i++) { dest[i] = static_cast(~(data[i])); } @@ -519,5 +531,4 @@ void BitmapOrNot(const uint8_t* left, int64_t left_offset, const uint8_t* right, BitmapOp(left, left_offset, right, right_offset, length, out_offset, out); } -} // namespace internal -} // namespace arrow +} // namespace arrow::internal From ce662c6c3e9d69a3f1a85bdb624d03780ffb2f96 Mon Sep 17 00:00:00 2001 From: AntoinePrv Date: Mon, 3 Aug 2026 16:32:55 +0200 Subject: [PATCH 2/7] Factorize in MapReadersWriter --- cpp/src/arrow/util/bitmap_ops.cc | 40 ++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 7 deletions(-) diff --git a/cpp/src/arrow/util/bitmap_ops.cc b/cpp/src/arrow/util/bitmap_ops.cc index 81b6d38fda39..2210b1382017 100644 --- a/cpp/src/arrow/util/bitmap_ops.cc +++ b/cpp/src/arrow/util/bitmap_ops.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include "arrow/buffer.h" #include "arrow/result.h" @@ -123,18 +124,43 @@ uint8_t GetReversedBlock(uint8_t block_left, uint8_t block_right, uint8_t length return ReverseUint8(((block_right << 8) + block_left) >> length); } -template -void TransferReaderWriter(auto&& reader, auto&& writer) { +void MapReadersWriter(auto&& writer, auto&& reducer, auto&& reader, auto&&... readers) { + constexpr auto kReaderCount = sizeof...(readers) + 1; + + // Need a real function so that the fold expression remains valid in release + const auto check_eq = [](auto a, auto b) { ARROW_DCHECK_EQ(a, b); }; + auto nwords = reader.words(); + ((check_eq(readers.words(), nwords)), ...); while (nwords--) { - auto word = reader.NextWord(); - writer.PutNextWord(mode == TransferMode::Invert ? ~word : word); + writer.PutNextWord(reducer(reader.NextWord(), readers.NextWord()...)); } + auto nbytes = reader.trailing_bytes(); + ((check_eq(readers.trailing_bytes(), nbytes)), ...); while (nbytes--) { - int valid_bits; - auto byte = reader.NextTrailingByte(valid_bits); - writer.PutNextTrailingByte(mode == TransferMode::Invert ? ~byte : byte, valid_bits); + int valid_bits = 0; + std::array bytes = {}; + { + auto* b = bytes.begin(); + *b++ = reader.NextTrailingByte(valid_bits); + auto read = [&](auto& r) { + int vb = 0; + *b++ = r.NextTrailingByte(vb); + check_eq(vb, valid_bits); + }; + (read(readers), ...); + } + writer.PutNextTrailingByte(std::apply(reducer, bytes), valid_bits); + } +} + +template +void TransferReaderWriter(auto&& reader, auto&& writer) { + if constexpr (mode == TransferMode::Invert) { + MapReadersWriter(writer, [](T x) { return static_cast(~x); }, reader); + } else { + MapReadersWriter(writer, [](auto x) { return x; }, reader); } } From 444d47678bd3032251d87cecb880eec75b74aae0 Mon Sep 17 00:00:00 2001 From: AntoinePrv Date: Tue, 4 Aug 2026 10:36:16 +0200 Subject: [PATCH 3/7] Align Bitmap op writer --- cpp/src/arrow/util/bitmap_ops.cc | 118 +++++++++++++++++++------------ 1 file changed, 73 insertions(+), 45 deletions(-) diff --git a/cpp/src/arrow/util/bitmap_ops.cc b/cpp/src/arrow/util/bitmap_ops.cc index 2210b1382017..7da1b030bd2d 100644 --- a/cpp/src/arrow/util/bitmap_ops.cc +++ b/cpp/src/arrow/util/bitmap_ops.cc @@ -124,7 +124,7 @@ uint8_t GetReversedBlock(uint8_t block_left, uint8_t block_right, uint8_t length return ReverseUint8(((block_right << 8) + block_left) >> length); } -void MapReadersWriter(auto&& writer, auto&& reducer, auto&& reader, auto&&... readers) { +void MapReadersWriter(auto&& writer, auto&& op, auto&& reader, auto&&... readers) { constexpr auto kReaderCount = sizeof...(readers) + 1; // Need a real function so that the fold expression remains valid in release @@ -133,7 +133,7 @@ void MapReadersWriter(auto&& writer, auto&& reducer, auto&& reader, auto&&... re auto nwords = reader.words(); ((check_eq(readers.words(), nwords)), ...); while (nwords--) { - writer.PutNextWord(reducer(reader.NextWord(), readers.NextWord()...)); + writer.PutNextWord(op(reader.NextWord(), readers.NextWord()...)); } auto nbytes = reader.trailing_bytes(); @@ -151,39 +151,67 @@ void MapReadersWriter(auto&& writer, auto&& reducer, auto&& reader, auto&&... re }; (read(readers), ...); } - writer.PutNextTrailingByte(std::apply(reducer, bytes), valid_bits); + writer.PutNextTrailingByte(std::apply(op, bytes), valid_bits); } } -template -void TransferReaderWriter(auto&& reader, auto&& writer) { - if constexpr (mode == TransferMode::Invert) { - MapReadersWriter(writer, [](T x) { return static_cast(~x); }, reader); +template +struct BitmapPtr { + Byte* data; + int64_t offset; + + BitmapPtr operator+(int64_t extra) { return {.data = data, .offset = offset + extra}; } +}; + +using BitmapConstPtr = BitmapPtr; +using BitmapMutPtr = BitmapPtr; + +template +void FastMapReadersWriter(BitmapMutPtr out, int64_t length, auto&& op, + BitmapConstPtr reader, auto&&... readers) { + const int64_t out_bit_offset = out.offset % 8; + + if (length == 0) { + return; + } else if (out_bit_offset) { + using Reader = internal::BitmapWordReader; + using Writer = internal::BitmapWordWriter; + + const auto count = std::min(8 - out_bit_offset, length); + auto writer = Writer(out.data, out.offset, count); + MapReadersWriter(writer, op, Reader(reader.data, reader.offset, count), + Reader(readers.data, readers.offset, count)...); + FastMapReadersWriter(out + count, length - count, op, reader + count, + readers + count...); } else { - MapReadersWriter(writer, [](auto x) { return x; }, reader); + using Reader = internal::BitmapWordReader; + using Writer = internal::BitmapWordWriter; + + auto writer = Writer(out.data, out.offset, length); + MapReadersWriter(writer, op, Reader(reader.data, reader.offset, length), + Reader(readers.data, readers.offset, length)...); } } template void TransferBitmap(const uint8_t* data, int64_t offset, int64_t length, int64_t dest_offset, uint8_t* dest) { - int64_t bit_offset = offset % 8; - int64_t dest_bit_offset = dest_offset % 8; + const int64_t bit_offset = offset % 8; + const int64_t dest_bit_offset = dest_offset % 8; - if (length == 0) { - return; - } else if (dest_bit_offset) { - const auto count = std::min(8 - dest_bit_offset, length); - auto reader = internal::BitmapWordReader(data, offset, count); - auto writer = internal::BitmapWordWriter(dest, dest_offset, count); - TransferReaderWriter(reader, writer); - TransferBitmap(data, offset + count, length - count, dest_offset + count, dest); - } else if (bit_offset) { - auto reader = internal::BitmapWordReader(data, offset, length); - auto writer = internal::BitmapWordWriter(dest, dest_offset, length); - TransferReaderWriter(reader, writer); - } else { - int64_t num_bytes = bit_util::BytesForBits(length); + constexpr auto op = [](T x) { + if constexpr (mode == TransferMode::Invert) { + return static_cast(~x); + } else { + return x; + } + }; // NOLINT: readability/braces + + if (bit_offset || dest_bit_offset) { + FastMapReadersWriter({.data = dest, .offset = dest_offset}, length, op, + {.data = data, .offset = offset}); + } else if (length > 0) { + const int64_t num_bytes = bit_util::BytesForBits(length); // Shift by its byte offset data += offset / 8; @@ -193,8 +221,8 @@ void TransferBitmap(const uint8_t* data, int64_t offset, int64_t length, // E.g., if trailing_bits = 5, last byte should be // - low 3 bits: new bits from last byte of data buffer // - high 5 bits: old bits from last byte of dest buffer - int64_t trailing_bits = num_bytes * 8 - length; - uint8_t trail_mask = (1U << (8 - trailing_bits)) - 1; + const int64_t trailing_bits = num_bytes * 8 - length; + const uint8_t trail_mask = (1U << (8 - trailing_bits)) - 1; uint8_t last_data; if constexpr (mode == TransferMode::Invert) { @@ -420,25 +448,9 @@ template