From 1b355671950ba9a34566b7993a4f2e24aedbf4bf Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Fri, 16 Oct 2026 00:05:42 +0100 Subject: [PATCH 1/6] Split helper out better --- cpp/src/arrow/util/bit_util.h | 26 +++++++++++++++++--------- cpp/src/arrow/util/bit_util_test.cc | 24 ++++++++++++++++++++++++ cpp/src/parquet/chunker_internal.cc | 10 +--------- cpp/src/parquet/encoder.cc | 12 ++---------- 4 files changed, 44 insertions(+), 28 deletions(-) diff --git a/cpp/src/arrow/util/bit_util.h b/cpp/src/arrow/util/bit_util.h index 971b77d69798..fee9844d8464 100644 --- a/cpp/src/arrow/util/bit_util.h +++ b/cpp/src/arrow/util/bit_util.h @@ -138,20 +138,28 @@ static inline uint64_t TrailingBits(uint64_t v, int num_bits) { return (v << n) >> n; } -// Returns ceil(log2(x)). -static inline int Log2(uint64_t x) { - // DCHECK_GT(x, 0); - -// TODO: We can remove this condition once CRAN upgrades its macOS -// SDK from 11.3. -// __apple_build_version__ should be defined only on Apple clang +// Returns the number of bits needed to represent x, i.e. std::bit_width(x). +// +// The libc++ in the macOS 11.3 SDK used by CRAN only has the pre-standard +// name std::log2p1. __apple_build_version__ should be defined only on Apple +// clang. +// TODO: We can remove this condition once CRAN upgrades its macOS SDK from 11.3. +template +constexpr T BitWidth(T x) noexcept { + static_assert(std::is_unsigned_v, "BitWidth requires an unsigned integer type"); #if defined(__apple_build_version__) && !defined(__cpp_lib_bitops) - return std::log2p1(x - 1); + return std::log2p1(x); #else - return std::bit_width(x - 1); + return std::bit_width(x); #endif } +// Returns ceil(log2(x)). +static inline int Log2(uint64_t x) { + // DCHECK_GT(x, 0); + return static_cast(BitWidth(x - 1)); +} + // // Utilities for reading and writing individual bits by their index // in a memory area. diff --git a/cpp/src/arrow/util/bit_util_test.cc b/cpp/src/arrow/util/bit_util_test.cc index a6af004f7c5b..437925398462 100644 --- a/cpp/src/arrow/util/bit_util_test.cc +++ b/cpp/src/arrow/util/bit_util_test.cc @@ -1108,4 +1108,28 @@ TEST(BitUtil, CopyBits) { 0b01011010); } +TEST(BitUtil, BitWidth) { + EXPECT_EQ(bit_util::BitWidth(uint8_t{0}), 0); + EXPECT_EQ(bit_util::BitWidth(uint8_t{1}), 1); + EXPECT_EQ(bit_util::BitWidth(uint8_t{2}), 2); + EXPECT_EQ(bit_util::BitWidth(uint8_t{3}), 2); + EXPECT_EQ(bit_util::BitWidth(uint8_t{127}), 7); + EXPECT_EQ(bit_util::BitWidth(uint8_t{128}), 8); + EXPECT_EQ(bit_util::BitWidth(std::numeric_limits::max()), 8); + + EXPECT_EQ(bit_util::BitWidth(uint32_t{0}), 0); + EXPECT_EQ(bit_util::BitWidth(uint32_t{1} << 20), 21); + EXPECT_EQ(bit_util::BitWidth((uint32_t{1} << 20) - 1), 20); + EXPECT_EQ(bit_util::BitWidth(std::numeric_limits::max()), 32); + + EXPECT_EQ(bit_util::BitWidth(uint64_t{0}), 0); + EXPECT_EQ(bit_util::BitWidth(uint64_t{1} << 40), 41); + EXPECT_EQ(bit_util::BitWidth((uint64_t{1} << 40) - 1), 40); + EXPECT_EQ(bit_util::BitWidth(uint64_t{1} << 63), 64); + EXPECT_EQ(bit_util::BitWidth(std::numeric_limits::max()), 64); + + // constexpr-evaluable. + static_assert(bit_util::BitWidth(uint16_t{0x0100}) == 9); +} + } // namespace arrow diff --git a/cpp/src/parquet/chunker_internal.cc b/cpp/src/parquet/chunker_internal.cc index a6f161fafeb8..7da385f8101a 100644 --- a/cpp/src/parquet/chunker_internal.cc +++ b/cpp/src/parquet/chunker_internal.cc @@ -17,7 +17,6 @@ #include "parquet/chunker_internal.h" -#include #include #include #include @@ -87,14 +86,7 @@ uint64_t CalculateMask(int64_t min_chunk_size, int64_t max_chunk_size, int norm_ // assuming that the gear hash has a uniform distribution, we can calculate the mask // by taking the floor(log2(target_size)) - // TODO: We can remove this condition once CRAN upgrades its macOS - // SDK from 11.3. - // __apple_build_version__ should be defined only on Apple clang -#if defined(__apple_build_version__) && !defined(__cpp_lib_bitops) - auto target_bits = std::log2p1(static_cast(target_size)); -#else - auto target_bits = std::bit_width(static_cast(target_size)); -#endif + auto target_bits = ::arrow::bit_util::BitWidth(static_cast(target_size)); int mask_bits = target_bits == 0 ? 0 : static_cast(target_bits - 1); // a user defined `norm_level` can be used to adjust the mask size, hence the matching diff --git a/cpp/src/parquet/encoder.cc b/cpp/src/parquet/encoder.cc index 94615bd125d5..30341bbe25e1 100644 --- a/cpp/src/parquet/encoder.cc +++ b/cpp/src/parquet/encoder.cc @@ -1261,16 +1261,8 @@ void DeltaBitPackEncoder::FlushBlock() { // The minimum number of bits required to write any of values in deltas_ vector. // See overflow comment above. - // TODO: We can remove this condition once CRAN upgrades its macOS - // SDK from 11.3. - // __apple_build_version__ should be defined only on Apple clang -#if defined(__apple_build_version__) && !defined(__cpp_lib_bitops) - const auto bit_width = bit_width_data[i] = - std::log2p1(static_cast(max_delta) - static_cast(min_delta)); -#else - const auto bit_width = bit_width_data[i] = - std::bit_width(static_cast(max_delta) - static_cast(min_delta)); -#endif + const auto bit_width = bit_width_data[i] = ::arrow::bit_util::BitWidth( + static_cast(max_delta) - static_cast(min_delta)); for (uint32_t j = start; j < start + values_current_mini_block; j++) { // Convert delta to frame of reference. See overflow comment above. From 3f0fec98de937de54b90c07c264a2007507d845f Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Fri, 16 Oct 2026 00:06:17 +0100 Subject: [PATCH 2/6] Add other helper usage --- cpp/src/arrow/util/rle_encoding_test.cc | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/cpp/src/arrow/util/rle_encoding_test.cc b/cpp/src/arrow/util/rle_encoding_test.cc index 16b702c23d49..07e9e5e515a0 100644 --- a/cpp/src/arrow/util/rle_encoding_test.cc +++ b/cpp/src/arrow/util/rle_encoding_test.cc @@ -18,7 +18,6 @@ // From Apache Impala (incubating) as of 2016-01-29 #include -#include #include #include #include @@ -942,14 +941,7 @@ TEST(BitRle, Random) { } parity = !parity; } - // TODO: We can remove this condition once CRAN upgrades its macOS - // SDK from 11.3. - // __apple_build_version__ should be defined only on Apple clang -#if defined(__apple_build_version__) && !defined(__cpp_lib_bitops) - if (!CheckRoundTrip(values, std::log2p1(values.size()))) { -#else - if (!CheckRoundTrip(values, std::bit_width(values.size()))) { -#endif + if (!CheckRoundTrip(values, static_cast(bit_util::BitWidth(values.size())))) { FAIL() << "failing seed: " << seed; } } From 30307d4d191e24a45e96feb8e5567092cd9b12a9 Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Fri, 16 Oct 2026 00:06:55 +0100 Subject: [PATCH 3/6] Use helper in ALP compression --- cpp/src/arrow/util/alp/alp_compression.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/src/arrow/util/alp/alp_compression.cc b/cpp/src/arrow/util/alp/alp_compression.cc index 6b40f4e39a9f..51ea1a0a3f96 100644 --- a/cpp/src/arrow/util/alp/alp_compression.cc +++ b/cpp/src/arrow/util/alp/alp_compression.cc @@ -18,7 +18,6 @@ #include "arrow/util/alp/alp_compression_internal.h" #include -#include #include #include #include @@ -200,7 +199,8 @@ std::optional EstimateCompressedSizeBits( const EncodedUnsigned delta = (static_cast(max_encoded_value) - static_cast(min_encoded_value)); - const int32_t estimated_bits_per_value = static_cast(std::bit_width(delta)); + const int32_t estimated_bits_per_value = + static_cast(bit_util::BitWidth(delta)); int64_t estimated_compression_size = static_cast(inputs.size()) * estimated_bits_per_value; estimated_compression_size += @@ -445,7 +445,7 @@ typename AlpCompression::BitPackingResult AlpCompression::BitPackIntegers( uint8_t bit_width = 0; if (min_max_diff > 0) { - bit_width = static_cast(std::bit_width(min_max_diff)); + bit_width = static_cast(bit_util::BitWidth(min_max_diff)); } const int32_t bit_packed_size = static_cast( bit_util::BytesForBits(bit_width * static_cast(for_deltas.size()))); From c3b352e2bcf7efea21336a475c3bb9fbf3e51c58 Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Fri, 16 Oct 2026 00:28:05 +0100 Subject: [PATCH 4/6] Make it and int and don't cast int to int --- cpp/src/arrow/util/alp/alp_compression.cc | 3 +-- cpp/src/arrow/util/bit_util.h | 8 ++++---- cpp/src/arrow/util/rle_encoding_test.cc | 2 +- cpp/src/parquet/chunker_internal.cc | 2 +- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/cpp/src/arrow/util/alp/alp_compression.cc b/cpp/src/arrow/util/alp/alp_compression.cc index 51ea1a0a3f96..b3e59dc9611b 100644 --- a/cpp/src/arrow/util/alp/alp_compression.cc +++ b/cpp/src/arrow/util/alp/alp_compression.cc @@ -199,8 +199,7 @@ std::optional EstimateCompressedSizeBits( const EncodedUnsigned delta = (static_cast(max_encoded_value) - static_cast(min_encoded_value)); - const int32_t estimated_bits_per_value = - static_cast(bit_util::BitWidth(delta)); + const int32_t estimated_bits_per_value = bit_util::BitWidth(delta); int64_t estimated_compression_size = static_cast(inputs.size()) * estimated_bits_per_value; estimated_compression_size += diff --git a/cpp/src/arrow/util/bit_util.h b/cpp/src/arrow/util/bit_util.h index fee9844d8464..11ce44301a5f 100644 --- a/cpp/src/arrow/util/bit_util.h +++ b/cpp/src/arrow/util/bit_util.h @@ -145,19 +145,19 @@ static inline uint64_t TrailingBits(uint64_t v, int num_bits) { // clang. // TODO: We can remove this condition once CRAN upgrades its macOS SDK from 11.3. template -constexpr T BitWidth(T x) noexcept { +constexpr int BitWidth(T x) noexcept { static_assert(std::is_unsigned_v, "BitWidth requires an unsigned integer type"); #if defined(__apple_build_version__) && !defined(__cpp_lib_bitops) - return std::log2p1(x); + return static_cast(std::log2p1(x)); #else - return std::bit_width(x); + return static_cast(std::bit_width(x)); #endif } // Returns ceil(log2(x)). static inline int Log2(uint64_t x) { // DCHECK_GT(x, 0); - return static_cast(BitWidth(x - 1)); + return BitWidth(x - 1); } // diff --git a/cpp/src/arrow/util/rle_encoding_test.cc b/cpp/src/arrow/util/rle_encoding_test.cc index 07e9e5e515a0..c795355f3ca4 100644 --- a/cpp/src/arrow/util/rle_encoding_test.cc +++ b/cpp/src/arrow/util/rle_encoding_test.cc @@ -941,7 +941,7 @@ TEST(BitRle, Random) { } parity = !parity; } - if (!CheckRoundTrip(values, static_cast(bit_util::BitWidth(values.size())))) { + if (!CheckRoundTrip(values, bit_util::BitWidth(values.size()))) { FAIL() << "failing seed: " << seed; } } diff --git a/cpp/src/parquet/chunker_internal.cc b/cpp/src/parquet/chunker_internal.cc index 7da385f8101a..4cb753ea1203 100644 --- a/cpp/src/parquet/chunker_internal.cc +++ b/cpp/src/parquet/chunker_internal.cc @@ -87,7 +87,7 @@ uint64_t CalculateMask(int64_t min_chunk_size, int64_t max_chunk_size, int norm_ // assuming that the gear hash has a uniform distribution, we can calculate the mask // by taking the floor(log2(target_size)) auto target_bits = ::arrow::bit_util::BitWidth(static_cast(target_size)); - int mask_bits = target_bits == 0 ? 0 : static_cast(target_bits - 1); + int mask_bits = target_bits == 0 ? 0 : target_bits - 1; // a user defined `norm_level` can be used to adjust the mask size, hence the matching // probability, by increasing the norm_level we increase the probability of matching From 896343ca3a6154b77f9b53b73dfab9e4f5ffa250 Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Fri, 16 Oct 2026 09:16:37 +0100 Subject: [PATCH 5/6] Add second helper function --- cpp/src/arrow/util/align_util.h | 11 +---------- cpp/src/arrow/util/alp/alp_codec.cc | 2 +- cpp/src/arrow/util/bit_util.h | 13 +++++++++++++ cpp/src/arrow/util/bit_util_test.cc | 18 ++++++++++++++++++ 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/cpp/src/arrow/util/align_util.h b/cpp/src/arrow/util/align_util.h index 0b23c6d77bd9..cac91f80fc55 100644 --- a/cpp/src/arrow/util/align_util.h +++ b/cpp/src/arrow/util/align_util.h @@ -18,7 +18,6 @@ #pragma once #include -#include #include "arrow/memory_pool.h" #include "arrow/type_fwd.h" @@ -44,16 +43,8 @@ struct BitmapWordAlignParams { template inline BitmapWordAlignParams BitmapWordAlign(const uint8_t* data, int64_t bit_offset, int64_t length) { - // TODO: We can remove this condition once CRAN upgrades its macOS - // SDK from 11.3. - // __apple_build_version__ should be defined only on Apple clang -#if defined(__apple_build_version__) && !defined(__cpp_lib_bitops) - static_assert((ALIGN_IN_BYTES != 0) && ((ALIGN_IN_BYTES & (ALIGN_IN_BYTES - 1)) == 0), + static_assert(bit_util::HasSingleBit(ALIGN_IN_BYTES), "ALIGN_IN_BYTES should be a positive power of two"); -#else - static_assert(std::has_single_bit(ALIGN_IN_BYTES), - "ALIGN_IN_BYTES should be a positive power of two"); -#endif constexpr uint64_t ALIGN_IN_BITS = ALIGN_IN_BYTES * 8; BitmapWordAlignParams p; diff --git a/cpp/src/arrow/util/alp/alp_codec.cc b/cpp/src/arrow/util/alp/alp_codec.cc index 315398c5c494..bbdc5d829287 100644 --- a/cpp/src/arrow/util/alp/alp_codec.cc +++ b/cpp/src/arrow/util/alp/alp_codec.cc @@ -136,7 +136,7 @@ Status ValidateEncodingPreset(const AlpEncodingPreset& preset) { Status ValidateVectorSize(int32_t vector_size) { constexpr int32_t kMin = 1 << AlpFormatConstants::kMinLogVectorSize; constexpr int32_t kMax = 1 << AlpFormatConstants::kMaxLogVectorSize; - if (vector_size <= 0 || !std::has_single_bit(static_cast(vector_size))) { + if (vector_size <= 0 || !bit_util::HasSingleBit(static_cast(vector_size))) { return Status::Invalid("ALP vector_size must be a positive power of 2, got ", vector_size); } diff --git a/cpp/src/arrow/util/bit_util.h b/cpp/src/arrow/util/bit_util.h index 11ce44301a5f..10e1fdfe2f2f 100644 --- a/cpp/src/arrow/util/bit_util.h +++ b/cpp/src/arrow/util/bit_util.h @@ -154,6 +154,19 @@ constexpr int BitWidth(T x) noexcept { #endif } +// Returns true if x is a power of two, i.e. std::has_single_bit(x). +// See the note on BitWidth: the macOS 11.3 SDK only has the pre-standard +// name std::ispow2. +template +constexpr bool HasSingleBit(T x) noexcept { + static_assert(std::is_unsigned_v, "HasSingleBit requires an unsigned integer type"); +#if defined(__apple_build_version__) && !defined(__cpp_lib_bitops) + return std::ispow2(x); +#else + return std::has_single_bit(x); +#endif +} + // Returns ceil(log2(x)). static inline int Log2(uint64_t x) { // DCHECK_GT(x, 0); diff --git a/cpp/src/arrow/util/bit_util_test.cc b/cpp/src/arrow/util/bit_util_test.cc index 437925398462..682b72eaa8d3 100644 --- a/cpp/src/arrow/util/bit_util_test.cc +++ b/cpp/src/arrow/util/bit_util_test.cc @@ -1132,4 +1132,22 @@ TEST(BitUtil, BitWidth) { static_assert(bit_util::BitWidth(uint16_t{0x0100}) == 9); } +TEST(BitUtil, HasSingleBit) { + EXPECT_FALSE(bit_util::HasSingleBit(uint8_t{0})); + EXPECT_TRUE(bit_util::HasSingleBit(uint8_t{1})); + EXPECT_TRUE(bit_util::HasSingleBit(uint8_t{2})); + EXPECT_FALSE(bit_util::HasSingleBit(uint8_t{3})); + EXPECT_TRUE(bit_util::HasSingleBit(uint8_t{128})); + EXPECT_FALSE(bit_util::HasSingleBit(std::numeric_limits::max())); + + EXPECT_TRUE(bit_util::HasSingleBit(uint32_t{1} << 20)); + EXPECT_FALSE(bit_util::HasSingleBit((uint32_t{1} << 20) + 1)); + EXPECT_TRUE(bit_util::HasSingleBit(uint64_t{1} << 63)); + EXPECT_FALSE(bit_util::HasSingleBit(std::numeric_limits::max())); + + // constexpr-evaluable. + static_assert(bit_util::HasSingleBit(uint16_t{0x0100})); + static_assert(!bit_util::HasSingleBit(uint16_t{0x0101})); +} + } // namespace arrow From c4e05e0ecdbf11a27c239350c0a69772980b9bb6 Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Fri, 16 Oct 2026 09:34:01 +0100 Subject: [PATCH 6/6] Trim tests --- cpp/src/arrow/util/bit_util_test.cc | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/cpp/src/arrow/util/bit_util_test.cc b/cpp/src/arrow/util/bit_util_test.cc index 682b72eaa8d3..2c315b75134e 100644 --- a/cpp/src/arrow/util/bit_util_test.cc +++ b/cpp/src/arrow/util/bit_util_test.cc @@ -1111,23 +1111,9 @@ TEST(BitUtil, CopyBits) { TEST(BitUtil, BitWidth) { EXPECT_EQ(bit_util::BitWidth(uint8_t{0}), 0); EXPECT_EQ(bit_util::BitWidth(uint8_t{1}), 1); - EXPECT_EQ(bit_util::BitWidth(uint8_t{2}), 2); - EXPECT_EQ(bit_util::BitWidth(uint8_t{3}), 2); - EXPECT_EQ(bit_util::BitWidth(uint8_t{127}), 7); - EXPECT_EQ(bit_util::BitWidth(uint8_t{128}), 8); - EXPECT_EQ(bit_util::BitWidth(std::numeric_limits::max()), 8); - - EXPECT_EQ(bit_util::BitWidth(uint32_t{0}), 0); EXPECT_EQ(bit_util::BitWidth(uint32_t{1} << 20), 21); EXPECT_EQ(bit_util::BitWidth((uint32_t{1} << 20) - 1), 20); - EXPECT_EQ(bit_util::BitWidth(std::numeric_limits::max()), 32); - - EXPECT_EQ(bit_util::BitWidth(uint64_t{0}), 0); - EXPECT_EQ(bit_util::BitWidth(uint64_t{1} << 40), 41); - EXPECT_EQ(bit_util::BitWidth((uint64_t{1} << 40) - 1), 40); - EXPECT_EQ(bit_util::BitWidth(uint64_t{1} << 63), 64); EXPECT_EQ(bit_util::BitWidth(std::numeric_limits::max()), 64); - // constexpr-evaluable. static_assert(bit_util::BitWidth(uint16_t{0x0100}) == 9); } @@ -1135,19 +1121,11 @@ TEST(BitUtil, BitWidth) { TEST(BitUtil, HasSingleBit) { EXPECT_FALSE(bit_util::HasSingleBit(uint8_t{0})); EXPECT_TRUE(bit_util::HasSingleBit(uint8_t{1})); - EXPECT_TRUE(bit_util::HasSingleBit(uint8_t{2})); - EXPECT_FALSE(bit_util::HasSingleBit(uint8_t{3})); - EXPECT_TRUE(bit_util::HasSingleBit(uint8_t{128})); - EXPECT_FALSE(bit_util::HasSingleBit(std::numeric_limits::max())); - EXPECT_TRUE(bit_util::HasSingleBit(uint32_t{1} << 20)); EXPECT_FALSE(bit_util::HasSingleBit((uint32_t{1} << 20) + 1)); - EXPECT_TRUE(bit_util::HasSingleBit(uint64_t{1} << 63)); EXPECT_FALSE(bit_util::HasSingleBit(std::numeric_limits::max())); - // constexpr-evaluable. static_assert(bit_util::HasSingleBit(uint16_t{0x0100})); - static_assert(!bit_util::HasSingleBit(uint16_t{0x0101})); } } // namespace arrow