Skip to content

GH-31992: [C++][Parquet] Handling the special case when DataPageV2 values buffer is empty - #45252

Merged
mapleFU merged 5 commits into
apache:mainfrom
mapleFU:GH-31992
Jan 20, 2025
Merged

GH-31992: [C++][Parquet] Handling the special case when DataPageV2 values buffer is empty#45252
mapleFU merged 5 commits into
apache:mainfrom
mapleFU:GH-31992

Conversation

@mapleFU

@mapleFUmapleFU commented Jan 14, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Rationale for this change

In DataPageV2, the levels and data will not be compressed together. So, we might get the "empty" data page buffer.

When meeting this, Snappy C++ will failed to decompress the (input_len == 0, output_len == 0) data.

What changes are included in this PR?

Handling the case in column_reader.cc

Are these changes tested?

  • Will add

Are there any user-facing changes?

Minor fix

@mapleFU
mapleFU requested a review from wgtmac as a code ownerJanuary 14, 2025 08:41
@github-actions

github-actionsBot commented Jan 14, 2025

Copy link
Copy Markdown

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

wgtmac
wgtmac reviewed Jan 14, 2025
View reviewed changes
Comment threadcpp/src/parquet/column_reader.cc
@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 14, 2025
mapleFU
mapleFU commented Jan 14, 2025
View reviewed changes
Comment threadcpp/src/parquet/column_writer_test.cc Outdated

ASSERT_EQ(100, this->metadata_num_values());
this->ReadColumn(Compression::SNAPPY);
ASSERT_EQ(0, this->values_read_);

@mapleFUmapleFUJan 14, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I've verified that the unittest test would test this case

@mapleFU

mapleFU commented Jan 14, 2025

Copy link
Copy Markdown
MemberAuthor

@pitrou@wgtmac

pitrou
pitrou reviewed Jan 14, 2025
View reviewed changes
Comment threadcpp/src/parquet/column_reader.cc Outdated
Comment threadcpp/src/parquet/column_writer_test.cc Outdated
@mapleFU

mapleFU commented Jan 15, 2025

Copy link
Copy Markdown
MemberAuthor

@pitrou so what should I need to do to merge this? Should I check #45252 (comment) in compression to check that "Compress" doesn't compress the zero-sized page? Or I should change other code?

pitrou
pitrou reviewed Jan 15, 2025
View reviewed changes
Comment threadcpp/src/parquet/column_writer_test.cc Outdated
pitrou
pitrou reviewed Jan 15, 2025
View reviewed changes
Comment threadcpp/src/parquet/column_writer_test.cc
@pitrou

pitrou commented Jan 15, 2025

Copy link
Copy Markdown
Member

@mapleFU Can we open a bug for Parquet-java and reference it here?

@mapleFUmapleFU mentioned this pull request Jan 15, 2025
Closed
@mapleFU

mapleFU commented Jan 15, 2025

Copy link
Copy Markdown
MemberAuthor

Created: apache/parquet-java#3122

@mapleFU
mapleFU requested a review from pitrouJanuary 20, 2025 02:34
pitrou
pitrou reviewed Jan 20, 2025
View reviewed changes
Comment threadcpp/src/parquet/column_reader.cc Outdated
pitrou
pitrou approved these changes Jan 20, 2025
View reviewed changes

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, just a comment update

@mapleFU@pitrou
Update cpp/src/parquet/column_reader.cc
0f11608
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
@mapleFU
mapleFU merged commit 9801801 into apache:mainJan 20, 2025
@mapleFUmapleFU removed the awaiting committer review Awaiting committer review label Jan 20, 2025
@mapleFUmapleFU mentioned this pull request Jan 20, 2025
Closed
@mapleFU
mapleFU deleted the GH-31992 branch January 20, 2025 11:02
@mapleFU

mapleFU commented Jan 20, 2025

Copy link
Copy Markdown
MemberAuthor

Failed CI unrelated, merged

@conbench-apache-arrow

conbench-apache-arrowBot commented Jan 20, 2025

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

@mapleFUmapleFU mentioned this pull request Mar 4, 2025
Merged
@EnricoMiEnricoMi mentioned this pull request Apr 6, 2025
Closed
@EnricoMi

EnricoMi commented Apr 7, 2025

Copy link
Copy Markdown
Collaborator

Fixes #22459.

This was referenced Apr 7, 2025
Closed
Merged
Closed
Merged
Closed
Merged
Mytherin added a commit to duckdb/duckdb that referenced this pull request Apr 9, 2025
@Mytherin
Handle Parquet with compressed empty DataPage v2 (#17031)
6c176c0
The issue is identical to this Apache Arrow issue:
apache/arrow#22459
The fix is identical to Apache Arrow fix:
apache/arrow#45252Fixes#17029.
adsharma pushed a commit to Pygmy-Goose/data that referenced this pull request Mar 12, 2026
@Mytherin
Handle Parquet with compressed empty DataPage v2 (#17031)
99433b5
The issue is identical to this Apache Arrow issue:
apache/arrow#22459
The fix is identical to Apache Arrow fix:
apache/arrow#45252
Fixes #17029.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wgtmacwgtmacwgtmac left review comments
@pitroupitroupitrou approved these changes

Assignees

No one assigned

Labels

Component: C++Component: Parquet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mapleFU@pitrou@EnricoMi@wgtmac