GH-41032: [C++][Parquet] Bugfixes and more tests in boolean arrow decoding by mapleFU · Pull Request #41037 · apache/arrow

mapleFU · 2024-04-05T16:14:20Z

Rationale for this change

Previous patches ( #40876 , #40995 ) introduce some bugs in boolean decoding. This patch add tests for DecodeArrow and fix the bug

What changes are included in this PR?

Add tests for DecodeArrow with nulls and without nulls
Fix 2 bugs

Are these changes tested?

Yes

Are there any user-facing changes?

no

GitHub Issue: [C++] Regression in PlainBooleanDecoder::DecodeArrow #41032

mapleFU · 2024-04-05T16:15:03Z

cc @pitrou @wgtmac

mapleFU · 2024-04-05T16:18:31Z

cpp/src/parquet/encoding_test.cc

Only true_prob = 0.999 but not 1 trigger the bug in issue, because if true_prob = 1, it goes to fastpath without hittin a all 1 block

mapleFU · 2024-04-05T16:39:29Z

cpp/src/parquet/encoding.cc

+        // set current_index_in_batch to current_batch_size means
+        // the whole batch is totally consumed.
+        current_index_in_batch = current_batch_size;


This is another bug I found by testing. "Release" will not trouble by this, but DCHECK might failed in next_boolean_batch

What's the point of current_index_in_batch exactly? It always ends up having the same value.

It's only useful in per-value next when null_count != 0. next_boolean_batch will check all values in this batch is consumed (.i.e current_index_in_batch == batch_size). When null_count != 0, it's useless actually, but it uses next_boolean_batch here.

mapleFU · 2024-04-05T16:44:26Z

Sorry for add these two untested path, I'll add more test rather than only benchmarks later

wgtmac · 2024-04-07T15:28:01Z

cpp/src/parquet/encoding.cc

  if (ARROW_PREDICT_FALSE(!bit_reader_->Advance(values_decoded))) {
-    ParquetException::EofException();
+    ParquetException::EofException(
+        "PlainDecoder doesn't have enough values in bit_reader_");


IMO, we should not expose variable name to user.

You're right. I change bit_reader_ to page now

pitrou · 2024-04-08T12:55:06Z

@github-actions crossbow submit -g cpp

pitrou · 2024-04-08T12:57:05Z

cpp/src/parquet/encoding_test.cc

+
+    // Initialize input_data_ for the encoder from the expected_array_ values
+    const auto& boolean_array =
+        static_cast<const ::arrow::BooleanArray&>(*expected_dense_);


Nit: checked_cast?

github-actions · 2024-04-08T12:57:39Z

Revision: 9c7e01c

Submitted crossbow builds: ursacomputing/crossbow @ actions-bbcae774eb

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14

pitrou · 2024-04-08T12:58:36Z

cpp/src/parquet/encoding_test.cc

+      for (double true_prob : true_probabilities_) {
+        for (int read_batch_size : this->read_batch_sizes_) {
+          // Resume the state of decoder
+          InitTestCase(encoding, np, true_prob);


This is regenerating the input data for each batch size, can we avoid this?

pitrou · 2024-04-08T12:58:48Z

cpp/src/parquet/encoding_test.cc

+    for (auto true_prob : true_probabilities_) {
+      for (int read_batch_size : this->read_batch_sizes_) {
+        // Resume the decoder
+        InitTestCase(encoding, /*null_probability=*/0, true_prob);


pitrou · 2024-04-08T12:59:23Z

cpp/src/parquet/encoding_test.cc

+  std::shared_ptr<Buffer> buffer_;
+};
+
+TEST_F(TestBooleanArrowDecoding, CheckDecodeArrowUsingDenseBuilderPlain) {


What is a "dense builder"? Can we rename this? For example DecodeArrowPlain.

pitrou · 2024-04-08T14:14:36Z

Thanks a lot for the quick submission @mapleFU !

conbench-apache-arrow · 2024-04-08T19:04:51Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit f8784ac.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…ow decoding (apache#41037) ### Rationale for this change Previous patches ( apache#40876 , apache#40995 ) introduce some bugs in boolean decoding. This patch add tests for DecodeArrow and fix the bug ### What changes are included in this PR? 1. Add tests for DecodeArrow with nulls and without nulls 2. Fix 2 bugs ### Are these changes tested? Yes ### Are there any user-facing changes? no * GitHub Issue: apache#41032 Lead-authored-by: mwish <maplewish117@gmail.com> Co-authored-by: mwish <anmmscs_maple@qq.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

add testing arrow decoding with batch-size

54bdddc

mapleFU requested a review from wgtmac as a code owner April 5, 2024 16:14

mapleFU changed the title ~~GH-41032: [C++][Parquet] Bugfixes More tests in boolean arrow decoding~~ GH-41032: [C++][Parquet] Bugfixes and more tests in boolean arrow decoding Apr 5, 2024

github-actions bot added the awaiting review Awaiting review label Apr 5, 2024

mapleFU force-pushed the more-tests-in-boolean-arrow-decoding branch from 5aec277 to 6f7f796 Compare April 5, 2024 16:16

mapleFU added the Critical Fix Bugfixes for security vulnerabilities, crashes, or invalid data. label Apr 5, 2024

github-actions bot added Component: Parquet Component: C++ labels Apr 5, 2024

mapleFU force-pushed the more-tests-in-boolean-arrow-decoding branch from 6f7f796 to 4b811f2 Compare April 5, 2024 16:17

mapleFU commented Apr 5, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 5, 2024

mapleFU requested a review from pitrou April 5, 2024 16:20

bugfix for impl

825bb90

mapleFU force-pushed the more-tests-in-boolean-arrow-decoding branch from 4b811f2 to 825bb90 Compare April 5, 2024 16:21

change num_values_ to constant

a05dea2

mapleFU commented Apr 5, 2024

View reviewed changes

wgtmac reviewed Apr 7, 2024

View reviewed changes

rename variable name to page

9c7e01c

pitrou reviewed Apr 8, 2024

View reviewed changes

mapleFU added 2 commits April 8, 2024 21:22

Merge branch 'main' into more-tests-in-boolean-arrow-decoding

ea6fdb3

fix comment

f5cafc4

mapleFU force-pushed the more-tests-in-boolean-arrow-decoding branch from 6bfa3c4 to f5cafc4 Compare April 8, 2024 13:35

pitrou approved these changes Apr 8, 2024

View reviewed changes

pitrou merged commit f8784ac into apache:main Apr 8, 2024

pitrou removed the awaiting committer review Awaiting committer review label Apr 8, 2024

pitrou mentioned this pull request Apr 8, 2024

[C++] Regression in PlainBooleanDecoder::DecodeArrow #41032

Closed

mapleFU deleted the more-tests-in-boolean-arrow-decoding branch April 8, 2024 14:40

Conversation

mapleFU commented Apr 5, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

mapleFU commented Apr 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Apr 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Apr 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Apr 8, 2024

Uh oh!

conbench-apache-arrow bot commented Apr 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mapleFU commented Apr 5, 2024 •

edited by github-actions bot

Loading