From 262bda4458cccb31311e7983d6b7cbb51fdecbeb Mon Sep 17 00:00:00 2001 From: SHIVANSH-ux-ys Date: Mon, 19 Oct 2026 14:55:29 +0530 Subject: [PATCH] GH-54533: [C++][Parquet] Avoid emitting empty data pages and advance 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: #54533 Signed-off-by: SHIVANSH-ux-ys --- .../parquet/arrow/arrow_reader_writer_test.cc | 67 +++++++++++++++++++ cpp/src/parquet/column_reader.cc | 4 +- cpp/src/parquet/column_writer.cc | 5 +- 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/cpp/src/parquet/arrow/arrow_reader_writer_test.cc b/cpp/src/parquet/arrow/arrow_reader_writer_test.cc index d858962d4728..428b61e1f8dd 100644 --- a/cpp/src/parquet/arrow/arrow_reader_writer_test.cc +++ b/cpp/src/parquet/arrow/arrow_reader_writer_test.cc @@ -3144,6 +3144,73 @@ TEST(TestArrowReadWrite, ManySmallLists) { default_arrow_writer_properties()); } +TEST(TestArrowReadWrite, SingleRowRepeatedSmallPageSize) { + // GH-54533: When writing a repeated column with a single row / small page size, + // the writer must not emit an empty leading data page, and the reader must + // handle data pages without prematurely terminating the column chunk. + auto list_type = ::arrow::list(::arrow::boolean()); + auto array = ArrayFromJSON(list_type, "[[true]]"); + auto field = ::arrow::field("c0", list_type); + auto schema = ::arrow::schema({field}); + auto table = Table::Make(schema, {std::make_shared(array)}); + + auto writer_props = WriterProperties::Builder() + .data_pagesize(64) + ->data_page_version(ParquetDataPageVersion::V2) + ->build(); + CheckConfiguredRoundtrip(table, table, writer_props); +} + +TEST(TestArrowReadWrite, LastRowGroupSingleRowRepeatedSmallPageSize) { + // GH-54533: Last row group holding a single repeated record. + auto list_type = ::arrow::list(::arrow::boolean()); + auto array = ArrayFromJSON(list_type, "[[true], [false], [true]]"); + auto field = ::arrow::field("c0", list_type); + auto schema = ::arrow::schema({field}); + auto table = Table::Make(schema, {std::make_shared(array)}); + + auto writer_props = WriterProperties::Builder() + .data_pagesize(64) + ->data_page_version(ParquetDataPageVersion::V2) + ->build(); + std::shared_ptr actual_table; + DoRoundtrip(table, /*row_group_size=*/2, &actual_table, writer_props); + ::arrow::AssertTablesEqual(*table, *actual_table); +} + +TEST(TestArrowReadWrite, ListDictionarySmallPageSize) { + // GH-54533: Dictionary encoding with very small data page size. + auto list_type = ::arrow::list(::arrow::int32()); + auto array = ArrayFromJSON(list_type, "[[1], [2], [3]]"); + auto field = ::arrow::field("c0", list_type); + auto schema = ::arrow::schema({field}); + auto table = Table::Make(schema, {std::make_shared(array)}); + + auto writer_props = WriterProperties::Builder().data_pagesize(4)->build(); + std::shared_ptr
actual_table; + DoRoundtrip(table, /*row_group_size=*/2, &actual_table, writer_props); + ::arrow::AssertTablesEqual(*table, *actual_table); +} + +TEST(TestArrowReadWrite, MultiColumnRepeatedSmallPageSize) { + // GH-54533: Two columns where one is repeated and page size is small. + auto list_type = ::arrow::list(::arrow::boolean()); + auto f0 = ::arrow::field("id", ::arrow::int64()); + auto f1 = ::arrow::field("c0", list_type); + auto schema = ::arrow::schema({f0, f1}); + + auto a0 = ArrayFromJSON(::arrow::int64(), "[1]"); + auto a1 = ArrayFromJSON(list_type, "[[true]]"); + auto table = Table::Make(schema, {std::make_shared(a0), + std::make_shared(a1)}); + + auto writer_props = WriterProperties::Builder() + .data_pagesize(64) + ->data_page_version(ParquetDataPageVersion::V2) + ->build(); + CheckConfiguredRoundtrip(table, table, writer_props); +} + TEST(TestArrowReadWrite, TableWithDuplicateColumns) { // See ARROW-1974 using ::arrow::ArrayFromVector; diff --git a/cpp/src/parquet/column_reader.cc b/cpp/src/parquet/column_reader.cc index ee2e30fb62c5..d9abb78df609 100644 --- a/cpp/src/parquet/column_reader.cc +++ b/cpp/src/parquet/column_reader.cc @@ -776,8 +776,8 @@ class ColumnReaderImplBase { bool HasNextInternal() { // Either there is no data page available yet, or the data page has been // exhausted - if (num_buffered_values_ == 0 || num_decoded_values_ == num_buffered_values_) { - if (!ReadNewPage() || num_buffered_values_ == 0) { + while (num_buffered_values_ == 0 || num_decoded_values_ == num_buffered_values_) { + if (!ReadNewPage()) { return false; } } diff --git a/cpp/src/parquet/column_writer.cc b/cpp/src/parquet/column_writer.cc index 3b4a873b5192..7f58fff595af 100644 --- a/cpp/src/parquet/column_writer.cc +++ b/cpp/src/parquet/column_writer.cc @@ -963,6 +963,9 @@ int64_t ColumnWriterImpl::RleEncodeLevels(const void* src_buffer, } void ColumnWriterImpl::AddDataPage() { + if (num_buffered_values_ == 0) { + return; + } int64_t definition_levels_rle_size = 0; int64_t repetition_levels_rle_size = 0; @@ -1838,7 +1841,7 @@ class TypedColumnWriterImpl : public ColumnWriterImpl, num_buffered_encoded_values_ += num_values; num_buffered_nulls_ += num_nulls; - if (check_page_limit && + if (check_page_limit && num_buffered_values_ > 0 && (current_encoder_->EstimatedDataEncodedSize() >= properties_->data_pagesize() || num_buffered_rows_ >= properties_->max_rows_per_page())) { AddDataPage();