Skip to content

Commit af0c94f

Browse files
committed
fix(parquet): keep output schema consistent with produced list arrays
Arrow honors set_list_type(LARGE_LIST) only when it derives the Arrow schema from the Parquet schema. When a file carries serialized ARROW:schema metadata, the reader keeps producing plain list arrays, but the output schema was rewritten to large_list unconditionally. ProjectRecordBatch then built the projected batch against a large_list schema while the incoming arrays were list arrays, which casts a ListArray to a LargeListArray. The output schema is the target of the projection, so it keeps being derived from the projected Iceberg schema, and the large_list rewrite is now applied only when the reader actually produces large lists. Adds a regression test that reads a file written through parquet::arrow::WriteTable, which serializes ARROW:schema, with use-large-list enabled. Also addresses review comments: - comment the forward declaration of UseLargeListField - extract the duplicated field rewriting into UseLargeListFields
1 parent 430a53c commit af0c94f

2 files changed

Lines changed: 144 additions & 17 deletions

File tree

src/iceberg/parquet/parquet_reader.cc

Lines changed: 56 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
#include "iceberg/parquet/parquet_reader.h"
2121

22+
#include <algorithm>
2223
#include <numeric>
2324

2425
#include <arrow/c/bridge.h>
@@ -85,6 +86,7 @@ class EmptyRecordBatchReader : public ::arrow::RecordBatchReader {
8586
}
8687
};
8788

89+
// forward declaration to unblock cycle dependence.
8890
std::shared_ptr<::arrow::Field> UseLargeListField(
8991
const std::shared_ptr<::arrow::Field>& field);
9092

@@ -120,6 +122,42 @@ std::shared_ptr<::arrow::Field> UseLargeListField(
120122
return field->WithType(UseLargeListType(field->type()));
121123
}
122124

125+
// Rewrite all fields in a field vector to use large_list instead of list.
126+
::arrow::FieldVector UseLargeListFields(const ::arrow::FieldVector& fields) {
127+
::arrow::FieldVector rewritten;
128+
rewritten.reserve(fields.size());
129+
for (const auto& field : fields) {
130+
rewritten.push_back(UseLargeListField(field));
131+
}
132+
return rewritten;
133+
}
134+
135+
// Returns true if the type contains a large_list, at any level of nesting.
136+
bool ContainsLargeList(const ::arrow::DataType& type) {
137+
if (type.id() == ::arrow::Type::LARGE_LIST) {
138+
return true;
139+
}
140+
return std::ranges::any_of(
141+
type.fields(), [](const auto& field) { return ContainsLargeList(*field->type()); });
142+
}
143+
144+
// Returns true if the reader produces large_list arrays.
145+
//
146+
// Arrow honors the requested large_list type only when it derives the Arrow schema from
147+
// the Parquet schema. A file that carries serialized ARROW:schema metadata keeps its
148+
// original list type instead, so whether large lists are produced can only be told from
149+
// the schema of the reader.
150+
bool ProducesLargeList(const ::arrow::RecordBatchReader& reader) {
151+
const auto& schema = reader.schema();
152+
if (schema == nullptr) {
153+
// an empty reader produces no arrays to be described
154+
return false;
155+
}
156+
return std::ranges::any_of(schema->fields(), [](const auto& field) {
157+
return ContainsLargeList(*field->type());
158+
});
159+
}
160+
123161
} // namespace
124162

125163
// A stateful context to keep track of the reading progress.
@@ -252,23 +290,6 @@ class ParquetReader::Impl {
252290
Status InitReadContext() {
253291
context_ = std::make_unique<ReadContext>();
254292

255-
// Build the output Arrow schema
256-
ArrowSchema arrow_schema;
257-
ICEBERG_RETURN_UNEXPECTED(ToArrowSchema(*read_schema_, &arrow_schema));
258-
ICEBERG_ARROW_ASSIGN_OR_RETURN(context_->output_arrow_schema_,
259-
::arrow::ImportSchema(&arrow_schema));
260-
if (use_large_list_) {
261-
// Align the output schema with the large_list arrays produced by the
262-
// Parquet reader when kArrowUseLargeList is enabled.
263-
::arrow::FieldVector fields;
264-
fields.reserve(context_->output_arrow_schema_->fields().size());
265-
for (const auto& field : context_->output_arrow_schema_->fields()) {
266-
fields.push_back(UseLargeListField(field));
267-
}
268-
context_->output_arrow_schema_ =
269-
::arrow::schema(std::move(fields), context_->output_arrow_schema_->metadata());
270-
}
271-
272293
// Row group pruning based on the split
273294
// TODO(gangwu): add row group filtering based on zone map, bloom filter, etc.
274295
std::vector<int> row_group_indices;
@@ -301,6 +322,24 @@ class ParquetReader::Impl {
301322
reader_->GetRecordBatchReader(row_group_indices, column_indices));
302323
}
303324

325+
// Build the output Arrow schema from the projected Iceberg schema. This schema is the
326+
// target of ProjectRecordBatch, so it must describe the projected schema rather than
327+
// the schema of the file.
328+
ArrowSchema arrow_schema;
329+
ICEBERG_RETURN_UNEXPECTED(ToArrowSchema(*read_schema_, &arrow_schema));
330+
ICEBERG_ARROW_ASSIGN_OR_RETURN(context_->output_arrow_schema_,
331+
::arrow::ImportSchema(&arrow_schema));
332+
333+
if (use_large_list_ && ProducesLargeList(*context_->record_batch_reader_)) {
334+
// Align the output schema with the large_list arrays produced by the Parquet
335+
// reader. Note that Arrow ignores the requested list type when the file carries
336+
// serialized ARROW:schema metadata, in which case the reader keeps producing plain
337+
// list arrays and the output schema must keep describing them as such.
338+
context_->output_arrow_schema_ =
339+
::arrow::schema(UseLargeListFields(context_->output_arrow_schema_->fields()),
340+
context_->output_arrow_schema_->metadata());
341+
}
342+
304343
return {};
305344
}
306345

src/iceberg/test/parquet_test.cc

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,38 @@ class ParquetReaderTest : public TempFileTestBase {
293293
.data_sequence_number = data_sequence_number});
294294
}
295295

296+
// Writes a list parquet file through parquet::arrow::WriteTable, which serializes the
297+
// Arrow schema of the table into the ARROW:schema key value metadata of the file.
298+
void CreateListParquetFileWithArrowSchema() {
299+
const std::string kParquetFieldIdKey = "PARQUET:field_id";
300+
auto arrow_schema = ::arrow::schema(
301+
{::arrow::field("id", ::arrow::int32(), /*nullable=*/false,
302+
::arrow::KeyValueMetadata::Make({kParquetFieldIdKey}, {"1"})),
303+
::arrow::field(
304+
"numbers",
305+
::arrow::list(::arrow::field(
306+
"element", ::arrow::int32(), /*nullable=*/true,
307+
::arrow::KeyValueMetadata::Make({kParquetFieldIdKey}, {"101"}))),
308+
/*nullable=*/true,
309+
::arrow::KeyValueMetadata::Make({kParquetFieldIdKey}, {"2"}))});
310+
auto batch =
311+
::arrow::RecordBatch::FromStructArray(
312+
::arrow::json::ArrayFromJSONString(::arrow::struct_(arrow_schema->fields()),
313+
R"([[1, [1, 2]], [2, [3]]])")
314+
.ValueOrDie())
315+
.ValueOrDie();
316+
auto table = ::arrow::Table::FromRecordBatches(arrow_schema, {batch}).ValueOrDie();
317+
318+
auto io = internal::checked_cast<arrow::ArrowFileSystemFileIO&>(*file_io_);
319+
auto outfile = io.fs()->OpenOutputStream(temp_parquet_file_).ValueOrDie();
320+
321+
// write a single row group so that one batch holds every row
322+
ASSERT_TRUE(::parquet::arrow::WriteTable(*table, ::arrow::default_memory_pool(),
323+
outfile, table->num_rows())
324+
.ok());
325+
ASSERT_TRUE(outfile->Close().ok());
326+
}
327+
296328
void VerifyNextBatch(Reader& reader, std::string_view expected_json) {
297329
// Boilerplate to get Arrow schema
298330
auto schema_result = reader.Schema();
@@ -552,6 +584,62 @@ TEST_F(ParquetReaderTest, ReadListAsLargeList) {
552584
ASSERT_NO_FATAL_FAILURE(VerifyExhausted(*reader));
553585
}
554586

587+
TEST_F(ParquetReaderTest, ReadListAsLargeListWithArrowSchema) {
588+
// A file written with serialized ARROW:schema metadata keeps its original list type, as
589+
// Arrow ignores the requested large_list type in that case. The output schema must keep
590+
// describing the arrays that are actually produced, otherwise projecting the record
591+
// batch casts a list array to a large_list array.
592+
CreateListParquetFileWithArrowSchema();
593+
594+
auto schema = std::make_shared<Schema>(std::vector<SchemaField>{
595+
SchemaField::MakeRequired(1, "id", int32()),
596+
SchemaField::MakeOptional(2, "numbers",
597+
std::make_shared<ListType>(SchemaField::MakeOptional(
598+
/*field_id=*/101, "element", int32())))});
599+
600+
ReaderProperties reader_properties;
601+
reader_properties.Set(ReaderProperties::kArrowUseLargeList, true);
602+
603+
auto reader_result = ReaderFactoryRegistry::Open(
604+
FileFormatType::kParquet, {.path = temp_parquet_file_,
605+
.io = file_io_,
606+
.projection = schema,
607+
.properties = std::move(reader_properties)});
608+
ASSERT_THAT(reader_result, IsOk());
609+
auto reader = std::move(reader_result.value());
610+
611+
auto schema_result = reader->Schema();
612+
ASSERT_THAT(schema_result, IsOk());
613+
auto arrow_c_schema = std::move(schema_result.value());
614+
auto arrow_type = ::arrow::ImportType(&arrow_c_schema).ValueOrDie();
615+
ASSERT_EQ(arrow_type->field(1)->type()->id(), ::arrow::Type::LIST);
616+
617+
auto data = reader->Next();
618+
ASSERT_THAT(data, IsOk());
619+
ASSERT_TRUE(data.value().has_value());
620+
auto arrow_c_array = data.value().value();
621+
622+
// Importing the array against the reported schema fails if the two disagree.
623+
auto import_result = ::arrow::ImportArray(&arrow_c_array, arrow_type);
624+
ASSERT_TRUE(import_result.ok()) << import_result.status().ToString();
625+
auto arrow_array = import_result.ValueOrDie();
626+
ASSERT_TRUE(arrow_array->ValidateFull().ok());
627+
628+
const auto& struct_array =
629+
internal::checked_cast<const ::arrow::StructArray&>(*arrow_array);
630+
ASSERT_EQ(struct_array.length(), 2);
631+
632+
const auto& id_array =
633+
internal::checked_cast<const ::arrow::Int32Array&>(*struct_array.field(0));
634+
ASSERT_EQ(id_array.Value(0), 1);
635+
ASSERT_EQ(id_array.Value(1), 2);
636+
637+
const auto& numbers_array =
638+
internal::checked_cast<const ::arrow::ListArray&>(*struct_array.field(1));
639+
ASSERT_EQ(numbers_array.value_slice(0)->length(), 2);
640+
ASSERT_EQ(numbers_array.value_slice(1)->length(), 1);
641+
}
642+
555643
TEST_F(ParquetReaderTest, ReadSplit) {
556644
CreateSplitParquetFile();
557645

0 commit comments

Comments
 (0)