HuaHuaY commented on code in PR #55408:
URL: https://github.com/apache/arrow/pull/55408#discussion_r4480736339
##########
cpp/src/parquet/size_statistics.cc:
##########
@@ -160,17 +160,11 @@ void UpdateLevelHistogram(std::span<const int16_t>
levels, std::span<int64_t> hi
if (max_level == 1) {
// Specialize the common case for non-repeated non-nested columns.
- // Summing the levels gives us the number of 1s, and the number of 0s
follows.
- // We do repeated sums in the int16_t space, which the compiler is likely
- // to vectorize efficiently.
- constexpr int64_t kChunkSize = 1 << 14; // to avoid int16_t overflows
- int64_t hist1 = 0;
- auto it = levels.begin();
- while (it != levels.end()) {
- const auto chunk_size = std::min<int64_t>(levels.end() - it, kChunkSize);
- hist1 += std::accumulate(it, it + chunk_size, int16_t{0});
- it += chunk_size;
- }
+ // Counting the 1s directly gives us the number of 1s, and the number of 0s
+ // follows. CountIf16 accumulates in the int16_t space in chunks, which the
+ // compiler vectorizes efficiently and which cannot overflow.
+ const int64_t hist1 = internal::CountIf16(levels.data(), num_levels,
Review Comment:
I’ve found that the ASSUME macro can easily resolve this issue
https://godbolt.org/z/Mf1EzdPP1. However, the best approach here might be to
leave the original code unmodified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
