ShivanshhhG opened a new pull request, #54582:
URL: https://github.com/apache/arrow/pull/54582
### Rationale for this change
When writing a repeated column with a single record or a small
`data_page_size`, the Parquet writer could evaluate page limits when
`num_buffered_values_ == 0`. Because empty encoders (such as
`RleBooleanEncoder` and dictionary encoders) return a small non-zero estimated
size (e.g. 69 bytes or 6 bytes), `EstimatedDataEncodedSize() >=
data_pagesize()` evaluated to true and `AddDataPage()` was invoked, writing an
empty data page (`num_values = 0`, `num_rows = 0`) at the start of the column
chunk.
On read, `ColumnReaderImplBase::HasNextInternal()` checked `if
(!ReadNewPage() || num_buffered_values_ == 0)` and returned `false`,
erroneously treating the empty data page as end-of-column-chunk and silently
dropping all rows in that row group.
### What changes are included in this PR?
1. **Writer Fixes**:
- In `TypedColumnWriterImpl::CommitWriteAndCheckPageLimit`: required
`num_buffered_values_ > 0` before checking page limit conditions and triggering
`AddDataPage()`.
- In `ColumnWriterImpl::AddDataPage()`: added an early-return guard `if
(num_buffered_values_ == 0) return;` to prevent writing empty data pages.
2. **Reader Fix**:
- In `ColumnReaderImplBase::HasNextInternal()`: converted the page check
to a `while (num_buffered_values_ == 0 || num_decoded_values_ ==
num_buffered_values_)` loop so that any data page with zero values is skipped,
advancing to subsequent pages rather than prematurely terminating read. This
provides backward compatibility for existing Parquet files.
3. **Tests**:
- Added unit tests in `cpp/src/parquet/arrow/arrow_reader_writer_test.cc`:
- `SingleRowRepeatedSmallPageSize`: single-row repeated column with
small `data_page_size` and V2 data pages.
- `LastRowGroupSingleRowRepeatedSmallPageSize`: repeated columns across
multiple row groups where the last row group has 1 row.
- `ListDictionarySmallPageSize`: repeated columns with dictionary
encoding and small page size.
- `MultiColumnRepeatedSmallPageSize`: multi-column table with a
repeated column.
### Are these changes tested?
Yes, new unit tests covering repeated columns with small page sizes and
dictionary encoding have been added to `arrow_reader_writer_test.cc`.
### Are there any user-facing changes?
No breaking changes. This fixes a silent data loss bug where repeated
columns with small page sizes or single-row batches had rows dropped.
**This PR contains a "Critical Fix".**
Fixes a bug that caused rows in Parquet files with repeated columns to be
silently missing on read.
### Was AI used for this PR?
**PR code and description written by:**
- [x] AI
**Reviewed before submission by:**
- [x] Human
Closes #54533.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
