pitrou commented on code in PR #50422:
URL: https://github.com/apache/arrow/pull/50422#discussion_r3552941476
##########
cpp/src/arrow/util/rle_encoding_test.cc:
##########
@@ -1257,6 +1404,19 @@ void DoTestGetBatchSpacedRoundtrip() {
CheckRoundTrip<ArrowType>(*array, case_.bit_width, /* spaced= */ false,
/* parts= */ 3);
+ // Tests for Advance
+ CheckAdvance<ArrowType>(*array, case_.bit_width);
+ CheckAdvance<ArrowType>(*array->Slice(1), case_.bit_width);
+
+ // Tests for CountUpTo, counting a value present in the data (the first
one)
+ // and a value that may not be (the max encodable one).
+ const auto first = static_cast<T>(
+ static_cast<const typename
TypeTraits<ArrowType>::ArrayType&>(*array).Value(0));
+ const auto max_value = static_cast<T>((T{1} << (case_.bit_width - 1)) - 1);
Review Comment:
Why `case_.bit_width - 1` and not `case_.bit_width`?
##########
cpp/src/arrow/util/rle_encoding_test.cc:
##########
@@ -1102,6 +1143,112 @@ void CheckRoundTrip(const Array& data, int bit_width,
bool spaced, int32_t parts
}
}
+/// Check RleBitPackedDecoder::Advance, which spans multiple runs through the
+/// parser (unlike the per-run decoder Advance).
+///
+/// Nulls are treated as regular data (i.e. their raw value is encoded and
+/// decoded). Advance and GetBatch are interleaved so that run boundaries are
+/// crossed and partial runs consumed, verifying decoded values as we go.
+template <typename Type>
+void CheckAdvance(const Array& data, int bit_width) {
+ using ArrayType = typename TypeTraits<Type>::ArrayType;
+ using value_type = typename Type::c_type;
+
+ const auto data_size = static_cast<rle_size_t>(data.length());
+ const value_type* data_values = static_cast<const
ArrayType&>(data).raw_values();
+
+ ARROW_SCOPED_TRACE("bit_width = ", bit_width, ", data_size = ", data_size);
+
+ // Encode all values into `buffer`.
+ const auto buffer = EncodeTestArray<Type>(data, bit_width);
+
+ // Interleave Advance and GetBatch, verifying decoded values against the
original.
+ RleBitPackedDecoder<value_type> decoder(buffer.data(),
static_cast<int>(buffer.size()),
+ bit_width);
+ rle_size_t pos = 0;
+ auto advance = [&](rle_size_t* pos, rle_size_t n) {
+ n = std::min(n, data_size - *pos);
+ EXPECT_EQ(decoder.Advance(n), n);
+ *pos += n;
+ };
+ auto read = [&](rle_size_t* pos, rle_size_t n) {
+ n = std::min(n, data_size - *pos);
+ std::vector<value_type> got(n);
+ EXPECT_EQ(decoder.GetBatch(got.data(), n), n);
+ for (rle_size_t i = 0; i < n; ++i) {
+ EXPECT_EQ(got[i], data_values[*pos + i]) << "at position " << (*pos + i);
+ }
+ *pos += n;
+ };
+
+ // Advance in a `step` small relative to the data size, so we span many
+ // iterations and repeatedly cross run boundaries.
+ const rle_size_t step = std::max<rle_size_t>(data_size / 16, 1);
+ int iter = 0;
+ while (pos < data_size) {
+ // Some way to make various successions of `read` of `advance`
+ if (bit_width % 2 == iter % 3) {
+ read(&pos, step);
+ }
+ advance(&pos, step);
+ ++iter;
+ }
+ // Note: we do not assert exhaustion here because the encoder pads the last
+ // literal group to a multiple of 8 with zeros, leaving up to 7 extra values.
+}
+
+/// Check RleBitPackedDecoder::CountUpTo, which spans multiple runs through the
+/// parser (unlike the per-run decoder CountUpTo).
+///
+/// Nulls are treated as regular data (i.e. their raw value is encoded and
+/// decoded). The counts returned over successive batches are compared against
a
+/// naive count over the original data.
+template <typename Type>
+void CheckCountUpTo(const Array& data, int bit_width, typename Type::c_type
value) {
+ using ArrayType = typename TypeTraits<Type>::ArrayType;
+ using value_type = typename Type::c_type;
+
+ const auto data_size = static_cast<rle_size_t>(data.length());
+ const value_type* data_values = static_cast<const
ArrayType&>(data).raw_values();
Review Comment:
Nit: use `checked_cast` for such dynamic downcast.
##########
cpp/src/parquet/column_reader.cc:
##########
@@ -629,6 +657,70 @@ std::unique_ptr<PageReader>
PageReader::Open(std::shared_ptr<ArrowInputStream> s
namespace {
+/// Wrapper around a `TypedDecoder` pointer to skip values.
+///
+/// Use a scratch buffer to decode values into that buffer.
+/// This was migrated here from a historical implementation.
+/// Ideally all decoders would implement a `Skip` functionality that would at
best
+/// avoid decoding, and at worst, decode without intermediary allocation.
Review Comment:
Ping on this comment.
--
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]
