Copilot commented on code in PR #50422:
URL: https://github.com/apache/arrow/pull/50422#discussion_r3558960137
##########
cpp/src/arrow/util/rle_encoding_internal.h:
##########
@@ -363,6 +384,29 @@ class BitPackedRunDecoder {
return steps;
}
+ /// Advance and count the number of occurrence of a values.
+ ///
+ /// The count is limited to at most the next `batch_size` items.
+ /// @return The matching value count and number of of element that were
processed.
Review Comment:
Docstring has grammatical typos ("occurrence of a values", "number of of
element"). The same wording appears in other `CountUpTo` comments below; please
fix consistently.
##########
cpp/src/parquet/column_reader.cc:
##########
@@ -1137,40 +1231,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 =
+ this->definition_level_decoder_.CountUpTo(this->max_def_level(),
batch_size);
+ non_null_values_to_skip = count.matching_count;
+ ARROW_DCHECK_EQ(count.processed_count, 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);
+
Review Comment:
`TypedColumnReaderImpl::Skip` now assumes level decoders and the value
decoder always advance exactly `batch_size` / `non_null_values_to_skip` and
uses only `ARROW_DCHECK` for validation. In non-debug builds, if a
truncated/corrupt page causes any of these operations to advance fewer
elements, the code will still call `ConsumeBufferedValues(batch_size)`, leaving
the reader state inconsistent (potentially leading to mis-decoding or later
crashes). Please make these checks unconditional and fail fast (or otherwise
avoid consuming buffered values when a short advance occurs).
--
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]
