GH-36939: [C++][Parquet] Direct put of BooleanArray is incorrect when called several times #36972

mapleFU · 2023-08-01T12:25:06Z

Rationale for this change

This is from a bug in PLAIN encoding with BooleanArray input. Boolean will introduce bad length when writing arrow data.

This interface is not widely used.

What changes are included in this PR?

Rewrite PLAIN boolean encoder to use TypedBufferBuilder instead of an incorrect hand-baked implementation.

Are these changes tested?

Yes

Are there any user-facing changes?

No.

Closes: [C++][Parquet] Boolean encoding has inconsistent implemention #36939

mapleFU · 2023-08-01T12:25:23Z

@pitrou @wgtmac PTAL

github-actions · 2023-08-01T12:25:35Z

⚠️ GitHub issue #36939 has been automatically assigned in GitHub to PR creator.

cpp/src/parquet/encoding.cc

cpp/src/parquet/encoding_test.cc

wgtmac · 2023-08-04T05:20:36Z

@pitrou @emkornfield Do you want to take a look?

cpp/src/parquet/encoding_test.cc

emkornfield · 2023-08-06T05:36:04Z

cpp/src/parquet/encoding.cc

@@ -365,7 +369,7 @@ class PlainEncoder<BooleanType> : public EncoderImpl, virtual public BooleanEnco
 }
 writer.Finish();
 }
- sink_.UnsafeAdvance(data.length());
+ sink_.UnsafeAdvance(data.length() - data.null_count());


shouldn't this be n_valid?

@emkornfield Parquet encoding stores "valid" value. The invalid value will be marked in rep-levels and def-levels

I was referring to the entire value passed into sink_.UnsafeAdvance

This seems like it was incorrect even for all present values because we are advancing the Byte buffer by number of values (in this case these would be number of bits) and not number bytes. So we would be overadvancing in both cases?

(i.e. we seem to be advancing by more bytes then are being reserved in both cases)

Yes, the logic here is confusing, but it's right after my change. When input k boolean values.

sink_.Reserve will only reserve bytes for bits (k).

sink_.UnsafeAdvance will advance k bytes.

However, when used, sink_.length() will only be regarded as bits. So (2) has a bug, but it works here...

I'd like to fix the bug first, and take time to optimize the code later.

In that case we need to be reserving k bytes and not k/8. I think this wasn't caught sooner because columnwriter appears to have a specialization for Boolean values that bypasses this method (e.g. I don't think anything but encoder tests will fail if you put throw exception(....) in this method in general.

emkornfield · 2023-08-06T06:16:48Z

General question, is this code path actually used in practice? (should we just delete the code)?

mapleFU · 2023-08-06T06:19:53Z

General question, is this code path actually used in practice? (should we just delete the code)?

We use parquet boolean type to store bool value. And this is exposed in python for a long time. So I think: 1. Boolean type is not widely used, otherwise it would be found earier 2. However, I think there're someone using it. So I think we'd better fix this

emkornfield · 2023-08-06T06:20:23Z

cpp/src/parquet/encoding.cc

@@ -354,6 +357,7 @@ class PlainEncoder<BooleanType> : public EncoderImpl, virtual public BooleanEnco
 sink_.length(), n_valid);


this line also seems problematic if this method is called multiple times with in a row with boolean arrays (not sure actual code does this though).

#36972 (comment)
You're right. This would not produce bug if PutArrow is not mixed with PutImpl, but will make Boolean leaves a larger space than expected

You mentioned this below, using sink_.length() to store the actual values seems wrong.

emkornfield · 2023-08-06T06:34:31Z

We use parquet boolean type to store bool value. And this is exposed in python for a long time. So I think:

Yes, I think we got lucky here because I think the python/c++ column writer code calls PutSpaced

mapleFU · 2023-08-06T06:41:55Z

Python/c++ column writer code calls PutSpaced

Thats lucky! Maybe I'll just fix like this and add some comments

conbench-apache-arrow · 2023-08-09T14:56:56Z

Thanks for your patience. Conbench analyzed the 3 benchmarking runs that have been run so far on PR commit 1a45056.

None of the specified runs had any associated benchmark results.

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

austin3dickey · 2023-08-09T15:01:05Z

^ Sorry, this is a mistake. There were C++ benchmark results, but they were suppressed because we typically only report on Python and R benchmark results. You can see the results by clicking through to the report. I'll fix this bug soon.

pitrou · 2023-08-09T15:50:07Z

I'd rather not suffer a potentially significant performance regression here. Ideally, writing a boolean Arrow array would go through this function (I'm not sure why it doesn't).

mapleFU · 2023-08-09T16:22:59Z

I'd rather not suffer a potentially significant performance regression here. Ideally, writing a boolean Arrow array would go through this function (I'm not sure why it doesn't).

Sure I'll rewrite it :-)

But I wonder that how can we optimize PutSpaced. Should we just regard it as before?

pitrou · 2023-08-09T17:17:29Z

But I wonder that how can we optimize PutSpaced. Should we just regard it as before?

For now, yes, but it could perhaps be improved if it's often used.

mapleFU · 2023-08-10T03:28:45Z

I've changed to bit_writer_.PutValue try to make non-spaced Put faster. Though it would still slower than CopyBitmap, but I think it's right...

cpp/src/parquet/encoding.cc

mapleFU · 2023-08-10T06:09:22Z

I've add an valid_bit_length_ to replace sink.size(). @wgtmac @pitrou @emkornfield

pitrou · 2023-08-10T09:40:05Z

Again, why not use TypedBufferBuilder<bool>?

mapleFU · 2023-08-10T09:43:02Z

Again, why not use TypedBufferBuilder<bool>?

Because PutImpl already uses sink. And I think TypedBufferBuilder<bool> is as slow as current implemention.

pitrou · 2023-08-10T10:16:48Z

TypedBufferBuilder<bool> uses CopyBitmap internally, so it should be faster. It should also help automate some of the bookkeeping you're doing by hand.

arrow/cpp/src/arrow/buffer_builder.h

Lines 377 to 383 in f93929f

 /// \brief Append bits from a packed bitmap 

 void UnsafeAppend(const uint8_t* bitmap, int64_t offset, int64_t num_elements) { 

 if (num_elements == 0) return; 

 internal::CopyBitmap(bitmap, offset, num_elements, mutable_data(), bit_length_); 

 false_count_ += num_elements - internal::CountSetBits(bitmap, offset, num_elements); 

 bit_length_ += num_elements; 

 }

mapleFU · 2023-08-10T10:55:11Z

Okay, I've refactor all into a TypedBufferBuilder @pitrou

I change PutImpl to use TypedBufferBuilder. I guess we can optimize it later.

pitrou

This is a nice simplification :-)

cpp/src/parquet/encoding.cc

cpp/src/parquet/encoding_test.cc

pitrou · 2023-08-10T16:33:27Z

@github-actions crossbow submit -g nightly-tests

github-actions · 2023-08-10T16:39:15Z

500 [No message]
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/5823722160

pitrou · 2023-08-10T16:47:23Z

@github-actions crossbow submit -g nightly-tests

github-actions · 2023-08-10T16:48:36Z

Revision: 8c23944

Submitted crossbow builds: ursacomputing/crossbow @ actions-a101b56646

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
test-alpine-linux-cpp
test-build-cpp-fuzz
test-build-vcpkg-win
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.4.1
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.4.1
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-cpp
test-cuda-python
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-go-1.17
test-debian-11-go-1.20
test-debian-11-python-3
test-debian-c-glib
test-debian-ruby
test-fedora-35-cpp
test-fedora-35-python-3
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-library-r-base-latest
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-skyhook-integration
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-20.04-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-docs
test-ubuntu-22.04-python-3
test-ubuntu-c-glib
test-ubuntu-r-sanitizer
test-ubuntu-ruby

conbench-apache-arrow · 2023-08-14T02:15:30Z

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

There were 2 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-08-13 23:40:22Z
- tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-06, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=1

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

…t when called several times (apache#36972) ### Rationale for this change This is from a bug in PLAIN encoding with `BooleanArray` input. Boolean will introduce bad length when writing arrow data. This interface is not widely used. ### What changes are included in this PR? Rewrite PLAIN boolean encoder to use `TypedBufferBuilder` instead of an incorrect hand-baked implementation. ### Are these changes tested? Yes ### Are there any user-facing changes? No. * Closes: apache#36939 Lead-authored-by: mwish <maplewish117@gmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>

mapleFU requested a review from wgtmac as a code owner August 1, 2023 12:25

mapleFU requested a review from pitrou August 1, 2023 12:25

github-actions bot added Component: Parquet Component: C++ awaiting review Awaiting review labels Aug 1, 2023

Parquet: fix and test inconsistent Boolean encoding

c0cd7e0

mapleFU force-pushed the parquet/check-inconsistent-boolean branch from 587e2e0 to c0cd7e0 Compare August 1, 2023 13:00

fix compiling

961e255

wgtmac requested changes Aug 1, 2023

View reviewed changes

cpp/src/parquet/encoding.cc Outdated Show resolved Hide resolved

cpp/src/parquet/encoding_test.cc Outdated Show resolved Hide resolved

cpp/src/parquet/encoding_test.cc Outdated Show resolved Hide resolved

cpp/src/parquet/encoding_test.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 1, 2023

wgtmac reviewed Aug 2, 2023

View reviewed changes

cpp/src/parquet/encoding_test.cc Outdated Show resolved Hide resolved

wgtmac approved these changes Aug 2, 2023

View reviewed changes

[Update] Fix comment

1765718

mapleFU force-pushed the parquet/check-inconsistent-boolean branch from 85749de to 1765718 Compare August 2, 2023 03:41

emkornfield reviewed Aug 6, 2023

View reviewed changes

cpp/src/parquet/encoding_test.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Aug 6, 2023

emkornfield reviewed Aug 6, 2023

View reviewed changes

mapleFU added 2 commits August 6, 2023 14:42

Merge branch 'main' into parquet/check-inconsistent-boolean

375c094

[Update] resolve comment

45bbef1

github-actions bot removed the awaiting changes Awaiting changes label Aug 6, 2023

tiny reflect logic

5c3a23e

wgtmac reviewed Aug 10, 2023

View reviewed changes

cpp/src/parquet/encoding.cc Show resolved Hide resolved

add a valid_bit_length_ for bug fixing

5e7d841

change valid_bit_length to i64

ccdedb4

refactor bit write using TypedBufferBuilder

68f28e8

pitrou requested changes Aug 10, 2023

View reviewed changes

mapleFU and others added 2 commits August 11, 2023 00:13

resolve comment and bug fix

06dac34

Improve tests to also exercise slicing, and arrays without nulls

8c23944

pitrou approved these changes Aug 10, 2023

View reviewed changes

pitrou changed the title ~~GH-36939: [C++][Parquet] Boolean encoding has inconsistent implemention~~ GH-36939: [C++][Parquet] Direct put of BooleanArray is incorrect when called several times Aug 10, 2023

pitrou merged commit be1b003 into apache:main Aug 10, 2023
30 of 32 checks passed

pitrou removed the awaiting change review Awaiting change review label Aug 10, 2023

mapleFU deleted the parquet/check-inconsistent-boolean branch August 11, 2023 02:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-36939: [C++][Parquet] Direct put of BooleanArray is incorrect when called several times #36972

GH-36939: [C++][Parquet] Direct put of BooleanArray is incorrect when called several times #36972

mapleFU commented Aug 1, 2023 •

edited by pitrou

Loading

mapleFU commented Aug 1, 2023

github-actions bot commented Aug 1, 2023

wgtmac commented Aug 4, 2023

emkornfield Aug 6, 2023

mapleFU Aug 6, 2023

emkornfield Aug 6, 2023 •

edited

Loading

emkornfield Aug 6, 2023

mapleFU Aug 6, 2023

emkornfield Aug 6, 2023 •

edited

Loading

emkornfield commented Aug 6, 2023

mapleFU commented Aug 6, 2023 •

edited

Loading

emkornfield Aug 6, 2023

mapleFU Aug 6, 2023

emkornfield Aug 6, 2023

emkornfield commented Aug 6, 2023 •

edited

Loading

mapleFU commented Aug 6, 2023 •

edited

Loading

conbench-apache-arrow bot commented Aug 9, 2023

austin3dickey commented Aug 9, 2023 •

edited

Loading

pitrou commented Aug 9, 2023

mapleFU commented Aug 9, 2023

pitrou commented Aug 9, 2023

mapleFU commented Aug 10, 2023

mapleFU commented Aug 10, 2023

pitrou commented Aug 10, 2023

mapleFU commented Aug 10, 2023

pitrou commented Aug 10, 2023 •

edited

Loading

mapleFU commented Aug 10, 2023 •

edited

Loading

pitrou left a comment

pitrou commented Aug 10, 2023

github-actions bot commented Aug 10, 2023

pitrou commented Aug 10, 2023

github-actions bot commented Aug 10, 2023

conbench-apache-arrow bot commented Aug 14, 2023

		@@ -354,6 +357,7 @@ class PlainEncoder<BooleanType> : public EncoderImpl, virtual public BooleanEnco
		sink_.length(), n_valid);

GH-36939: [C++][Parquet] Direct put of BooleanArray is incorrect when called several times #36972

GH-36939: [C++][Parquet] Direct put of BooleanArray is incorrect when called several times #36972

Conversation

mapleFU commented Aug 1, 2023 • edited by pitrou Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

mapleFU commented Aug 1, 2023

github-actions bot commented Aug 1, 2023

wgtmac commented Aug 4, 2023

emkornfield Aug 6, 2023

Choose a reason for hiding this comment

mapleFU Aug 6, 2023

Choose a reason for hiding this comment

emkornfield Aug 6, 2023 • edited Loading

Choose a reason for hiding this comment

emkornfield Aug 6, 2023

Choose a reason for hiding this comment

mapleFU Aug 6, 2023

Choose a reason for hiding this comment

emkornfield Aug 6, 2023 • edited Loading

Choose a reason for hiding this comment

emkornfield commented Aug 6, 2023

mapleFU commented Aug 6, 2023 • edited Loading

emkornfield Aug 6, 2023

Choose a reason for hiding this comment

mapleFU Aug 6, 2023

Choose a reason for hiding this comment

emkornfield Aug 6, 2023

Choose a reason for hiding this comment

emkornfield commented Aug 6, 2023 • edited Loading

mapleFU commented Aug 6, 2023 • edited Loading

conbench-apache-arrow bot commented Aug 9, 2023

austin3dickey commented Aug 9, 2023 • edited Loading

pitrou commented Aug 9, 2023

mapleFU commented Aug 9, 2023

pitrou commented Aug 9, 2023

mapleFU commented Aug 10, 2023

mapleFU commented Aug 10, 2023

pitrou commented Aug 10, 2023

mapleFU commented Aug 10, 2023

pitrou commented Aug 10, 2023 • edited Loading

mapleFU commented Aug 10, 2023 • edited Loading

pitrou left a comment

Choose a reason for hiding this comment

pitrou commented Aug 10, 2023

github-actions bot commented Aug 10, 2023

pitrou commented Aug 10, 2023

github-actions bot commented Aug 10, 2023

conbench-apache-arrow bot commented Aug 14, 2023

mapleFU commented Aug 1, 2023 •

edited by pitrou

Loading

emkornfield Aug 6, 2023 •

edited

Loading

emkornfield Aug 6, 2023 •

edited

Loading

mapleFU commented Aug 6, 2023 •

edited

Loading

emkornfield commented Aug 6, 2023 •

edited

Loading

mapleFU commented Aug 6, 2023 •

edited

Loading

austin3dickey commented Aug 9, 2023 •

edited

Loading

pitrou commented Aug 10, 2023 •

edited

Loading

mapleFU commented Aug 10, 2023 •

edited

Loading