Skip to content

Commit fdec80a

Browse files
committed
[C++][Parquet] Correct three comments on the DELTA_BINARY_PACKED scan
The coalescing guard is against values_remaining_current_mini_block_, so what it needs room for is the rest of the current miniblock, not a whole one; a run legitimately starts from a partly consumed miniblock. "Four is the narrowest width measured to win" reused a word that means bit width everywhere else in this file. The threshold is four lanes. PrefixSumVectorAndTail claimed its deltas pin the stored width at every step of its loop. At the top of the range the spread is all ones, which as a signed delta is -1, so the frame absorbs it and the miniblock stores width 1. Every stored width from 0 to the type's width is still reached, so the coverage is unchanged; only the comment was wrong. Comments only.
1 parent 4c8423d commit fdec80a

2 files changed

Lines changed: 10 additions & 9 deletions

File tree

cpp/src/parquet/decoder.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1471,9 +1471,9 @@ std::make_unsigned_t<T> PrefixSumDeltas(T* values, int num_values,
14711471
// A vector scan only pays off once a register holds enough values to beat the
14721472
// chain of additions it replaces. At two lanes it loses: one doubling step plus
14731473
// carrying the running value across vectors costs more than the two additions it
1474-
// saves. Four is the narrowest width measured to win, so that is the threshold;
1475-
// where a register holds fewer, this loop is dropped and the one below does all
1476-
// the work.
1474+
// saves. Four lanes is the narrowest register measured to win, so that is the
1475+
// threshold; where a register holds fewer, this loop is dropped and the one below
1476+
// does all the work.
14771477
if constexpr (kLanes >= 4) {
14781478
// Broadcasts the last lane, which carries the running value into the next
14791479
// vector without a round trip through a general-purpose register. Reading the
@@ -1684,7 +1684,7 @@ class DeltaBitPackDecoder : public TypedDecoderImpl<DType> {
16841684
// width that has not been checked, including the non-conformant widths InitBlock
16851685
// tolerates for extraneous miniblocks.
16861686
uint32_t CoalescibleMiniBlocks(uint32_t values_available) const {
1687-
// Folding in a whole miniblock first requires room for the current one in full.
1687+
// Folding in a whole miniblock first requires room for the rest of the current one.
16881688
if (values_available < values_remaining_current_mini_block_) {
16891689
return 0;
16901690
}

cpp/src/parquet/encoding_test.cc

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2118,16 +2118,17 @@ TYPED_TEST(TestDeltaBitPackEncoding, MiniblockBitWidthRuns) {
21182118
TYPED_TEST(TestDeltaBitPackEncoding, PrefixSumVectorAndTail) {
21192119
// A decoder may accumulate the running total several deltas at a time, finishing
21202120
// whatever does not fill a whole group one delta at a time and carrying the total
2121-
// from each group into the next. Walk every residual bit width, and at each one
2121+
// from each group into the next. Walk the residual bit widths, and at each one
21222122
// enough lengths to leave every remainder such a group can leave, so each width is
21232123
// decoded through the grouped path, through the remainder, and across the hand-off
21242124
// between them.
21252125
//
21262126
// Deltas alternate between the frame and the widest value the width can hold above
2127-
// it. That pins the stored width, keeps a non-zero frame in play -- whose running
2128-
// multiple grows with the index, so a decoder that folds it in per group has to get
2129-
// that multiple right -- and wraps the running total repeatedly, which is where a
2130-
// grouped total and a value-at-a-time one part company if any term is signed.
2127+
// it, which drives the stored width across the whole range 0 to the type's width.
2128+
// It also keeps a non-zero frame in play -- whose running multiple grows with the
2129+
// index, so a decoder that folds it in per group has to get that multiple right --
2130+
// and wraps the running total repeatedly, where a grouped total and a
2131+
// value-at-a-time one disagree if any term is signed.
21312132
using T = typename TypeParam::c_type;
21322133
using UT = std::make_unsigned_t<T>;
21332134
constexpr int kBits = static_cast<int>(sizeof(T) * 8);

0 commit comments

Comments
 (0)