Copilot commented on code in PR #50422:
URL: https://github.com/apache/arrow/pull/50422#discussion_r3550255054
##########
cpp/src/arrow/util/rle_encoding_internal.h:
##########
@@ -515,6 +573,84 @@ class RleBitPackedDecoder {
int64_t valid_bits_offset, rle_size_t
null_count);
};
+/// Minimal decoder for legacy bit packed encoding (BIT_PACKED = 4).
+///
+/// The number of values that the decoder can represent is up to 2^31 - 1.
+template <typename T>
+class BitPackedDecoder : private BitPackedRunDecoder<T> {
+ private:
+ using Base = BitPackedRunDecoder<T>;
+
+ public:
+ /// The type in which the data should be decoded.
+ using value_type = T;
+
+ using Base::Advance;
+
+ BitPackedDecoder() noexcept = default;
+
+ /// Create a decoder object.
+ ///
+ /// Unless explicitly given, then number of values is deduced from the data
size,
+ /// in which case it may read more values from the input stream than the user
+ /// intended (to reach final byte alignment). This option is mandatory if the
+ /// `value_bit_width` is zero.
+ ///
+ /// @param data and data_size are the raw bytes to decode.
+ /// @param value_bit_width is the size in bits of each encoded value.
+ /// @param value_count is an optional number of values in the run.
+ BitPackedDecoder(const uint8_t* data, rle_size_t data_size, rle_size_t
value_bit_width,
+ rle_size_t value_count = -1) noexcept {
+ Reset(data, data_size, value_bit_width, value_count);
+ }
+
+ void Reset(const uint8_t* data, rle_size_t data_size, rle_size_t
value_bit_width,
+ rle_size_t value_count = -1) noexcept {
+ ARROW_DCHECK_GE(value_bit_width, 0);
+ ARROW_DCHECK_LE(value_bit_width, 64);
+
+ value_bit_width_ = value_bit_width;
+ if (value_count < 0) {
+ ARROW_DCHECK_GT(value_bit_width, 0);
+ value_count = (static_cast<int64_t>(data_size) * 8) / value_bit_width;
+ }
Review Comment:
`BitPackedDecoder::Reset` can still divide by zero in release builds when
`value_bit_width == 0` and the caller doesn’t provide `value_count` (the
current guard is only an `ARROW_DCHECK`). Even if current call sites always
pass `value_count`, this is an easy footgun for future use and can lead to
UB/crashes.
##########
cpp/src/parquet/column_reader.cc:
##########
@@ -191,6 +203,22 @@ int LevelDecoder::Decode(int batch_size, int16_t* levels) {
return num_decoded;
}
+int LevelDecoder::Skip(int batch_size) {
+ const int num_values = std::min(num_values_remaining_, batch_size);
+ const int num_advanced = decoder_->Advance(num_values);
+ ARROW_DCHECK_EQ(num_values, num_advanced);
+ num_values_remaining_ -= num_advanced;
+ return num_advanced;
+}
Review Comment:
`LevelDecoder::Skip` assumes the underlying decoder advances exactly
`num_values` and only checks it with `ARROW_DCHECK`. In release builds a short
advance (e.g. truncated/corrupt page) would silently desynchronize the page
state and downstream callers still consume `batch_size` values. This should be
a runtime check that throws.
##########
cpp/src/parquet/column_reader.cc:
##########
@@ -1137,40 +1225,40 @@ int64_t TypedColumnReaderImpl<DType>::ReadBatch(int64_t
batch_size, int16_t* def
return total_values;
}
-template <typename DType>
-void TypedColumnReaderImpl<DType>::InitScratchForSkip() {
- if (this->scratch_for_skip_ == nullptr) {
- int value_size = type_traits<DType::type_num>::value_byte_size;
- this->scratch_for_skip_ = AllocateBuffer(
- this->pool_, kSkipScratchBatchSize * std::max<int>(sizeof(int16_t),
value_size));
- }
-}
-
template <typename DType>
int64_t TypedColumnReaderImpl<DType>::Skip(int64_t num_values_to_skip) {
int64_t values_to_skip = num_values_to_skip;
// Optimization: Do not call HasNext() when values_to_skip == 0.
while (values_to_skip > 0 && HasNext()) {
// If the number of values to skip is more than the number of undecoded
values, skip
- // the Page.
+ // the whole Page without decoding levels or values.
const int64_t available_values = this->available_values_current_page();
if (values_to_skip >= available_values) {
values_to_skip -= available_values;
this->ConsumeBufferedValues(available_values);
} else {
- // We need to read this Page
- // Jump to the right offset in the Page
- int64_t values_read = 0;
- InitScratchForSkip();
- ARROW_DCHECK_NE(this->scratch_for_skip_, nullptr);
- do {
- int64_t batch_size = std::min(kSkipScratchBatchSize, values_to_skip);
- values_read = ReadBatch(static_cast<int>(batch_size),
- scratch_for_skip_->mutable_data_as<int16_t>(),
- scratch_for_skip_->mutable_data_as<int16_t>(),
- scratch_for_skip_->mutable_data_as<T>(),
&values_read);
- values_to_skip -= values_read;
- } while (values_read > 0 && values_to_skip > 0);
+ // Skip within the current Page. Since `values_to_skip <
available_values`, the
+ // whole batch fits inside this Page and no page boundary is crossed.
+ const int batch_size = static_cast<int>(values_to_skip);
+
+ // Advance the definition levels, counting how many correspond to present
+ // (non-null) values that must be skipped in the data decoder.
+ int64_t non_null_values_to_skip = batch_size;
+ if (this->max_def_level() > 0) {
+ const auto [count, advanced] =
+ this->definition_level_decoder_.CountUpTo(this->max_def_level(),
batch_size);
+ non_null_values_to_skip = count;
+ ARROW_DCHECK_EQ(advanced, batch_size);
+ }
+ // Advance the repetition levels; their values are not needed.
+ if (this->max_rep_level() > 0) {
+ this->repetition_level_decoder_.Skip(batch_size);
+ }
+ // Skip the corresponding data values.
+ this->current_decoder_.Skip(non_null_values_to_skip, this->pool_);
+
+ this->ConsumeBufferedValues(batch_size);
+ values_to_skip -= batch_size;
Review Comment:
`TypedColumnReaderImpl::Skip` consumes `batch_size` buffered values
unconditionally, but it doesn’t enforce (in release builds) that the level
decoders and value decoder actually advanced the requested counts (the only
checks are `ARROW_DCHECK`s or ignored return values). On truncated/corrupt
input this can desynchronize the decoders and potentially lead to incorrect
skipping or subsequent decode failures. Add runtime checks and throw if any of
the advances are short.
##########
cpp/src/parquet/column_reader.cc:
##########
@@ -191,6 +203,22 @@ int LevelDecoder::Decode(int batch_size, int16_t* levels) {
return num_decoded;
}
+int LevelDecoder::Skip(int batch_size) {
+ const int num_values = std::min(num_values_remaining_, batch_size);
+ const int num_advanced = decoder_->Advance(num_values);
+ ARROW_DCHECK_EQ(num_values, num_advanced);
+ num_values_remaining_ -= num_advanced;
+ return num_advanced;
+}
+
+std::pair<int, int> LevelDecoder::CountUpTo(int16_t value, int batch_size) {
+ const int num_values = std::min(num_values_remaining_, batch_size);
+ const auto result = decoder_->CountUpTo(value, num_values);
+ ARROW_DCHECK_EQ(num_values, result.advanced_count);
+ num_values_remaining_ -= result.advanced_count;
+ return {result.count, result.advanced_count};
+}
Review Comment:
`LevelDecoder::CountUpTo` also relies on `ARROW_DCHECK` to assert it
advanced exactly `num_values`. In release builds a short advance would be
ignored, but callers (e.g. `TypedColumnReaderImpl::Skip`) assume full
advancement and will consume the full batch, leaving decoder state
inconsistent. Prefer a runtime check and throw on mismatch.
--
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]
