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-36882: [C++][Parquet] Use RLE as BOOLEAN default encoding when both data page and version is V2 #38163

Merged
merged 9 commits into from
Oct 10, 2023
Merged

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

merged 9 commits into from
Oct 10, 2023

Conversation

mapleFU
Copy link
Member

@mapleFU mapleFU commented Oct 9, 2023 •
edited by pitrou
Loading

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:

  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.

@mapleFU mapleFU mentioned this pull request Oct 9, 2023
Closed
@github-actions github-actions bot added the awaiting review Awaiting review label Oct 9, 2023
wgtmac
wgtmac reviewed Oct 10, 2023
View reviewed changes
cpp/src/parquet/column_writer.cc
@@ -2336,7 +2336,8 @@ std::shared_ptr<ColumnWriter> ColumnWriter::Make(ColumnChunkMetaDataBuilder* met
Encoding::type encoding = properties->encoding(descr->path());
if (encoding == Encoding::UNKNOWN) {
encoding = (descr->physical_type() == Type::BOOLEAN &&
properties->version() != ParquetVersion::PARQUET_1_0)
properties->version() != ParquetVersion::PARQUET_1_0 &&
properties->data_page_version() == ParquetDataPageVersion::V2)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Let's do it separately.

@wgtmac
Copy link
Member

wgtmac commented Oct 10, 2023

@mapleFU There are CI failures.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 10, 2023
@wgtmac wgtmac marked this pull request as ready for review October 10, 2023 02:21
@mapleFU
Copy link
Member Author

mapleFU commented Oct 10, 2023

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

wgtmac
wgtmac approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

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

+1

@pitrou
Copy link
Member

pitrou commented Oct 10, 2023

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

pitrou
pitrou requested changes Oct 10, 2023
View reviewed changes
cpp/src/parquet/column_writer_test.cc Outdated
this->SetUpSchema(Repetition::REQUIRED);
auto writer = this->BuildWriter(SMALL_SIZE, ColumnProperties(), version,
ParquetDataPageVersion::V1,
Copy link
Member

Choose a reason for hiding this comment

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

Why not data_page_version?

cpp/src/parquet/column_writer_test.cc Outdated
TestWithEncoding(ParquetVersion::PARQUET_2_4, ParquetDataPageVersion::V1,
Encoding::PLAIN);
TestWithEncoding(ParquetVersion::PARQUET_2_4, ParquetDataPageVersion::V2,
Encoding::RLE);
Copy link
Member

Choose a reason for hiding this comment

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

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

@mapleFU mapleFU requested a review from pitrou October 10, 2023 15:18
@mapleFU
Copy link
Member Author

mapleFU commented Oct 10, 2023

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

pitrou
pitrou approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1, thank you @mapleFU

@pitrou pitrou merged commit 64c7cae into apache:main Oct 10, 2023
32 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Oct 10, 2023
@conbench-apache-arrow
Copy link

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.

JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 23, 2023
@mapleFU @pitrou
apacheGH-36882: [C++][Parquet] Use RLE as BOOLEAN default encoding wh…
21ed8a3 
…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 <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
@mapleFU @pitrou @loicalleyne
apacheGH-36882: [C++][Parquet] Use RLE as BOOLEAN default encoding wh…
d87a7a8 
…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 <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
@mapleFU @pitrou @dgreiss
apacheGH-36882: [C++][Parquet] Use RLE as BOOLEAN default encoding wh…
ec104ef 
…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 <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wgtmac wgtmac wgtmac approved these changes

@pitrou pitrou pitrou approved these changes

Assignees
No one assigned
Labels
Component: C++ Component: Parquet Component: Python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C++][Parquet] Use RLE for boolean type by default when parquet version is 2.x
3 participants
@mapleFU @wgtmac @pitrou