GH-36882: [C++][Parquet] Use RLE as BOOLEAN default encoding when both data page and version is V2 by mapleFU · Pull Request #38163 · apache/arrow

mapleFU · 2023-10-09T16:37:10Z

Rationale for this change

Only use RLE as BOOLEAN default encoding when data page is V2.

Previous patch ( #36955 ) set RLE encoding for Boolean type by default. However, parquet-cpp might write format v2 file with page v1 by default. This might cause parquet-cpp generating RLE encoding for boolean type by default. As https://issues.apache.org/jira/browse/PARQUET-2222 says, we still need some talks about that. So, we:

Still allow writing RLE on DataPage V2. This keeps same as parquet rust
If DataPage V1 is used, don't use RLE as default Boolean encoding.

What changes are included in this PR?

Only use RLE as BOOLEAN default encoding when both data page and version is V2.

Are these changes tested?

Yes

Are there any user-facing changes?

RLE encoding change for Boolean.

Closes: [C++][Parquet] Use RLE for boolean type by default when parquet version is 2.x #36882

wgtmac · 2023-10-10T01:23:37Z

    encoding = (descr->physical_type() == Type::BOOLEAN &&
-                properties->version() != ParquetVersion::PARQUET_1_0)
+                properties->version() != ParquetVersion::PARQUET_1_0 &&
+                properties->data_page_version() == ParquetDataPageVersion::V2)


IIUC, if the data page version is v2, it is user's responsibility to set parquet version to PARQUET_2_X. Unfortunately, this is not enforced yet.

Should we prevent from this or just keep the behavior here?

Let's do it separately.

wgtmac · 2023-10-10T01:24:32Z

@mapleFU There are CI failures.

mapleFU · 2023-10-10T11:04:36Z

@pitrou @jorisvandenbossche @wgtmac Would you mind take a look? Would this ok? Or just write RLE as default on V1 data page?

wgtmac

+1

pitrou · 2023-10-10T14:55:53Z

Can you update the PR description to explain the rationale for this change?

pitrou · 2023-10-10T14:57:24Z

+                        Encoding::type encoding) {
    this->SetUpSchema(Repetition::REQUIRED);
    auto writer = this->BuildWriter(SMALL_SIZE, ColumnProperties(), version,
+                                    ParquetDataPageVersion::V1,


Why not data_page_version?

pitrou · 2023-10-10T14:58:13Z

+  TestWithEncoding(ParquetVersion::PARQUET_2_4, ParquetDataPageVersion::V1,
+                   Encoding::PLAIN);
+  TestWithEncoding(ParquetVersion::PARQUET_2_4, ParquetDataPageVersion::V2,
+                   Encoding::RLE);


Well, RLE is always in the encodings regardless, since it's used to RL/DL, right?

mapleFU · 2023-10-10T15:19:21Z

I've resolve the comment and update the description @pitrou

pitrou

+1, thank you @mapleFU

conbench-apache-arrow · 2023-10-13T05:57:01Z

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

There were no benchmark performance regressions. 🎉

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

…en both data page and version is V2 (apache#38163) ### Rationale for this change Only use RLE as BOOLEAN default encoding when data page is V2. Previous patch ( apache#36955 ) set RLE encoding for Boolean type by default. However, parquet-cpp might write format v2 file with page v1 by default. This might cause parquet-cpp generating RLE encoding for boolean type by default. As https://issues.apache.org/jira/browse/PARQUET-2222 says, we still need some talks about that. So, we: 1. Still allow writing RLE on DataPage V2. This keeps same as parquet rust 2. If DataPage V1 is used, don't use RLE as default Boolean encoding. ### What changes are included in this PR? Only use RLE as BOOLEAN default encoding when both data page and version is V2. ### Are these changes tested? Yes ### Are there any user-facing changes? RLE encoding change for Boolean. * Closes: apache#36882 Lead-authored-by: mwish <maplewish117@gmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>

some cleanup works

e71518e

github-actions Bot added Component: Parquet Component: C++ labels Oct 9, 2023

mapleFU mentioned this pull request Oct 9, 2023

[C++][Parquet] Use RLE for boolean type by default when parquet version is 2.x #36882

Closed

github-actions Bot added the awaiting review Awaiting review label Oct 9, 2023

fix dict fallback logic

a061728

wgtmac reviewed Oct 10, 2023

View reviewed changes

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 10, 2023

fix compile

cb25923

wgtmac marked this pull request as ready for review October 10, 2023 02:21

mapleFU added 3 commits October 10, 2023 10:51

fix test

0918cd7

also do some metadata cleaning

94dca7d

revert python ut

d6ded08

github-actions Bot added the Component: Python label Oct 10, 2023

wgtmac approved these changes Oct 10, 2023

View reviewed changes

pitrou requested changes Oct 10, 2023

View reviewed changes

Merge branch 'main' into some-cleanup

b8d3592

mapleFU requested a review from pitrou October 10, 2023 15:18

fix comment

fb98583

mapleFU force-pushed the some-cleanup branch from d20ac55 to fb98583 Compare October 10, 2023 15:20

Nit: variable naming

9f4999a

pitrou approved these changes Oct 10, 2023

View reviewed changes

pitrou merged commit 64c7cae into apache:main Oct 10, 2023

pitrou removed the awaiting committer review Awaiting committer review label Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

GH-36882: [C++][Parquet] Use RLE as BOOLEAN default encoding when both data page and version is V2#38163

GH-36882: [C++][Parquet] Use RLE as BOOLEAN default encoding when both data page and version is V2#38163
pitrou merged 9 commits into
apache:mainfrom
mapleFU:some-cleanup

mapleFU commented Oct 9, 2023 •

edited by pitrou

Loading

Uh oh!

wgtmac Oct 10, 2023

Uh oh!

mapleFU Oct 10, 2023

Uh oh!

wgtmac Oct 10, 2023

Uh oh!

wgtmac commented Oct 10, 2023

Uh oh!

mapleFU commented Oct 10, 2023

Uh oh!

wgtmac left a comment

Uh oh!

pitrou commented Oct 10, 2023

Uh oh!

pitrou Oct 10, 2023

Uh oh!

pitrou Oct 10, 2023

Uh oh!

mapleFU commented Oct 10, 2023

Uh oh!

pitrou left a comment

Uh oh!

conbench-apache-arrow Bot commented Oct 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

mapleFU commented Oct 9, 2023 • edited by pitrou 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!

wgtmac Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

mapleFU Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

wgtmac Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Oct 10, 2023

Uh oh!

mapleFU commented Oct 10, 2023

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Oct 10, 2023

Uh oh!

pitrou Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Oct 10, 2023

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

conbench-apache-arrow Bot commented Oct 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mapleFU commented Oct 9, 2023 •

edited by pitrou

Loading