Copilot commented on code in PR #50682:
URL: https://github.com/apache/arrow/pull/50682#discussion_r3664181283
##########
cpp/src/parquet/column_reader.cc:
##########
@@ -189,16 +191,15 @@ void LevelDecoder::SetDataV2(int32_t num_bytes, int16_t
max_level,
int LevelDecoder::Decode(int batch_size, int16_t* levels) {
const int num_values = std::min(num_values_remaining_, batch_size);
- const int num_decoded = impl_->GetBatch(levels, num_values);
- if (num_decoded > 0) {
- internal::MinMax min_max = internal::FindMinMax(levels, num_decoded);
- if (ARROW_PREDICT_FALSE(min_max.min < 0 || min_max.max > max_level_)) {
- std::stringstream ss;
- ss << "Malformed levels. min: " << min_max.min << " max: " << min_max.max
- << " out of range. Max Level: " << max_level_;
- throw ParquetException(ss.str());
- }
- }
+ const int num_decoded =
+ impl_->GetBatch(levels, num_values, [max = max_level_](int16_t value) {
+ if (ARROW_PREDICT_FALSE(value < 0 || value > max)) {
+ std::stringstream ss;
+ ss << "Malformed level with value " << value << " is not in the
range [0, "
+ << max << ")";
+ throw ParquetException(ss.str());
Review Comment:
The exception message says the valid range is "[0, max)" and prints a
closing ")", but the check is `value > max` (i.e., `max` is inclusive). This is
confusing and looks like a typo in the range formatting.
##########
cpp/src/arrow/util/rle_encoding_internal.h:
##########
@@ -322,19 +326,25 @@ class RleRunDecoder {
}
/// Get the next value and return false if there are no more.
- [[nodiscard]] constexpr bool Get(value_type* out_value, rle_size_t
value_bit_width) {
- return GetBatch(out_value, 1, value_bit_width) == 1;
+ template <typename Func = RleDefault>
+ [[nodiscard]] constexpr bool Get(value_type* out_value, rle_size_t
value_bit_width,
+ Func&& validator = {}) {
+ return GetBatch(out_value, 1, value_bit_width,
std::forward<Func>(validator)) == 1;
}
/// Get a batch of values return the number of decoded elements.
/// May write fewer elements to the output than requested if there are not
enough values
/// left.
+ template <typename Func = RleDefault>
[[nodiscard]] rle_size_t GetBatch(value_type* out, rle_size_t batch_size,
- rle_size_t value_bit_width) {
+ rle_size_t value_bit_width, Func&&
validator = {}) {
if (ARROW_PREDICT_FALSE(remaining_count_ == 0)) {
return 0;
}
+ if constexpr (!std::is_same_v<std::decay_t<Func>, RleDefault>) {
+ validator(value_);
+ }
Review Comment:
`RleRunDecoder::GetBatch` now calls `validator(value_)` even when
`batch_size == 0` (it only checks `remaining_count_ == 0`). That can make
`GetBatch(..., 0, ..., validator)` throw despite decoding zero values, which is
surprising and can break callers that legitimately request an empty batch.
--
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]
