Repository navigation
GH-54533: [C++][Parquet] Avoid emitting empty data pages and advance past empty pages in reader - #54582
GH-54533: [C++][Parquet] Avoid emitting empty data pages and advance past empty pages in reader#54582ShivanshhhG wants to merge 1 commit into
Conversation
|
|
|
|
github-actions
Bot
added
Component: Parquet
Component: C++
awaiting review
…vance past empty pages in reader When writing a repeated column with a single record batch or a small data_page_size, the writer could check page limits when num_buffered_values_ is zero. Because empty encoders (such as RleBooleanEncoder or DictionaryEncoder) estimate a small non-zero encoded size, the writer previously emitted an empty first data page (num_values = 0). On read, HasNextInternal returned false upon encountering the empty page, prematurely truncating the column chunk and silently dropping the row group's rows. This commit: 1. In CommitWriteAndCheckPageLimit, ensures num_buffered_values_ > 0 before triggering AddDataPage(). 2. In ColumnWriterImpl::AddDataPage(), guards against adding empty pages when num_buffered_values_ == 0. 3. In ColumnReaderImplBase::HasNextInternal(), loops across any data page with zero buffered values rather than treating it as end of column chunk, ensuring robust backward compatibility with existing Parquet files. 4. Adds unit tests in arrow_reader_writer_test.cc verifying repeated columns with small page sizes, V2 pages, dictionary encoding, and multi-column schemas. Closes: apache#54533 Signed-off-by: SHIVANSH-ux-ys <singaser78@gmail.com>
262bda4 to
ffb6e89
Compare
|
This looks good to me. @wgtmac @adamreeve @HuaHuaY Do you think we should add a file with an empty data page in parquet-testing? @ShivanshhhG The PR description looks AI-generated and is quite verbose, can you trim it down for human consumption? |
|
Thanks for the review @pitrou! I have trimmed down the PR description to be concise and straight to the point. |
|
|
adamreeve
left a comment
adamreeve
left a comment
There was a problem hiding this comment.
This change also looks correct to me, thanks for the fix @ShivanshhhG.
This looks good to me. @wgtmac @adamreeve @HuaHuaY Do you think we should add a file with an empty data page in parquet-testing?
Yes, as far as I can tell, an empty page is valid according to the spec. Although not when an offset index is used and the empty page isn't the last one. While it makes sense to avoid writing an empty page, it's a good edge case to include in parquet-testing. Maybe there should be a bad_data example with the offset index included, and a valid file without it?
Rationale for this change
When writing repeated columns with single records or small
data_page_size, empty encoders (such asRleBooleanEncoder) return small non-zero estimated sizes, causingAddDataPage()to emit an empty data page (num_values = 0). On read,ColumnReaderImplBase::HasNextInternal()returnedfalseon empty data pages, prematurely terminating reading and dropping subsequent rows in the row group.What changes are included in this PR?
CommitWriteAndCheckPageLimitandAddDataPageto requirenum_buffered_values_ > 0before writing data pages.ColumnReaderImplBase::HasNextInternal, loop past empty data pages (num_buffered_values_ == 0) to maintain backward compatibility with existing files.arrow_reader_writer_test.ccfor repeated columns with small page sizes and dictionary encoding.Are these changes tested?
Yes, unit tests added to
arrow_reader_writer_test.cc.Are there any user-facing changes?
No breaking changes. Fixes potential data omission when reading files with empty data pages.
Closes #54533.