From 351a895f9c983cdf80a181d7f66b848058a98182 Mon Sep 17 00:00:00 2001 From: Xin Huang Date: Wed, 1 Jul 2026 20:51:39 -0700 Subject: [PATCH 1/4] feat(avro): apply column default values when reading missing fields (3/4) When a column is present in the read schema but missing from an Avro data file (written before the column existed), fill it with the column's v3 initial-default instead of null. Reuses the shared arrow/literal_util materializer (merged in #792) and adds AppendDefaultToBuilder for the row-by-row Avro decode paths, plus a kDefault projection branch in the Avro schema/data projection. Part 3 of the v3 column-default-values work (POC #731), built on the schema support in #746 and the Parquet read path in #792. --- src/iceberg/arrow/literal_util.cc | 24 ++++++++++++++ src/iceberg/arrow/literal_util_internal.h | 4 +++ src/iceberg/avro/avro_data_util.cc | 9 ++++++ src/iceberg/avro/avro_direct_decoder.cc | 4 +++ src/iceberg/avro/avro_schema_util.cc | 4 +++ src/iceberg/test/avro_data_test.cc | 38 +++++++++++++++++++++++ src/iceberg/test/avro_test.cc | 29 +++++++++++++++++ src/iceberg/test/literal_util_test.cc | 26 ++++++++++++++++ 8 files changed, 138 insertions(+) diff --git a/src/iceberg/arrow/literal_util.cc b/src/iceberg/arrow/literal_util.cc index e8ee4d6c6..d68f99956 100644 --- a/src/iceberg/arrow/literal_util.cc +++ b/src/iceberg/arrow/literal_util.cc @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -193,4 +194,27 @@ Result> MakeDefaultArray( return array; } +Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder) { + // The builder's own memory pool is not exposed, so the small scalar buffer uses the + // default pool. + ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr<::arrow::Scalar> scalar, + ToArrowScalar(literal, ::arrow::default_memory_pool())); + + // For an extension builder (e.g. `arrow.uuid`) target its storage type: ToArrowScalar + // yields the storage scalar (fixed_size_binary(16) for uuid) and Scalar::CastTo has no + // kernel that targets an extension type. This mirrors MakeDefaultArray's extension + // handling. + std::shared_ptr<::arrow::DataType> target_type = builder->type(); + if (target_type->id() == ::arrow::Type::EXTENSION) { + target_type = internal::checked_cast(*target_type) + .storage_type(); + } + + if (!scalar->type->Equals(*target_type)) { + ICEBERG_ARROW_ASSIGN_OR_RETURN(scalar, scalar->CastTo(target_type)); + } + ICEBERG_ARROW_RETURN_NOT_OK(builder->AppendScalar(*scalar)); + return {}; +} + } // namespace iceberg::arrow diff --git a/src/iceberg/arrow/literal_util_internal.h b/src/iceberg/arrow/literal_util_internal.h index 4de60b528..d4c22a89a 100644 --- a/src/iceberg/arrow/literal_util_internal.h +++ b/src/iceberg/arrow/literal_util_internal.h @@ -44,4 +44,8 @@ Result> MakeDefaultArray( const Literal& literal, const std::shared_ptr<::arrow::DataType>& type, int64_t num_rows, ::arrow::MemoryPool* pool); +/// \brief Append the literal value once to `builder`, e.g. to materialize a missing +/// field with a default value while building rows one at a time. +Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder); + } // namespace iceberg::arrow diff --git a/src/iceberg/avro/avro_data_util.cc b/src/iceberg/avro/avro_data_util.cc index 7931b0719..de556cfbb 100644 --- a/src/iceberg/avro/avro_data_util.cc +++ b/src/iceberg/avro/avro_data_util.cc @@ -31,6 +31,7 @@ #include #include "iceberg/arrow/arrow_status_internal.h" +#include "iceberg/arrow/literal_util_internal.h" #include "iceberg/avro/avro_data_util_internal.h" #include "iceberg/avro/avro_schema_util_internal.h" #include "iceberg/metadata_columns.h" @@ -88,6 +89,9 @@ Status AppendStructToBuilder(const ::avro::NodePtr& avro_node, metadata_context, field_builder)); } else if (field_projection.kind == FieldProjection::Kind::kNull) { ICEBERG_ARROW_RETURN_NOT_OK(field_builder->AppendNull()); + } else if (field_projection.kind == FieldProjection::Kind::kDefault) { + ICEBERG_RETURN_UNEXPECTED(arrow::AppendDefaultToBuilder( + std::get(field_projection.from), field_builder)); } else if (field_projection.kind == FieldProjection::Kind::kMetadata) { int32_t field_id = expected_field.field_id(); if (field_id == MetadataColumns::kFilePathColumnId) { @@ -466,6 +470,11 @@ Status AppendFieldToBuilder(const ::avro::NodePtr& avro_node, return {}; } + if (projection.kind == FieldProjection::Kind::kDefault) { + return arrow::AppendDefaultToBuilder(std::get(projection.from), + array_builder); + } + const bool is_row_lineage = MetadataColumns::IsRowLineageColumn(projected_field.field_id()); diff --git a/src/iceberg/avro/avro_direct_decoder.cc b/src/iceberg/avro/avro_direct_decoder.cc index b66a93109..47ffafa92 100644 --- a/src/iceberg/avro/avro_direct_decoder.cc +++ b/src/iceberg/avro/avro_direct_decoder.cc @@ -29,6 +29,7 @@ #include #include "iceberg/arrow/arrow_status_internal.h" +#include "iceberg/arrow/literal_util_internal.h" #include "iceberg/avro/avro_direct_decoder_internal.h" #include "iceberg/avro/avro_schema_util_internal.h" #include "iceberg/metadata_columns.h" @@ -209,6 +210,9 @@ Status DecodeStructToBuilder(const ::avro::NodePtr& avro_node, ::avro::Decoder& auto* field_builder = struct_builder->field_builder(static_cast(proj_idx)); if (field_projection.kind == FieldProjection::Kind::kNull) { ICEBERG_ARROW_RETURN_NOT_OK(field_builder->AppendNull()); + } else if (field_projection.kind == FieldProjection::Kind::kDefault) { + ICEBERG_RETURN_UNEXPECTED(arrow::AppendDefaultToBuilder( + std::get(field_projection.from), field_builder)); } else if (field_projection.kind == FieldProjection::Kind::kMetadata) { int32_t field_id = expected_field.field_id(); if (field_id == MetadataColumns::kFilePathColumnId) { diff --git a/src/iceberg/avro/avro_schema_util.cc b/src/iceberg/avro/avro_schema_util.cc index 14b464cee..5e6bee957 100644 --- a/src/iceberg/avro/avro_schema_util.cc +++ b/src/iceberg/avro/avro_schema_util.cc @@ -752,6 +752,10 @@ Result ProjectStruct(const StructType& struct_type, iter->second.local_index, prune_source)); } else if (MetadataColumns::IsMetadataColumn(field_id)) { child_projection.kind = FieldProjection::Kind::kMetadata; + } else if (expected_field.initial_default() != nullptr) { + // Rows written before the field existed assume its `initial-default` value. + child_projection.kind = FieldProjection::Kind::kDefault; + child_projection.from = *expected_field.initial_default(); } else if (expected_field.optional()) { child_projection.kind = FieldProjection::Kind::kNull; } else { diff --git a/src/iceberg/test/avro_data_test.cc b/src/iceberg/test/avro_data_test.cc index 7731f58d3..a3e970edd 100644 --- a/src/iceberg/test/avro_data_test.cc +++ b/src/iceberg/test/avro_data_test.cc @@ -31,6 +31,7 @@ #include "iceberg/avro/avro_data_util_internal.h" #include "iceberg/avro/avro_schema_util_internal.h" +#include "iceberg/expression/literal.h" #include "iceberg/schema.h" #include "iceberg/schema_internal.h" #include "iceberg/schema_util.h" @@ -662,6 +663,43 @@ TEST(AppendDatumToBuilderTest, StructWithMissingOptionalField) { avro_data, expected_json)); } +TEST(AppendDatumToBuilderTest, StructWithMissingDefaultFields) { + Schema iceberg_schema({ + SchemaField::MakeRequired(1, "id", iceberg::int32()), + // Missing required field with an initial-default: filled with the default. + SchemaField(2, "score", iceberg::int64(), /*optional=*/false, /*doc=*/{}, + std::make_shared(Literal::Long(100))), + // Missing optional field with an initial-default: also filled, not null. + SchemaField(3, "grade", iceberg::string(), /*optional=*/true, /*doc=*/{}, + std::make_shared(Literal::String("A"))), + }); + + // Create Avro schema that only has the id field (missing score and grade). + std::string avro_schema_json = R"({ + "type": "record", + "name": "person", + "fields": [ + {"name": "id", "type": "int", "field-id": 1} + ] + })"; + auto avro_schema = ::avro::compileJsonSchemaFromString(avro_schema_json); + + std::vector<::avro::GenericDatum> avro_data; + for (int i = 0; i < 2; ++i) { + ::avro::GenericDatum avro_datum(avro_schema.root()); + auto& record = avro_datum.value<::avro::GenericRecord>(); + record.fieldAt(0).value() = i + 1; + avro_data.push_back(avro_datum); + } + + const std::string expected_json = R"([ + {"id": 1, "score": 100, "grade": "A"}, + {"id": 2, "score": 100, "grade": "A"} + ])"; + ASSERT_NO_FATAL_FAILURE(VerifyAppendDatumToBuilder(iceberg_schema, avro_schema.root(), + avro_data, expected_json)); +} + TEST(AppendDatumToBuilderTest, NestedStructWithMissingOptionalFields) { Schema iceberg_schema({ SchemaField::MakeRequired(1, "id", iceberg::int32()), diff --git a/src/iceberg/test/avro_test.cc b/src/iceberg/test/avro_test.cc index 604cf5b0a..20156a57c 100644 --- a/src/iceberg/test/avro_test.cc +++ b/src/iceberg/test/avro_test.cc @@ -42,6 +42,7 @@ #include "iceberg/avro/avro_register.h" #include "iceberg/avro/avro_stream_internal.h" #include "iceberg/avro/avro_writer.h" +#include "iceberg/expression/literal.h" #include "iceberg/file_reader.h" #include "iceberg/metadata_columns.h" #include "iceberg/schema.h" @@ -318,6 +319,34 @@ TEST_F(AvroReaderTest, ReadTwoFields) { ASSERT_NO_FATAL_FAILURE(VerifyExhausted(*reader)); } +TEST_F(AvroReaderTest, ReadMissingFieldsWithDefaults) { + // The file contains only fields 1 and 2; the projected schema adds fields 3 and 4 + // with initial-defaults, which are filled for all rows written before the columns + // existed. + CreateSimpleAvroFile(); + + auto schema = std::make_shared(std::vector{ + SchemaField::MakeRequired(1, "id", std::make_shared()), + SchemaField::MakeOptional(2, "name", std::make_shared()), + SchemaField(3, "score", std::make_shared(), /*optional=*/false, + /*doc=*/{}, std::make_shared(Literal::Long(100))), + SchemaField(4, "status", std::make_shared(), /*optional=*/true, + /*doc=*/{}, std::make_shared(Literal::String("active"))), + }); + + auto reader_result = ReaderFactoryRegistry::Open( + FileFormatType::kAvro, + {.path = temp_avro_file_, .io = file_io_, .projection = schema}); + ASSERT_THAT(reader_result, IsOk()); + auto reader = std::move(reader_result.value()); + + ASSERT_NO_FATAL_FAILURE(VerifyNextBatch(*reader, + R"([[1, "Alice", 100, "active"], + [2, "Bob", 100, "active"], + [3, "Charlie", 100, "active"]])")); + ASSERT_NO_FATAL_FAILURE(VerifyExhausted(*reader)); +} + TEST_F(AvroReaderTest, RoundTripWithGenericFileIO) { file_io_ = std::make_shared(); temp_avro_file_ = CreateNewTempFilePathWithSuffix(".avro"); diff --git a/src/iceberg/test/literal_util_test.cc b/src/iceberg/test/literal_util_test.cc index 9f21050f3..94cd536b5 100644 --- a/src/iceberg/test/literal_util_test.cc +++ b/src/iceberg/test/literal_util_test.cc @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -186,6 +187,31 @@ TEST(LiteralUtilTest, MakeDefaultArrayWrapsExtensionType) { } } +TEST(LiteralUtilTest, AppendDefaultToBuilderAppendsValue) { + ::arrow::Int64Builder builder; + ASSERT_THAT(AppendDefaultToBuilder(Literal::Long(42), &builder), IsOk()); + ASSERT_THAT(AppendDefaultToBuilder(Literal::Long(42), &builder), IsOk()); + + std::shared_ptr<::arrow::Array> array; + ASSERT_TRUE(builder.Finish(&array).ok()); + ASSERT_EQ(array->length(), 2); + const auto& long_array = static_cast(*array); + ASSERT_EQ(long_array.Value(0), 42); + ASSERT_EQ(long_array.Value(1), 42); +} + +TEST(LiteralUtilTest, AppendDefaultToBuilderCastsToBuilderType) { + // The literal's natural type (int32) differs from the builder type (int64); the value + // is cast to the builder type. + ::arrow::Int64Builder builder; + ASSERT_THAT(AppendDefaultToBuilder(Literal::Int(7), &builder), IsOk()); + + std::shared_ptr<::arrow::Array> array; + ASSERT_TRUE(builder.Finish(&array).ok()); + ASSERT_EQ(array->length(), 1); + ASSERT_EQ(static_cast(*array).Value(0), 7); +} + } // namespace } // namespace iceberg::arrow From 9e1605f03e2448129db1d96453ee8c7111938de7 Mon Sep 17 00:00:00 2001 From: Xin Huang Date: Sun, 19 Jul 2026 21:19:05 -0700 Subject: [PATCH 2/4] refactor(avro): keep AppendDefaultToBuilder local to Avro Row-oriented Avro decode needs per-builder appends, while Parquet uses batch MakeDefaultArray. Share ToArrowScalar only; don't put the Avro shape into the shared arrow literal util. --- src/iceberg/arrow/literal_util.cc | 24 ------------------ src/iceberg/arrow/literal_util_internal.h | 4 --- src/iceberg/avro/avro_data_util.cc | 29 +++++++++++++++++++--- src/iceberg/avro/avro_data_util_internal.h | 8 ++++++ src/iceberg/avro/avro_direct_decoder.cc | 4 +-- src/iceberg/test/avro_data_test.cc | 26 +++++++++++++++++++ src/iceberg/test/literal_util_test.cc | 26 ------------------- 7 files changed, 62 insertions(+), 59 deletions(-) diff --git a/src/iceberg/arrow/literal_util.cc b/src/iceberg/arrow/literal_util.cc index d68f99956..e8ee4d6c6 100644 --- a/src/iceberg/arrow/literal_util.cc +++ b/src/iceberg/arrow/literal_util.cc @@ -22,7 +22,6 @@ #include #include -#include #include #include #include @@ -194,27 +193,4 @@ Result> MakeDefaultArray( return array; } -Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder) { - // The builder's own memory pool is not exposed, so the small scalar buffer uses the - // default pool. - ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr<::arrow::Scalar> scalar, - ToArrowScalar(literal, ::arrow::default_memory_pool())); - - // For an extension builder (e.g. `arrow.uuid`) target its storage type: ToArrowScalar - // yields the storage scalar (fixed_size_binary(16) for uuid) and Scalar::CastTo has no - // kernel that targets an extension type. This mirrors MakeDefaultArray's extension - // handling. - std::shared_ptr<::arrow::DataType> target_type = builder->type(); - if (target_type->id() == ::arrow::Type::EXTENSION) { - target_type = internal::checked_cast(*target_type) - .storage_type(); - } - - if (!scalar->type->Equals(*target_type)) { - ICEBERG_ARROW_ASSIGN_OR_RETURN(scalar, scalar->CastTo(target_type)); - } - ICEBERG_ARROW_RETURN_NOT_OK(builder->AppendScalar(*scalar)); - return {}; -} - } // namespace iceberg::arrow diff --git a/src/iceberg/arrow/literal_util_internal.h b/src/iceberg/arrow/literal_util_internal.h index d4c22a89a..4de60b528 100644 --- a/src/iceberg/arrow/literal_util_internal.h +++ b/src/iceberg/arrow/literal_util_internal.h @@ -44,8 +44,4 @@ Result> MakeDefaultArray( const Literal& literal, const std::shared_ptr<::arrow::DataType>& type, int64_t num_rows, ::arrow::MemoryPool* pool); -/// \brief Append the literal value once to `builder`, e.g. to materialize a missing -/// field with a default value while building rows one at a time. -Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder); - } // namespace iceberg::arrow diff --git a/src/iceberg/avro/avro_data_util.cc b/src/iceberg/avro/avro_data_util.cc index de556cfbb..09d65adf2 100644 --- a/src/iceberg/avro/avro_data_util.cc +++ b/src/iceberg/avro/avro_data_util.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -90,7 +91,7 @@ Status AppendStructToBuilder(const ::avro::NodePtr& avro_node, } else if (field_projection.kind == FieldProjection::Kind::kNull) { ICEBERG_ARROW_RETURN_NOT_OK(field_builder->AppendNull()); } else if (field_projection.kind == FieldProjection::Kind::kDefault) { - ICEBERG_RETURN_UNEXPECTED(arrow::AppendDefaultToBuilder( + ICEBERG_RETURN_UNEXPECTED(AppendDefaultToBuilder( std::get(field_projection.from), field_builder)); } else if (field_projection.kind == FieldProjection::Kind::kMetadata) { int32_t field_id = expected_field.field_id(); @@ -471,8 +472,7 @@ Status AppendFieldToBuilder(const ::avro::NodePtr& avro_node, } if (projection.kind == FieldProjection::Kind::kDefault) { - return arrow::AppendDefaultToBuilder(std::get(projection.from), - array_builder); + return AppendDefaultToBuilder(std::get(projection.from), array_builder); } const bool is_row_lineage = @@ -506,6 +506,29 @@ Status AppendFieldToBuilder(const ::avro::NodePtr& avro_node, } // namespace +Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder) { + // The builder's own memory pool is not exposed, so the small scalar buffer uses the + // default pool. + ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr<::arrow::Scalar> scalar, + arrow::ToArrowScalar(literal, ::arrow::default_memory_pool())); + + // For an extension builder (e.g. `arrow.uuid`) target its storage type: ToArrowScalar + // yields the storage scalar (fixed_size_binary(16) for uuid) and Scalar::CastTo has no + // kernel that targets an extension type. This mirrors MakeDefaultArray's extension + // handling. + std::shared_ptr<::arrow::DataType> target_type = builder->type(); + if (target_type->id() == ::arrow::Type::EXTENSION) { + target_type = internal::checked_cast(*target_type) + .storage_type(); + } + + if (!scalar->type->Equals(*target_type)) { + ICEBERG_ARROW_ASSIGN_OR_RETURN(scalar, scalar->CastTo(target_type)); + } + ICEBERG_ARROW_RETURN_NOT_OK(builder->AppendScalar(*scalar)); + return {}; +} + Status AppendDatumToBuilder(const ::avro::NodePtr& avro_node, const ::avro::GenericDatum& avro_datum, const SchemaProjection& projection, diff --git a/src/iceberg/avro/avro_data_util_internal.h b/src/iceberg/avro/avro_data_util_internal.h index 281187f12..ac08d32f1 100644 --- a/src/iceberg/avro/avro_data_util_internal.h +++ b/src/iceberg/avro/avro_data_util_internal.h @@ -23,10 +23,18 @@ #include #include "iceberg/arrow/metadata_column_util_internal.h" +#include "iceberg/expression/literal.h" #include "iceberg/schema_util.h" namespace iceberg::avro { +/// \brief Append a literal once to `builder` while decoding Avro row-by-row. +/// +/// Used to materialize `FieldProjection::Kind::kDefault`. Shares `ToArrowScalar` with +/// Parquet's batch path (`MakeDefaultArray`); the append shape stays Avro-local because +/// Avro builds Arrow arrays via per-row `ArrayBuilder`s rather than whole-column arrays. +Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder); + /// \brief Append an Avro datum to an Arrow array builder. /// /// This function handles schema evolution by using the provided projection to map diff --git a/src/iceberg/avro/avro_direct_decoder.cc b/src/iceberg/avro/avro_direct_decoder.cc index 47ffafa92..00b7ac194 100644 --- a/src/iceberg/avro/avro_direct_decoder.cc +++ b/src/iceberg/avro/avro_direct_decoder.cc @@ -29,7 +29,7 @@ #include #include "iceberg/arrow/arrow_status_internal.h" -#include "iceberg/arrow/literal_util_internal.h" +#include "iceberg/avro/avro_data_util_internal.h" #include "iceberg/avro/avro_direct_decoder_internal.h" #include "iceberg/avro/avro_schema_util_internal.h" #include "iceberg/metadata_columns.h" @@ -211,7 +211,7 @@ Status DecodeStructToBuilder(const ::avro::NodePtr& avro_node, ::avro::Decoder& if (field_projection.kind == FieldProjection::Kind::kNull) { ICEBERG_ARROW_RETURN_NOT_OK(field_builder->AppendNull()); } else if (field_projection.kind == FieldProjection::Kind::kDefault) { - ICEBERG_RETURN_UNEXPECTED(arrow::AppendDefaultToBuilder( + ICEBERG_RETURN_UNEXPECTED(AppendDefaultToBuilder( std::get(field_projection.from), field_builder)); } else if (field_projection.kind == FieldProjection::Kind::kMetadata) { int32_t field_id = expected_field.field_id(); diff --git a/src/iceberg/test/avro_data_test.cc b/src/iceberg/test/avro_data_test.cc index a3e970edd..44e14da18 100644 --- a/src/iceberg/test/avro_data_test.cc +++ b/src/iceberg/test/avro_data_test.cc @@ -19,6 +19,7 @@ #include +#include #include #include #include @@ -700,6 +701,31 @@ TEST(AppendDatumToBuilderTest, StructWithMissingDefaultFields) { avro_data, expected_json)); } +TEST(AppendDefaultToBuilderTest, AppendsValue) { + ::arrow::Int64Builder builder; + ASSERT_THAT(AppendDefaultToBuilder(Literal::Long(42), &builder), IsOk()); + ASSERT_THAT(AppendDefaultToBuilder(Literal::Long(42), &builder), IsOk()); + + std::shared_ptr<::arrow::Array> array; + ASSERT_TRUE(builder.Finish(&array).ok()); + ASSERT_EQ(array->length(), 2); + const auto& long_array = static_cast(*array); + ASSERT_EQ(long_array.Value(0), 42); + ASSERT_EQ(long_array.Value(1), 42); +} + +TEST(AppendDefaultToBuilderTest, CastsToBuilderType) { + // The literal's natural type (int32) differs from the builder type (int64); the value + // is cast to the builder type. + ::arrow::Int64Builder builder; + ASSERT_THAT(AppendDefaultToBuilder(Literal::Int(7), &builder), IsOk()); + + std::shared_ptr<::arrow::Array> array; + ASSERT_TRUE(builder.Finish(&array).ok()); + ASSERT_EQ(array->length(), 1); + ASSERT_EQ(static_cast(*array).Value(0), 7); +} + TEST(AppendDatumToBuilderTest, NestedStructWithMissingOptionalFields) { Schema iceberg_schema({ SchemaField::MakeRequired(1, "id", iceberg::int32()), diff --git a/src/iceberg/test/literal_util_test.cc b/src/iceberg/test/literal_util_test.cc index 94cd536b5..9f21050f3 100644 --- a/src/iceberg/test/literal_util_test.cc +++ b/src/iceberg/test/literal_util_test.cc @@ -25,7 +25,6 @@ #include #include -#include #include #include #include @@ -187,31 +186,6 @@ TEST(LiteralUtilTest, MakeDefaultArrayWrapsExtensionType) { } } -TEST(LiteralUtilTest, AppendDefaultToBuilderAppendsValue) { - ::arrow::Int64Builder builder; - ASSERT_THAT(AppendDefaultToBuilder(Literal::Long(42), &builder), IsOk()); - ASSERT_THAT(AppendDefaultToBuilder(Literal::Long(42), &builder), IsOk()); - - std::shared_ptr<::arrow::Array> array; - ASSERT_TRUE(builder.Finish(&array).ok()); - ASSERT_EQ(array->length(), 2); - const auto& long_array = static_cast(*array); - ASSERT_EQ(long_array.Value(0), 42); - ASSERT_EQ(long_array.Value(1), 42); -} - -TEST(LiteralUtilTest, AppendDefaultToBuilderCastsToBuilderType) { - // The literal's natural type (int32) differs from the builder type (int64); the value - // is cast to the builder type. - ::arrow::Int64Builder builder; - ASSERT_THAT(AppendDefaultToBuilder(Literal::Int(7), &builder), IsOk()); - - std::shared_ptr<::arrow::Array> array; - ASSERT_TRUE(builder.Finish(&array).ok()); - ASSERT_EQ(array->length(), 1); - ASSERT_EQ(static_cast(*array).Value(0), 7); -} - } // namespace } // namespace iceberg::arrow From 0e4c6a40f9d141db1833c18bc8304ccd3b7905fa Mon Sep 17 00:00:00 2001 From: Xin Huang Date: Wed, 22 Jul 2026 09:04:47 -0700 Subject: [PATCH 3/4] Address review: cache default Arrow scalars for Avro reads Precompute cast scalars in FieldProjection attributes via PrepareDefaultScalars at reader init so per-row default fills only call AppendScalar. --- src/iceberg/avro/avro_data_util.cc | 110 ++++++++++++++++++++- src/iceberg/avro/avro_data_util_internal.h | 26 +++++ src/iceberg/avro/avro_direct_decoder.cc | 3 +- src/iceberg/avro/avro_reader.cc | 2 + src/iceberg/test/avro_data_test.cc | 29 ++++++ 5 files changed, 163 insertions(+), 7 deletions(-) diff --git a/src/iceberg/avro/avro_data_util.cc b/src/iceberg/avro/avro_data_util.cc index 09d65adf2..d81bea977 100644 --- a/src/iceberg/avro/avro_data_util.cc +++ b/src/iceberg/avro/avro_data_util.cc @@ -17,6 +17,8 @@ * under the License. */ +#include + #include #include #include @@ -91,8 +93,7 @@ Status AppendStructToBuilder(const ::avro::NodePtr& avro_node, } else if (field_projection.kind == FieldProjection::Kind::kNull) { ICEBERG_ARROW_RETURN_NOT_OK(field_builder->AppendNull()); } else if (field_projection.kind == FieldProjection::Kind::kDefault) { - ICEBERG_RETURN_UNEXPECTED(AppendDefaultToBuilder( - std::get(field_projection.from), field_builder)); + ICEBERG_RETURN_UNEXPECTED(AppendDefaultToBuilder(field_projection, field_builder)); } else if (field_projection.kind == FieldProjection::Kind::kMetadata) { int32_t field_id = expected_field.field_id(); if (field_id == MetadataColumns::kFilePathColumnId) { @@ -472,7 +473,7 @@ Status AppendFieldToBuilder(const ::avro::NodePtr& avro_node, } if (projection.kind == FieldProjection::Kind::kDefault) { - return AppendDefaultToBuilder(std::get(projection.from), array_builder); + return AppendDefaultToBuilder(projection, array_builder); } const bool is_row_lineage = @@ -506,7 +507,10 @@ Status AppendFieldToBuilder(const ::avro::NodePtr& avro_node, } // namespace -Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder) { +namespace { + +Result> MakeDefaultScalar( + const Literal& literal, const std::shared_ptr<::arrow::DataType>& builder_type) { // The builder's own memory pool is not exposed, so the small scalar buffer uses the // default pool. ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr<::arrow::Scalar> scalar, @@ -516,7 +520,7 @@ Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* bui // yields the storage scalar (fixed_size_binary(16) for uuid) and Scalar::CastTo has no // kernel that targets an extension type. This mirrors MakeDefaultArray's extension // handling. - std::shared_ptr<::arrow::DataType> target_type = builder->type(); + std::shared_ptr<::arrow::DataType> target_type = builder_type; if (target_type->id() == ::arrow::Type::EXTENSION) { target_type = internal::checked_cast(*target_type) .storage_type(); @@ -525,10 +529,106 @@ Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* bui if (!scalar->type->Equals(*target_type)) { ICEBERG_ARROW_ASSIGN_OR_RETURN(scalar, scalar->CastTo(target_type)); } + return scalar; +} + +Status PrepareDefaultScalars(std::span projections, + ::arrow::ArrayBuilder* builder) { + auto* struct_builder = internal::checked_cast<::arrow::StructBuilder*>(builder); + if (static_cast(struct_builder->num_fields()) != projections.size()) { + return InvalidArgument( + "Inconsistent number of struct builder fields ({}) and projections ({})", + struct_builder->num_fields(), projections.size()); + } + + for (size_t i = 0; i < projections.size(); ++i) { + auto& field_projection = projections[i]; + auto* field_builder = struct_builder->field_builder(static_cast(i)); + + if (field_projection.kind == FieldProjection::Kind::kDefault) { + if (dynamic_cast(field_projection.attributes.get()) != + nullptr) { + continue; + } + auto attrs = std::make_shared(); + ICEBERG_ASSIGN_OR_RAISE(attrs->scalar, + MakeDefaultScalar(std::get(field_projection.from), + field_builder->type())); + field_projection.attributes = std::move(attrs); + continue; + } + + if (field_projection.kind != FieldProjection::Kind::kProjected || + field_projection.children.empty()) { + continue; + } + + switch (field_builder->type()->id()) { + case ::arrow::Type::STRUCT: + ICEBERG_RETURN_UNEXPECTED( + PrepareDefaultScalars(field_projection.children, field_builder)); + break; + case ::arrow::Type::LIST: { + // List projections store a single child for the element. Defaults only appear + // on nested struct fields of that element. + auto* list_builder = internal::checked_cast<::arrow::ListBuilder*>(field_builder); + auto& element_projection = field_projection.children[0]; + if (element_projection.kind == FieldProjection::Kind::kProjected && + !element_projection.children.empty() && + list_builder->value_builder()->type()->id() == ::arrow::Type::STRUCT) { + ICEBERG_RETURN_UNEXPECTED(PrepareDefaultScalars(element_projection.children, + list_builder->value_builder())); + } + break; + } + case ::arrow::Type::MAP: { + auto* map_builder = internal::checked_cast<::arrow::MapBuilder*>(field_builder); + if (field_projection.children.size() >= 1 && + !field_projection.children[0].children.empty() && + map_builder->key_builder()->type()->id() == ::arrow::Type::STRUCT) { + ICEBERG_RETURN_UNEXPECTED(PrepareDefaultScalars( + field_projection.children[0].children, map_builder->key_builder())); + } + if (field_projection.children.size() >= 2 && + !field_projection.children[1].children.empty() && + map_builder->item_builder()->type()->id() == ::arrow::Type::STRUCT) { + ICEBERG_RETURN_UNEXPECTED(PrepareDefaultScalars( + field_projection.children[1].children, map_builder->item_builder())); + } + break; + } + default: + break; + } + } + return {}; +} + +} // namespace + +Status PrepareDefaultScalars(SchemaProjection& projection, + ::arrow::ArrayBuilder* root_builder) { + return PrepareDefaultScalars(projection.fields, root_builder); +} + +Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder) { + ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr<::arrow::Scalar> scalar, + MakeDefaultScalar(literal, builder->type())); ICEBERG_ARROW_RETURN_NOT_OK(builder->AppendScalar(*scalar)); return {}; } +Status AppendDefaultToBuilder(const FieldProjection& projection, + ::arrow::ArrayBuilder* builder) { + if (const auto* attrs = + dynamic_cast(projection.attributes.get()); + attrs != nullptr && attrs->scalar != nullptr) { + ICEBERG_ARROW_RETURN_NOT_OK(builder->AppendScalar(*attrs->scalar)); + return {}; + } + return AppendDefaultToBuilder(std::get(projection.from), builder); +} + Status AppendDatumToBuilder(const ::avro::NodePtr& avro_node, const ::avro::GenericDatum& avro_datum, const SchemaProjection& projection, diff --git a/src/iceberg/avro/avro_data_util_internal.h b/src/iceberg/avro/avro_data_util_internal.h index ac08d32f1..f894f204f 100644 --- a/src/iceberg/avro/avro_data_util_internal.h +++ b/src/iceberg/avro/avro_data_util_internal.h @@ -19,7 +19,10 @@ #pragma once +#include + #include +#include #include #include "iceberg/arrow/metadata_column_util_internal.h" @@ -28,13 +31,36 @@ namespace iceberg::avro { +/// \brief Cached Arrow scalar for a `kDefault` field projection. +/// +/// Materialized once (see `PrepareDefaultScalars`) so row-by-row Avro decode only +/// needs `AppendScalar` instead of repeating `ToArrowScalar` / `CastTo` per row. +struct AvroDefaultAttributes : FieldProjection::ExtraAttributes { + std::shared_ptr<::arrow::Scalar> scalar; +}; + +/// \brief Precompute cast Arrow scalars for every `kDefault` field under `projection`. +/// +/// Walks `root_builder` in lockstep with the projection so each default is cast to the +/// builder's Arrow type once per scan. Safe to call repeatedly; existing +/// `AvroDefaultAttributes` entries are left unchanged. +Status PrepareDefaultScalars(SchemaProjection& projection, + ::arrow::ArrayBuilder* root_builder); + /// \brief Append a literal once to `builder` while decoding Avro row-by-row. /// /// Used to materialize `FieldProjection::Kind::kDefault`. Shares `ToArrowScalar` with /// Parquet's batch path (`MakeDefaultArray`); the append shape stays Avro-local because /// Avro builds Arrow arrays via per-row `ArrayBuilder`s rather than whole-column arrays. +/// Prefer the `FieldProjection` overload after `PrepareDefaultScalars` so the scalar is +/// reused across rows. Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder); +/// \brief Append a `kDefault` projection, reusing a scalar cached on +/// `projection.attributes` when present. +Status AppendDefaultToBuilder(const FieldProjection& projection, + ::arrow::ArrayBuilder* builder); + /// \brief Append an Avro datum to an Arrow array builder. /// /// This function handles schema evolution by using the provided projection to map diff --git a/src/iceberg/avro/avro_direct_decoder.cc b/src/iceberg/avro/avro_direct_decoder.cc index 00b7ac194..8f563b3ba 100644 --- a/src/iceberg/avro/avro_direct_decoder.cc +++ b/src/iceberg/avro/avro_direct_decoder.cc @@ -211,8 +211,7 @@ Status DecodeStructToBuilder(const ::avro::NodePtr& avro_node, ::avro::Decoder& if (field_projection.kind == FieldProjection::Kind::kNull) { ICEBERG_ARROW_RETURN_NOT_OK(field_builder->AppendNull()); } else if (field_projection.kind == FieldProjection::Kind::kDefault) { - ICEBERG_RETURN_UNEXPECTED(AppendDefaultToBuilder( - std::get(field_projection.from), field_builder)); + ICEBERG_RETURN_UNEXPECTED(AppendDefaultToBuilder(field_projection, field_builder)); } else if (field_projection.kind == FieldProjection::Kind::kMetadata) { int32_t field_id = expected_field.field_id(); if (field_id == MetadataColumns::kFilePathColumnId) { diff --git a/src/iceberg/avro/avro_reader.cc b/src/iceberg/avro/avro_reader.cc index b0977bcdb..f333a38cb 100644 --- a/src/iceberg/avro/avro_reader.cc +++ b/src/iceberg/avro/avro_reader.cc @@ -434,6 +434,8 @@ class AvroReader::Impl { builder_result.status().message()); } context_->builder_ = builder_result.MoveValueUnsafe(); + ICEBERG_RETURN_UNEXPECTED( + PrepareDefaultScalars(projection_, context_->builder_.get())); backend_->InitReadContext(backend_->GetReaderSchema()); return {}; diff --git a/src/iceberg/test/avro_data_test.cc b/src/iceberg/test/avro_data_test.cc index 44e14da18..66678c8b4 100644 --- a/src/iceberg/test/avro_data_test.cc +++ b/src/iceberg/test/avro_data_test.cc @@ -726,6 +726,35 @@ TEST(AppendDefaultToBuilderTest, CastsToBuilderType) { ASSERT_EQ(static_cast(*array).Value(0), 7); } +TEST(AppendDefaultToBuilderTest, ReusesPreparedScalar) { + auto pool = ::arrow::default_memory_pool(); + auto child = std::make_shared<::arrow::Int64Builder>(pool); + ::arrow::StructBuilder struct_builder( + ::arrow::struct_({::arrow::field("d", ::arrow::int64())}), pool, {child}); + + FieldProjection projection; + projection.kind = FieldProjection::Kind::kDefault; + projection.from = Literal::Long(42); + + SchemaProjection schema_projection; + schema_projection.fields.push_back(projection); + ASSERT_THAT(PrepareDefaultScalars(schema_projection, &struct_builder), IsOk()); + ASSERT_NE(dynamic_cast( + schema_projection.fields[0].attributes.get()), + nullptr); + + auto* field_builder = struct_builder.field_builder(0); + ASSERT_THAT(AppendDefaultToBuilder(schema_projection.fields[0], field_builder), IsOk()); + ASSERT_THAT(AppendDefaultToBuilder(schema_projection.fields[0], field_builder), IsOk()); + + std::shared_ptr<::arrow::Array> array; + ASSERT_TRUE(field_builder->Finish(&array).ok()); + ASSERT_EQ(array->length(), 2); + const auto& long_array = static_cast(*array); + ASSERT_EQ(long_array.Value(0), 42); + ASSERT_EQ(long_array.Value(1), 42); +} + TEST(AppendDatumToBuilderTest, NestedStructWithMissingOptionalFields) { Schema iceberg_schema({ SchemaField::MakeRequired(1, "id", iceberg::int32()), From 289a7c961b33edac30d3da260235eef9396415f1 Mon Sep 17 00:00:00 2001 From: Xin Huang Date: Wed, 22 Jul 2026 11:24:27 -0700 Subject: [PATCH 4/4] fix(test): include arrow builder_nested for StructBuilder --- src/iceberg/test/avro_data_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/iceberg/test/avro_data_test.cc b/src/iceberg/test/avro_data_test.cc index 66678c8b4..46d940bb1 100644 --- a/src/iceberg/test/avro_data_test.cc +++ b/src/iceberg/test/avro_data_test.cc @@ -19,9 +19,12 @@ #include +#include #include #include #include +#include +#include #include #include #include