Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls #40995

Merged
merged 9 commits into from
Apr 4, 2024
Merged

GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls #40995

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8a33bea
GH-40994: RleBooleanDecoder supports DecodeArrow with nulls
mapleFU Apr 4, 2024
f83b959
fix test
mapleFU Apr 4, 2024
b6f6552
resolve comment
mapleFU Apr 4, 2024
cb1a842
fix bug for decodeArrow
mapleFU Apr 4, 2024
bc77a15
add a test for DecodeArrow
mapleFU Apr 4, 2024
df385be
Merge branch 'rle-boolean-impl-decode-arrow' of github.com:mapleFU/ar…
mapleFU Apr 4, 2024
43d0ea7
resolve comment
mapleFU Apr 4, 2024
df15e10
resolve comment
mapleFU Apr 4, 2024
b0ba4e1
simplify variable
mapleFU Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 45 additions & 16 deletions cpp/src/parquet/encoding.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3143,27 +3143,56 @@ class RleBooleanDecoder : public DecoderImpl, virtual public BooleanDecoder {
int DecodeArrow(int num_values, int null_count, const uint8_t* valid_bits,
int64_t valid_bits_offset,
typename EncodingTraits<BooleanType>::Accumulator* out) override {
if (null_count != 0) {
// TODO(ARROW-34660): implement DecodeArrow with null slots.
ParquetException::NYI("RleBoolean DecodeArrow with null slots");
if (null_count == num_values) {
PARQUET_THROW_NOT_OK(out->AppendNulls(null_count));
return 0;
}
constexpr int kBatchSize = 1024;
std::array<bool, kBatchSize> values;
int sum_decode_count = 0;
do {
int current_batch = std::min(kBatchSize, num_values);
int decoded_count = decoder_->GetBatch(values.data(), current_batch);
if (decoded_count == 0) {
break;
// Reserve all values including nulls first
PARQUET_THROW_NOT_OK(out->Reserve(num_values));
const int num_boolean_values_sum = num_values - null_count;
mapleFU marked this conversation as resolved.
Show resolved Hide resolved
// Remaining boolean values to read
int num_boolean_values = num_boolean_values_sum;
int current_index_in_batch = 0;
int current_batch_size = 0;
auto next_boolean_batch = [&]() {
DCHECK_GT(num_boolean_values, 0);
DCHECK_EQ(current_index_in_batch, current_batch_size);
current_batch_size = std::min(num_boolean_values, kBatchSize);
int decoded_count = decoder_->GetBatch(values.data(), current_batch_size);
if (decoded_count != current_batch_size) {
mapleFU marked this conversation as resolved.
Show resolved Hide resolved
ParquetException::EofException();
}
sum_decode_count += decoded_count;
PARQUET_THROW_NOT_OK(out->Reserve(sum_decode_count));
for (int i = 0; i < decoded_count; ++i) {
PARQUET_THROW_NOT_OK(out->Append(values[i]));
num_boolean_values -= current_batch_size;
current_index_in_batch = 0;
};
if (null_count == 0) {
int sum_decode_count = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why introduce a separate variable? We know that we're going to return the original value of num_values.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it, it's copy-pasted from origin impl

do {
next_boolean_batch();
sum_decode_count += current_batch_size;
PARQUET_THROW_NOT_OK(
out->AppendValues(values.begin(), values.begin() + current_batch_size));
num_values -= current_batch_size;
current_index_in_batch = 0;
} while (num_values > 0);
return sum_decode_count;
}
auto next_value = [&]() -> bool {
if (current_index_in_batch == current_batch_size) {
next_boolean_batch();
DCHECK_GT(current_batch_size, 0);
}
num_values -= decoded_count;
} while (num_values > 0);
return sum_decode_count;
DCHECK_LT(current_index_in_batch, current_batch_size);
bool value = values[current_index_in_batch];
++current_index_in_batch;
return value;
};
VisitNullBitmapInline(
valid_bits, valid_bits_offset, num_values, null_count,
[&]() { out->UnsafeAppend(next_value()); }, [&]() { out->UnsafeAppendNull(); });
return num_boolean_values_sum;
}

int DecodeArrow(
Expand Down
9 changes: 4 additions & 5 deletions cpp/src/parquet/encoding_benchmark.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1518,11 +1518,10 @@ BENCHMARK_DEFINE_F(BM_DecodeArrowBooleanRle, DecodeArrowNonNull)
(benchmark::State& state) { DecodeArrowNonNullDenseBenchmark(state); }
BENCHMARK_REGISTER_F(BM_DecodeArrowBooleanRle, DecodeArrowNonNull)
->Range(MIN_RANGE, MAX_RANGE);
// TODO(mwish): RleBoolean not implemented DecodeArrow with null slots yet.
// BENCHMARK_DEFINE_F(BM_DecodeArrowBooleanRle, DecodeArrowWithNull)
//(benchmark::State& state) { DecodeArrowWithNullDenseBenchmark(state); }
// BENCHMARK_REGISTER_F(BM_DecodeArrowBooleanRle, DecodeArrowWithNull)
// ->Apply(BooleanWithNullCustomArguments);
BENCHMARK_DEFINE_F(BM_DecodeArrowBooleanRle, DecodeArrowWithNull)
(benchmark::State& state) { DecodeArrowWithNullDenseBenchmark(state); }
BENCHMARK_REGISTER_F(BM_DecodeArrowBooleanRle, DecodeArrowWithNull)
->Apply(BooleanWithNullCustomArguments);
mapleFU marked this conversation as resolved.
Show resolved Hide resolved

BENCHMARK_DEFINE_F(BM_DecodeArrowBooleanPlain, DecodeArrow)
(benchmark::State& state) { DecodeArrowDenseBenchmark(state); }
Expand Down
2 changes: 0 additions & 2 deletions cpp/src/parquet/encoding_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,8 +963,6 @@ TYPED_TEST(EncodingAdHocTyped, ByteStreamSplitArrowDirectPut) {
}

TYPED_TEST(EncodingAdHocTyped, RleArrowDirectPut) {
// TODO: test with nulls once RleBooleanDecoder::DecodeArrow supports them
this->null_probability_ = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have coverage yet?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm what does this mean? The case is covered here

for (auto seed : {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}) {
this->Rle(seed);
}
Expand Down
Loading