From f029ac38c987f6afe722bdcb4c49239a4e090df4 Mon Sep 17 00:00:00 2001 From: Krisztian Szucs Date: Fri, 30 Oct 2026 19:16:50 +0100 Subject: [PATCH 1/2] [C++][Parquet] Test reading a file with data pages without values Update parquet-testing with empty_data_pages.parquet from apache/parquet-testing#131. The test fails until the next commit. --- cpp/src/parquet/arrow/arrow_reader_writer_test.cc | 15 +++++++++++++++ cpp/submodules/parquet-testing | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/cpp/src/parquet/arrow/arrow_reader_writer_test.cc b/cpp/src/parquet/arrow/arrow_reader_writer_test.cc index 421769521d1e..e70026f91a9f 100644 --- a/cpp/src/parquet/arrow/arrow_reader_writer_test.cc +++ b/cpp/src/parquet/arrow/arrow_reader_writer_test.cc @@ -3085,6 +3085,21 @@ std::shared_ptr InvalidTable() { return Table::Make(schema, {array1, array2}); } +TEST(TestArrowReadWrite, ReadEmptyDataPages) { + // A data page without values doesn't end the column chunk, the column chunk of the file + // starts with such a page and has another one in the middle + ASSERT_OK_AND_ASSIGN( + auto reader, + FileReader::Make(::arrow::default_memory_pool(), + ParquetFileReader::OpenFile( + test::get_data_file("empty_data_pages.parquet"), false))); + ASSERT_OK_AND_ASSIGN(auto table, reader->ReadTable()); + auto expected = ::arrow::TableFromJSON( + ::arrow::schema({::arrow::field("int64", ::arrow::int64(), /*nullable=*/false)}), + {R"([{"int64": 1}, {"int64": 2}, {"int64": 3}])"}); + AssertTablesEqual(*expected, *table, /*same_chunk_layout=*/false); +} + TEST(TestArrowReadWrite, InvalidTable) { // ARROW-4774: Shouldn't segfault on writing an invalid table. auto sink = CreateOutputStream(); diff --git a/cpp/submodules/parquet-testing b/cpp/submodules/parquet-testing index 56653c437c80..8a6a8881cf99 160000 --- a/cpp/submodules/parquet-testing +++ b/cpp/submodules/parquet-testing @@ -1 +1 @@ -Subproject commit 56653c437c8092f704a092d0d1d4e600124cd49f +Subproject commit 8a6a8881cf99b2e1951bb47e537decbebc1db12f From 4c61fbf4247e2bc3706903f0ef436a79244451e9 Mon Sep 17 00:00:00 2001 From: Krisztian Szucs Date: Fri, 30 Oct 2026 19:19:43 +0100 Subject: [PATCH 2/2] [C++][Parquet] Fix rows dropped after a data page without values EnsureDataPage() returned false on a data page without values, so the reader skipped the rest of the column chunk. Keep reading pages until one with values is found; the now unreachable batch_size == 0 checks become debug checks. --- cpp/src/parquet/column_reader.cc | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/cpp/src/parquet/column_reader.cc b/cpp/src/parquet/column_reader.cc index 6aca8a23ce16..3e2d12813496 100644 --- a/cpp/src/parquet/column_reader.cc +++ b/cpp/src/parquet/column_reader.cc @@ -1145,8 +1145,8 @@ class ColumnChunkReader { /// Return true if there is more data. /// - /// If the current page is exhausted, it will process more pages until some data - /// page is found. + /// If the current page is exhausted, it will process more pages until a data page + /// with values is found or the column chunk ends. bool EnsureDataPage(); /// Check the encoding of the current page or throw an exception. @@ -1279,8 +1279,8 @@ template bool ColumnChunkReader::EnsureDataPage() { // 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_decoded_values_ == num_buffered_values_) { + if (!ReadNewPage()) { return false; } } @@ -1438,7 +1438,7 @@ int32_t ColumnChunkReader::AdvanceLevels(int32_t num_levels) { template int64_t ColumnChunkReader::Skip(int64_t num_values_to_skip) { int64_t values_to_skip = num_values_to_skip; - // Optimization: Do not call HasNext() when values_to_skip == 0. + // Optimization: Do not call EnsureDataPage() when values_to_skip == 0. while (values_to_skip > 0 && EnsureDataPage()) { // If the number of values to skip is more than the number of undecoded values, skip // the whole Page without decoding levels or values. @@ -1925,11 +1925,7 @@ int64_t TypedRecordReader::ReadRecords(int64_t num_records) { /// or observe the desired number of records const int32_t batch_size = narrow_min(level_batch_size, this->available_values_current_page()); - - // No more data in column - if (batch_size == 0) { - break; - } + ARROW_DCHECK_GT(batch_size, 0); if (this->max_def_level() > 0) { def_levels_.Decode(this->def_levels_decoder_, batch_size); @@ -2020,7 +2016,7 @@ int64_t TypedRecordReader::SkipRecordsRepeated(int64_t num_records) { // of its values yet. while (!at_record_start_ || skipped_records < num_records) { // Is there more data to read in this row group? - // HasNextInternal() will advance to the next page if necessary. + // EnsureDataPage() will advance to the next page if necessary. if (!this->EnsureDataPage()) { if (!at_record_start_) { // We ended the row group while inside a record that we haven't seen @@ -2037,13 +2033,7 @@ int64_t TypedRecordReader::SkipRecordsRepeated(int64_t num_records) { std::min(level_batch_size, this->available_values_current_page()); // available_values_current_page fits in int32_t const auto batch_size = static_cast(batch_size_64); - - // No more data in column. This must be an empty page. - // If we had exhausted the last page, HasNextInternal() must have advanced - // to the next page. So there must be available values to process. - if (batch_size == 0) { - break; - } + ARROW_DCHECK_GT(batch_size, 0); def_levels_.Decode(this->def_levels_decoder_, batch_size); rep_levels_.Decode(this->rep_levels_decoder_, batch_size);