fix(parquet): skip empty CDC page flushes in add_data_page - #11003
Open
M-Tesla wants to merge 1 commit into
Open
fix(parquet): skip empty CDC page flushes in add_data_page#11003M-Tesla wants to merge 1 commit into
M-Tesla wants to merge 1 commit into
Conversation
A forced content-defined chunk boundary can run after the chunk already flushed its page. Flushing with nothing buffered panics BOOLEAN RLE and emits zero-value pages for other encodings.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Content-defined chunking forces a data page break after every chunk except the last. Writing the chunk can already have flushed that page, when the chunk hits
data_page_size_limitordata_page_row_count_limitexactly at the boundary. The forcedadd_data_pagethen flushes with nothing buffered.For BOOLEAN under
PARQUET_2_0(RLE), that panics:RLE value encoder is not initialized. Other encodings do not panic, but they write a data page with zero values.should_add_data_page,dict_fallback, andflush_data_pagesalready skip the empty case.add_data_pagedid not.What changes are included in this PR?
GenericColumnWriter::add_data_pagereturnsOk(())whennum_buffered_values == 0.No public API change.
Are these changes tested?
data_page_size_limit(1024)and CDC 8KiB/16KiB: write succeeds, no empty data pages, roundtrip row count matchesdata_page_row_count_limit(128)and the same CDC options: no empty data pagescargo test -p parquet --lib -- cdc_boolean_small_pages_does_not_panicandcdc_int32_row_count_limit_does_not_emit_empty_pages.cargo clippy -p parquet --lib -- -D warnings.Are there any user-facing changes?
BOOLEAN CDC writes that previously panicked now succeed. Empty data pages are no longer emitted for the same forced-break case on other encodings.
AI Disclosure
Assisted draft of the empty-buffer guard and regression tests. The skip matches the existing guards on
should_add_data_page,dict_fallback, andflush_data_pages. Reviewed and verified with the checks above.