Copilot commented on code in PR #50422:
URL: https://github.com/apache/arrow/pull/50422#discussion_r3550345218
##########
cpp/src/arrow/util/rle_encoding_internal.h:
##########
@@ -515,6 +573,85 @@ 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);
+ const int64_t data_bit_size = static_cast<int64_t>(data_size) * 8;
+ value_count = static_cast<rle_size_t>(data_bit_size / value_bit_width);
+ }
Review Comment:
`BitPackedDecoder::Reset` still divides by `value_bit_width` when
`value_count < 0`. In release builds `ARROW_DCHECK_GT(value_bit_width, 0)` is
compiled out, so calling `Reset(..., value_bit_width=0)` without an explicit
`value_count` can still trigger a divide-by-zero. Since the API documents that
`value_count` is mandatory for zero-width runs, this should fail fast at
runtime (not just in debug).
##########
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::Skip` / `CountUpTo` rely on `ARROW_DCHECK_EQ(...)` to verify
the underlying decoder advanced the requested number of values. In release
builds this check is compiled out; if the level stream is truncated/corrupt and
advances fewer values, `num_values_remaining_` and the caller’s page accounting
can get out of sync, potentially leading to incorrect decoding. These methods
should throw on mismatch (similar to other level/value count checks in this
file).
##########
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);
Review Comment:
In `TypedColumnReaderImpl::Skip`, the return value of
`current_decoder_.Skip(...)` is ignored. If the data decoder reaches EOF early
(truncated/corrupt page), this method will still
`ConsumeBufferedValues(batch_size)`, desynchronizing buffered value counts from
the underlying decoder state. This should validate that the requested physical
values were actually skipped and throw on short reads (mirroring
`ReadAndThrowAwayValues`).
--
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]
