You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mapleFU (via GitHub)" <gi...@apache.org> on 2024/04/04 05:15:14 UTC
[PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
mapleFU opened a new pull request, #40995:
URL: https://github.com/apache/arrow/pull/40995
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Supports DecodeArrow with nulls in RleBooleanDecoder
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Supports DecodeArrow with nulls in RleBooleanDecoder
### Are these changes tested?
Yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
currently not
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #40995:
URL: https://github.com/apache/arrow/pull/40995#discussion_r1551277327
##########
cpp/src/parquet/encoding.cc:
##########
@@ -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;
Review Comment:
```suggestion
const int num_non_null_values = num_values - null_count;
```
##########
cpp/src/parquet/encoding.cc:
##########
@@ -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;
+ // 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) {
Review Comment:
Add UNLIKELY?
##########
cpp/src/parquet/encoding_test.cc:
##########
@@ -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;
Review Comment:
Do we have coverage yet?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40995:
URL: https://github.com/apache/arrow/pull/40995#issuecomment-2036538527
Well, it seems the benchmarks fail:
```
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:1024/null_in_ten_thousand:100 3572 ns 3572 ns 191956 bytes_per_second=34.1742Mi/s items_per_second=286.674M/s
terminate called after throwing an instance of 'parquet::ParquetException'
what(): Unexpected end of stream
Abandon
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40995:
URL: https://github.com/apache/arrow/pull/40995#issuecomment-2036618129
Revision: cb1a8421780e92a01b5807ea8d85e7e7748ef436
Submitted crossbow builds: [ursacomputing/crossbow @ actions-35133d7bec](https://github.com/ursacomputing/crossbow/branches/all?query=actions-35133d7bec)
|Task|Status|
|----|------|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140856/job/23432640554)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140868/job/23432640633)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140654/job/23432639378)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/runs/23432642057)|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140845/job/23432640548)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140861/job/23432640759)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140638/job/23432639440)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140782/job/23432640065)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140662/job/23432639538)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140802/job/23432640331)|
|test-ubuntu-20.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140765/job/23432639967)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140700/job/23432639613)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140706/job/23432639777)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140921/job/23432640905)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140751/job/23432639930)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140881/job/23432640840)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/8552140756/job/23432640003)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #40995:
URL: https://github.com/apache/arrow/pull/40995#issuecomment-2036536344
benchmark number wouldn't be great, because RleDecoder need to decode to `bool` first, and unpack8 goes to unpack32.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #40995:
URL: https://github.com/apache/arrow/pull/40995#discussion_r1551300664
##########
cpp/src/parquet/encoding_test.cc:
##########
@@ -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;
Review Comment:
Hmm what does this mean? The case is covered here
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40995:
URL: https://github.com/apache/arrow/pull/40995#issuecomment-2036206854
:warning: GitHub issue #40994 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #40995:
URL: https://github.com/apache/arrow/pull/40995#discussion_r1551527804
##########
cpp/src/parquet/encoding.cc:
##########
@@ -3143,27 +3143,57 @@ 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_non_null_values = num_values - null_count;
+ // Remaining boolean values to read
+ int num_remain_non_null_values = num_non_null_values;
+ int current_index_in_batch = 0;
+ int current_batch_size = 0;
+ auto next_boolean_batch = [&]() {
+ DCHECK_GT(num_remain_non_null_values, 0);
+ DCHECK_EQ(current_index_in_batch, current_batch_size);
+ current_batch_size = std::min(num_remain_non_null_values, kBatchSize);
+ int decoded_count = decoder_->GetBatch(values.data(), current_batch_size);
+ if (ARROW_PREDICT_FALSE(decoded_count != current_batch_size)) {
+ // required values is more than values in decoder.
+ 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_remain_non_null_values -= current_batch_size;
+ current_index_in_batch = 0;
+ };
+ if (null_count == 0) {
+ int sum_decode_count = 0;
Review Comment:
Removed it, it's copy-pasted from origin impl
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40995:
URL: https://github.com/apache/arrow/pull/40995#discussion_r1551507618
##########
cpp/src/parquet/encoding.cc:
##########
@@ -3143,27 +3143,57 @@ 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_non_null_values = num_values - null_count;
+ // Remaining boolean values to read
+ int num_remain_non_null_values = num_non_null_values;
+ int current_index_in_batch = 0;
+ int current_batch_size = 0;
+ auto next_boolean_batch = [&]() {
+ DCHECK_GT(num_remain_non_null_values, 0);
+ DCHECK_EQ(current_index_in_batch, current_batch_size);
+ current_batch_size = std::min(num_remain_non_null_values, kBatchSize);
+ int decoded_count = decoder_->GetBatch(values.data(), current_batch_size);
+ if (ARROW_PREDICT_FALSE(decoded_count != current_batch_size)) {
+ // required values is more than values in decoder.
+ 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_remain_non_null_values -= current_batch_size;
+ current_index_in_batch = 0;
+ };
+ if (null_count == 0) {
+ int sum_decode_count = 0;
Review Comment:
Why introduce a separate variable? We _know_ that we're going to return the original value of `num_values`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40995:
URL: https://github.com/apache/arrow/pull/40995#issuecomment-2037246625
This is a nice improvement, thank you @mapleFU !
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #40995:
URL: https://github.com/apache/arrow/pull/40995#issuecomment-2036373173
@pitrou @wgtmac
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40995:
URL: https://github.com/apache/arrow/pull/40995#discussion_r1551132063
##########
cpp/src/parquet/encoding_benchmark.cc:
##########
@@ -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);
Review Comment:
What do the benchmark numbers look like?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #40995:
URL: https://github.com/apache/arrow/pull/40995#issuecomment-2036589910
RLE
```
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:1024/null_in_ten_thousand:1 1490 ns 1490 ns 507916 bytes_per_second=81.913Mi/s items_per_second=687.136M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:4096/null_in_ten_thousand:1 3761 ns 3760 ns 187492 bytes_per_second=129.862Mi/s items_per_second=1.08936G/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:16384/null_in_ten_thousand:1 94859 ns 94860 ns 7488 bytes_per_second=20.5896Mi/s items_per_second=172.718M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:65536/null_in_ten_thousand:1 439572 ns 439574 ns 1660 bytes_per_second=17.7729Mi/s items_per_second=149.09M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:1024/null_in_ten_thousand:100 4670 ns 4672 ns 146128 bytes_per_second=26.1272Mi/s items_per_second=219.171M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:4096/null_in_ten_thousand:100 15912 ns 15912 ns 44211 bytes_per_second=30.6856Mi/s items_per_second=257.41M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:16384/null_in_ten_thousand:100 105908 ns 105905 ns 6467 bytes_per_second=18.4422Mi/s items_per_second=154.705M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:65536/null_in_ten_thousand:100 467966 ns 467968 ns 1418 bytes_per_second=16.6945Mi/s items_per_second=140.044M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:1024/null_in_ten_thousand:1000 4134 ns 4134 ns 169739 bytes_per_second=29.5278Mi/s items_per_second=247.697M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:4096/null_in_ten_thousand:1000 15159 ns 15159 ns 46279 bytes_per_second=32.2106Mi/s items_per_second=270.202M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:16384/null_in_ten_thousand:1000 101632 ns 101632 ns 7019 bytes_per_second=19.2175Mi/s items_per_second=161.208M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:65536/null_in_ten_thousand:1000 454805 ns 454805 ns 1562 bytes_per_second=17.1777Mi/s items_per_second=144.097M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:1024/null_in_ten_thousand:5000 4355 ns 4355 ns 157860 bytes_per_second=28.0298Mi/s items_per_second=235.131M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:4096/null_in_ten_thousand:5000 15928 ns 15928 ns 44130 bytes_per_second=30.6553Mi/s items_per_second=257.155M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:16384/null_in_ten_thousand:5000 114474 ns 114474 ns 5980 bytes_per_second=17.0617Mi/s items_per_second=143.124M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:65536/null_in_ten_thousand:5000 529471 ns 529466 ns 1303 bytes_per_second=14.7554Mi/s items_per_second=123.778M/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:1024/null_in_ten_thousand:10000 403 ns 403 ns 1734553 bytes_per_second=302.976Mi/s items_per_second=2.54155G/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:4096/null_in_ten_thousand:10000 406 ns 406 ns 1703918 bytes_per_second=1.17413Gi/s items_per_second=10.0857G/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:16384/null_in_ten_thousand:10000 439 ns 439 ns 1572933 bytes_per_second=4.34285Gi/s items_per_second=37.3048G/s
BM_DecodeArrowBooleanRle/DecodeArrowWithNull/num_values:65536/null_in_ten_thousand:10000 660 ns 660 ns 1053931 bytes_per_second=11.5619Gi/s items_per_second=99.316G/s
```
PLAIN:
```
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:1024/null_in_ten_thousand:1 709 ns 709 ns 1202751 bytes_per_second=172.176Mi/s items_per_second=1.44432G/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:4096/null_in_ten_thousand:1 602 ns 602 ns 924857 bytes_per_second=810.604Mi/s items_per_second=6.79984G/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:16384/null_in_ten_thousand:1 25105 ns 25105 ns 22236 bytes_per_second=77.7987Mi/s items_per_second=652.623M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:65536/null_in_ten_thousand:1 99624 ns 99607 ns 7260 bytes_per_second=78.4332Mi/s items_per_second=657.945M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:1024/null_in_ten_thousand:100 3402 ns 3402 ns 208561 bytes_per_second=35.8773Mi/s items_per_second=300.96M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:4096/null_in_ten_thousand:100 10747 ns 10747 ns 70905 bytes_per_second=45.4327Mi/s items_per_second=381.117M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:16384/null_in_ten_thousand:100 65832 ns 65832 ns 8721 bytes_per_second=29.6684Mi/s items_per_second=248.877M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:65536/null_in_ten_thousand:100 309811 ns 309809 ns 2156 bytes_per_second=25.2172Mi/s items_per_second=211.537M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:1024/null_in_ten_thousand:1000 4216 ns 4216 ns 144072 bytes_per_second=28.9514Mi/s items_per_second=242.862M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:4096/null_in_ten_thousand:1000 19173 ns 19173 ns 29821 bytes_per_second=25.4678Mi/s items_per_second=213.639M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:16384/null_in_ten_thousand:1000 117705 ns 117706 ns 5677 bytes_per_second=16.5933Mi/s items_per_second=139.195M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:65536/null_in_ten_thousand:1000 527801 ns 527790 ns 1359 bytes_per_second=14.8023Mi/s items_per_second=124.171M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:1024/null_in_ten_thousand:5000 5134 ns 5134 ns 128720 bytes_per_second=23.7753Mi/s items_per_second=199.442M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:4096/null_in_ten_thousand:5000 18807 ns 18807 ns 36614 bytes_per_second=25.9622Mi/s items_per_second=217.787M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:16384/null_in_ten_thousand:5000 122685 ns 122684 ns 6010 bytes_per_second=15.92Mi/s items_per_second=133.547M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:65536/null_in_ten_thousand:5000 611359 ns 611363 ns 1003 bytes_per_second=12.7788Mi/s items_per_second=107.197M/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:1024/null_in_ten_thousand:10000 784 ns 784 ns 779600 bytes_per_second=155.664Mi/s items_per_second=1.3058G/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:4096/null_in_ten_thousand:10000 1789 ns 1789 ns 390985 bytes_per_second=272.889Mi/s items_per_second=2.28916G/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:16384/null_in_ten_thousand:10000 5843 ns 5843 ns 125562 bytes_per_second=334.283Mi/s items_per_second=2.80417G/s
BM_DecodeArrowBooleanPlain/DecodeArrowWithNull/num_values:65536/null_in_ten_thousand:10000 22560 ns 22561 ns 30684 bytes_per_second=346.29Mi/s items_per_second=2.90489G/s
```
For all-nulls seems that Rle is faster because it hacks the logic without any checking
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40995:
URL: https://github.com/apache/arrow/pull/40995#discussion_r1551131788
##########
cpp/src/parquet/encoding.cc:
##########
@@ -3143,27 +3143,52 @@ 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;
+ const 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) {
+ 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]));
+ current_index_in_batch = 0;
+ };
+ if (null_count == 0) {
+ int sum_decode_count = 0;
+ 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();
}
- num_values -= decoded_count;
- } while (num_values > 0);
- return sum_decode_count;
+ bool value = values.at(current_index_in_batch);
Review Comment:
Might this check bounds at runtime? We probably want to avoid that.
```suggestion
bool value = values[current_index_in_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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40995:
URL: https://github.com/apache/arrow/pull/40995#issuecomment-2036612437
@github-actions crossbow submit -g cpp
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40994: [C++][Parquet] RleBooleanDecoder supports DecodeArrow with nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #40995:
URL: https://github.com/apache/arrow/pull/40995
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org