Copilot commented on code in PR #51128:
URL: https://github.com/apache/arrow/pull/51128#discussion_r4526135886
##########
cpp/src/parquet/decoder.cc:
##########
@@ -1613,11 +1613,24 @@ class DeltaBitPackDecoder : public
TypedDecoderImpl<DType> {
}
total_values_remaining_ = total_value_count_;
- if (delta_bit_widths_ == nullptr) {
- delta_bit_widths_ = AllocateBuffer(pool_, mini_blocks_per_block_);
- } else {
- PARQUET_THROW_NOT_OK(
- delta_bit_widths_->Resize(mini_blocks_per_block_, /*shrink_to_fit*/
false));
+ if (total_value_count_ > 1) {
+ const int64_t required_blocks = ::arrow::bit_util::CeilDiv(
+ static_cast<int64_t>(total_value_count_) - 1, values_per_block_);
+ const int64_t required_bit_widths =
+ required_blocks * static_cast<int64_t>(mini_blocks_per_block_);
+ const int64_t bytes_left = decoder_->bytes_left();
Review Comment:
`values_per_block_` and `mini_blocks_per_block_` originate from encoded
data; using `values_per_block_` as a divisor and multiplying to form
`required_bit_widths` can crash (division by zero) or overflow on corrupt
headers. Add explicit validation (e.g., `values_per_block_ > 0`,
`mini_blocks_per_block_ > 0`) before `CeilDiv`, and use checked/overflow-safe
multiplication when computing `required_bit_widths` so the bound check cannot
be bypassed via integer overflow.
##########
cpp/src/parquet/encoding_test.cc:
##########
@@ -1907,6 +1908,94 @@ TYPED_TEST(TestDeltaBitPackEncoding, BasicRoundTrip) {
}
}
+TYPED_TEST(TestDeltaBitPackEncoding, SingleValueSkipsMiniblockAllocation) {
+ using T = typename TypeParam::c_type;
+
+ // Header: 2^25 values per block, 2^20 miniblocks, 1 value, and first value
0.
+ const std::vector<uint8_t> encoded = {0x80, 0x80, 0x80, 0x10, 0x80,
+ 0x80, 0x40, 0x01, 0x00};
+ ::arrow::ProxyMemoryPool pool(default_memory_pool());
+ auto decoder = MakeTypedDecoder<TypeParam>(Encoding::DELTA_BINARY_PACKED,
+ this->descr_.get(), &pool);
+ T decoded = 1;
+
+ decoder->SetData(1, encoded.data(), static_cast<int>(encoded.size()));
+ ASSERT_EQ(decoder->Decode(&decoded, 1), 1);
+ EXPECT_EQ(decoded, 0);
+ EXPECT_EQ(pool.total_bytes_allocated(), 0);
+}
+
+TYPED_TEST(TestDeltaBitPackEncoding, RejectsMiniblockWidthsLargerThanInput) {
+ using T = typename TypeParam::c_type;
+
+ // Header: 2^25 values per block, 2^20 miniblocks, 2 values, and first value
0,
+ // followed by min delta 0 and no miniblock bit widths.
+ const std::vector<uint8_t> encoded = {0x80, 0x80, 0x80, 0x10, 0x80,
+ 0x80, 0x40, 0x02, 0x00, 0x00};
+ ::arrow::ProxyMemoryPool pool(default_memory_pool());
+ auto decoder = MakeTypedDecoder<TypeParam>(Encoding::DELTA_BINARY_PACKED,
+ this->descr_.get(), &pool);
+ std::vector<T> decoded(2);
+
+ EXPECT_THROW_THAT(
+ [&] {
+ decoder->SetData(2, encoded.data(), static_cast<int>(encoded.size()));
+ decoder->Decode(decoded.data(), static_cast<int>(decoded.size()));
+ },
+ ParquetException,
+ ::testing::Property(
+ &ParquetException::what,
+ ::testing::HasSubstr("the required number of miniblock bit widths "
+ "(1048576) exceeds the number of bytes
remaining "
+ "in the data (1)")));
Review Comment:
This depends on `EXPECT_THROW_THAT` invoking the callable; if it behaves
like the standard gtest `EXPECT_THROW`-style macros (which evaluate a
statement), constructing a lambda without calling it won’t execute the body and
the test won’t actually exercise the throw. Prefer passing a braced statement
block directly, or explicitly invoking the lambda (and apply the same pattern
to the other new `EXPECT_THROW_THAT` uses in this file).
##########
cpp/src/parquet/encoding_test.cc:
##########
@@ -1907,6 +1908,94 @@ TYPED_TEST(TestDeltaBitPackEncoding, BasicRoundTrip) {
}
}
+TYPED_TEST(TestDeltaBitPackEncoding, SingleValueSkipsMiniblockAllocation) {
+ using T = typename TypeParam::c_type;
+
+ // Header: 2^25 values per block, 2^20 miniblocks, 1 value, and first value
0.
+ const std::vector<uint8_t> encoded = {0x80, 0x80, 0x80, 0x10, 0x80,
+ 0x80, 0x40, 0x01, 0x00};
+ ::arrow::ProxyMemoryPool pool(default_memory_pool());
+ auto decoder = MakeTypedDecoder<TypeParam>(Encoding::DELTA_BINARY_PACKED,
+ this->descr_.get(), &pool);
+ T decoded = 1;
+
+ decoder->SetData(1, encoded.data(), static_cast<int>(encoded.size()));
+ ASSERT_EQ(decoder->Decode(&decoded, 1), 1);
+ EXPECT_EQ(decoded, 0);
+ EXPECT_EQ(pool.total_bytes_allocated(), 0);
+}
+
+TYPED_TEST(TestDeltaBitPackEncoding, RejectsMiniblockWidthsLargerThanInput) {
+ using T = typename TypeParam::c_type;
+
+ // Header: 2^25 values per block, 2^20 miniblocks, 2 values, and first value
0,
+ // followed by min delta 0 and no miniblock bit widths.
+ const std::vector<uint8_t> encoded = {0x80, 0x80, 0x80, 0x10, 0x80,
+ 0x80, 0x40, 0x02, 0x00, 0x00};
+ ::arrow::ProxyMemoryPool pool(default_memory_pool());
+ auto decoder = MakeTypedDecoder<TypeParam>(Encoding::DELTA_BINARY_PACKED,
+ this->descr_.get(), &pool);
+ std::vector<T> decoded(2);
+
+ EXPECT_THROW_THAT(
+ [&] {
+ decoder->SetData(2, encoded.data(), static_cast<int>(encoded.size()));
+ decoder->Decode(decoded.data(), static_cast<int>(decoded.size()));
+ },
+ ParquetException,
+ ::testing::Property(
+ &ParquetException::what,
+ ::testing::HasSubstr("the required number of miniblock bit widths "
+ "(1048576) exceeds the number of bytes
remaining "
+ "in the data (1)")));
Review Comment:
These assertions hard-code specific numeric values (e.g., `1048576`, `1`)
inside the expected substring, which makes the test brittle to future changes
in how the decoder computes/report bounds or formats messages. Consider
asserting on stable wording plus separately-derived expected numbers (built in
test code) or matching via a less exact substring/regex that still verifies the
correct error path.
--
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]
