Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/20470 )
Change subject: WIP [cfile] introduce BP128 integer encoding ...................................................................... Patch Set 4: (14 comments) http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bitpacking-test.cc File src/kudu/cfile/bitpacking-test.cc: http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bitpacking-test.cc@73 PS4, Line 73: __m128i packed[ints.size()/4]; nit: __m128i packed[ints.size()/4] is a VLA. ints.size() is 128 here, so a fixed-size array would do, right? http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bitpacking-test.cc@235 PS4, Line 235: uint32_t mask = (1 << bits) - 1; bits goes up to 31 here, and 1 is a signed int -- doesn't 1 << 31 overflow into the sign bit (UB)? Should this be 1U << bits? http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bitpacking-test.cc@255 PS4, Line 255: uint64_t mask = (1 << bits) - 1; uint64_t mask = (1 << bits) - 1 with bits up to 32: 1 << 32 shifts an int by >= its width (UB), and 1 << 31 already overflows. Should be 1ULL << bits -- otherwise the highest-bit-width cases aren't actually testing what they intend. http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bitpacking.h File src/kudu/cfile/bitpacking.h: http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bitpacking.h@50 PS4, Line 50: template<> These explicit specializations are defined in the header but not inline. Since explicit specializations aren't implicitly inline, wouldn't including this header from a second TU cause a multiple-definition error? Should they be marked inline? http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bitpacking.h@61 PS4, Line 61: uint16_t Shouldn't this be uint8_t? http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bitpacking.h@189 PS4, Line 189: deltas[i] = page[i] - page[i - 1]; The FOR/delta arithmetic runs in signed CPP_TYPE: ComputeDeltas (page[i]-page[i-1]), SubtractFOR (page[i]-min_val), and the UncompressFullPage:297 reconstruction. Even in the plain FOR path the result is non-negative but the signed subtraction overflows when a page spans > 2^63 (e.g. a value near INT64_MAX and one near INT64_MIN) TestIntBlockRoundTrip feeds full-range signed values, so this looks reachable once the loop count is restored. Should these be computed in the unsigned domain (cast to UnsignedCppType before subtracting/adding) and reinterpreted at the end? http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bitpacking.h@211 PS4, Line 211: CPP_TYPE deltas[page_len]; nit: would CPP_TYPE deltas[kPageSize] be both standard-compliant and cheaper? (non VLA, as page_len is bounded by kPageSize (128)) http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bp128_block.h File src/kudu/cfile/bp128_block.h: http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bp128_block.h@82 PS4, Line 82: CppType cell(int idx) const { data_ is std::vector<CppType> (element-indexed), but cell() indexes data_[idx * size_of_type] -- for multi-byte types doesn't that land on the wrong element (idx*8 for int64)? It looks unused (GetFirst/LastKey index data_ directly), so is this dead code left over from a byte-buffer representation? If so, could we drop it (and the unused UnsignedCppType aliases) to avoid a latent trap? http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bp128_block.h@88 PS4, Line 88: static const size_t kHeaderSize = sizeof(uint32_t) * 5; nit: this kHeaderSize (sizeof(uint32_t)*5 = 20) appears unused and disagrees with the decoder's kHeaderSize (8) and the actual 12-byte header. Worth removing/reconciling so there's one source of truth for the header size. http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bp128_block.cc File src/kudu/cfile/bp128_block.cc: http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bp128_block.cc@168 PS4, Line 168: const int kHeaderSize = 8; kHeaderSize is 8, but ParseHeader reads three fixed32 fields (first_row_idx_, num_elems_, use_deltas = 12 bytes) below. A block with size in [8,11] passes this guard and then over-reads at offset 8. Should the minimum be 12? http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bp128_block.cc@183 PS4, Line 183: CHECK(num_elems_ >= 0); num_elems_ is an int decoded from a uint32 on disk, so a corrupt/truncated block could make this CHECK abort the whole process. Should decode paths that consume persisted (potentially corrupt) data return Status::Corruption here instead of CHECK-failing? Same question for the CHECK(!parsed_)/CHECK(parsed_) guards — is a corrupt block able to reach those, and if so should they be graceful errors rather than aborts? http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bp128_block.cc@190 PS4, Line 190: min_vals_ = { min_vals_ is an ArrayView<CppType> pointing at decode_slice.data(), which sits at byte offset 12 (after 3x fixed32). For INT64/UINT64 that's only 4-byte aligned -- doesn't dereferencing min_vals_[i] (e.g. in SeekAtOrAfterValue's upper_bound, and min_deltas_ likewise) then become a misaligned 8-byte load? Should the 8-byte arrays be laid out at an 8-byte-aligned offset, or read via UnalignedLoad/memcpy? http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/bp128_block.cc@194 PS4, Line 194: decode_slice.remove_prefix(num_pages * sizeof(CppType)); After decoding the 12-byte header there's no validation that decode_slice actually contains num_pages*sizeof(CppType) + num_pages + (optional num_pages*sizeof(CppType)) bytes before the reinterpret_casts and remove_prefix() calls. On a truncated block these remove_prefix() calls can underflow / the ArrayViews can point past the buffer. The "// TODO: check sizes" at line 166 acknowledges this; flagging so it isn't lost before this leaves WIP, since this is a decode path for persisted (potentially corrupt) data. http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/encoding-test.cc File src/kudu/cfile/encoding-test.cc: http://gerrit.cloudera.org:8080/#/c/20470/4/src/kudu/cfile/encoding-test.cc@679 PS4, Line 679: for (int i = 0; i < 2; i++) { This loop bound was changed from 10003 to 2. Is this a debugging leftover? -- To view, visit http://gerrit.cloudera.org:8080/20470 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I07088a39907f0904846104b4d1d272a5968875e2 Gerrit-Change-Number: 20470 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Thu, 06 Aug 2026 14:36:20 +0000 Gerrit-HasComments: Yes
