Skip to content

Commit 0961b90

Browse files
huan233uschuan233usc
committed
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.
1 parent 950f4dc commit 0961b90

7 files changed

Lines changed: 62 additions & 59 deletions

File tree

‎src/iceberg/arrow/literal_util.cc‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
#include <utility>
2323
#include <vector>
2424

25-
#include <arrow/array/builder_base.h>
2625
#include <arrow/array/util.h>
2726
#include <arrow/buffer.h>
2827
#include <arrow/compute/api.h>
@@ -194,27 +193,4 @@ Result<std::shared_ptr<::arrow::Array>> MakeDefaultArray(
194193
return array;
195194
}
196195

197-
Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder) {
198-
// The builder's own memory pool is not exposed, so the small scalar buffer uses the
199-
// default pool.
200-
ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr<::arrow::Scalar> scalar,
201-
ToArrowScalar(literal, ::arrow::default_memory_pool()));
202-
203-
// For an extension builder (e.g. `arrow.uuid`) target its storage type: ToArrowScalar
204-
// yields the storage scalar (fixed_size_binary(16) for uuid) and Scalar::CastTo has no
205-
// kernel that targets an extension type. This mirrors MakeDefaultArray's extension
206-
// handling.
207-
std::shared_ptr<::arrow::DataType> target_type = builder->type();
208-
if (target_type->id() == ::arrow::Type::EXTENSION) {
209-
target_type = internal::checked_cast<const ::arrow::ExtensionType&>(*target_type)
210-
.storage_type();
211-
}
212-
213-
if (!scalar->type->Equals(*target_type)) {
214-
ICEBERG_ARROW_ASSIGN_OR_RETURN(scalar, scalar->CastTo(target_type));
215-
}
216-
ICEBERG_ARROW_RETURN_NOT_OK(builder->AppendScalar(*scalar));
217-
return {};
218-
}
219-
220196
} // namespace iceberg::arrow

‎src/iceberg/arrow/literal_util_internal.h‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,4 @@ Result<std::shared_ptr<::arrow::Array>> MakeDefaultArray(
4444
const Literal& literal, const std::shared_ptr<::arrow::DataType>& type,
4545
int64_t num_rows, ::arrow::MemoryPool* pool);
4646

47-
/// \brief Append the literal value once to `builder`, e.g. to materialize a missing
48-
/// field with a default value while building rows one at a time.
49-
Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder);
50-
5147
} // namespace iceberg::arrow

‎src/iceberg/avro/avro_data_util.cc‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include <arrow/array/builder_primitive.h>
2424
#include <arrow/extension_type.h>
2525
#include <arrow/json/from_string.h>
26+
#include <arrow/scalar.h>
2627
#include <arrow/type.h>
2728
#include <arrow/util/decimal.h>
2829
#include <avro/Generic.hh>
@@ -90,7 +91,7 @@ Status AppendStructToBuilder(const ::avro::NodePtr& avro_node,
9091
} else if (field_projection.kind == FieldProjection::Kind::kNull) {
9192
ICEBERG_ARROW_RETURN_NOT_OK(field_builder->AppendNull());
9293
} else if (field_projection.kind == FieldProjection::Kind::kDefault) {
93-
ICEBERG_RETURN_UNEXPECTED(arrow::AppendDefaultToBuilder(
94+
ICEBERG_RETURN_UNEXPECTED(AppendDefaultToBuilder(
9495
std::get<Literal>(field_projection.from), field_builder));
9596
} else if (field_projection.kind == FieldProjection::Kind::kMetadata) {
9697
int32_t field_id = expected_field.field_id();
@@ -468,8 +469,7 @@ Status AppendFieldToBuilder(const ::avro::NodePtr& avro_node,
468469
}
469470

470471
if (projection.kind == FieldProjection::Kind::kDefault) {
471-
return arrow::AppendDefaultToBuilder(std::get<Literal>(projection.from),
472-
array_builder);
472+
return AppendDefaultToBuilder(std::get<Literal>(projection.from), array_builder);
473473
}
474474

475475
if (avro_node->type() == ::avro::AVRO_UNION) {
@@ -496,6 +496,29 @@ Status AppendFieldToBuilder(const ::avro::NodePtr& avro_node,
496496

497497
} // namespace
498498

499+
Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder) {
500+
// The builder's own memory pool is not exposed, so the small scalar buffer uses the
501+
// default pool.
502+
ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr<::arrow::Scalar> scalar,
503+
arrow::ToArrowScalar(literal, ::arrow::default_memory_pool()));
504+
505+
// For an extension builder (e.g. `arrow.uuid`) target its storage type: ToArrowScalar
506+
// yields the storage scalar (fixed_size_binary(16) for uuid) and Scalar::CastTo has no
507+
// kernel that targets an extension type. This mirrors MakeDefaultArray's extension
508+
// handling.
509+
std::shared_ptr<::arrow::DataType> target_type = builder->type();
510+
if (target_type->id() == ::arrow::Type::EXTENSION) {
511+
target_type = internal::checked_cast<const ::arrow::ExtensionType&>(*target_type)
512+
.storage_type();
513+
}
514+
515+
if (!scalar->type->Equals(*target_type)) {
516+
ICEBERG_ARROW_ASSIGN_OR_RETURN(scalar, scalar->CastTo(target_type));
517+
}
518+
ICEBERG_ARROW_RETURN_NOT_OK(builder->AppendScalar(*scalar));
519+
return {};
520+
}
521+
499522
Status AppendDatumToBuilder(const ::avro::NodePtr& avro_node,
500523
const ::avro::GenericDatum& avro_datum,
501524
const SchemaProjection& projection,

‎src/iceberg/avro/avro_data_util_internal.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,18 @@
2323
#include <avro/GenericDatum.hh>
2424

2525
#include "iceberg/arrow/metadata_column_util_internal.h"
26+
#include "iceberg/expression/literal.h"
2627
#include "iceberg/schema_util.h"
2728

2829
namespace iceberg::avro {
2930

31+
/// \brief Append a literal once to `builder` while decoding Avro row-by-row.
32+
///
33+
/// Used to materialize `FieldProjection::Kind::kDefault`. Shares `ToArrowScalar` with
34+
/// Parquet's batch path (`MakeDefaultArray`); the append shape stays Avro-local because
35+
/// Avro builds Arrow arrays via per-row `ArrayBuilder`s rather than whole-column arrays.
36+
Status AppendDefaultToBuilder(const Literal& literal, ::arrow::ArrayBuilder* builder);
37+
3038
/// \brief Append an Avro datum to an Arrow array builder.
3139
///
3240
/// This function handles schema evolution by using the provided projection to map

‎src/iceberg/avro/avro_direct_decoder.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
#include <avro/Types.hh>
3030

3131
#include "iceberg/arrow/arrow_status_internal.h"
32-
#include "iceberg/arrow/literal_util_internal.h"
32+
#include "iceberg/avro/avro_data_util_internal.h"
3333
#include "iceberg/avro/avro_direct_decoder_internal.h"
3434
#include "iceberg/avro/avro_schema_util_internal.h"
3535
#include "iceberg/metadata_columns.h"
@@ -211,7 +211,7 @@ Status DecodeStructToBuilder(const ::avro::NodePtr& avro_node, ::avro::Decoder&
211211
if (field_projection.kind == FieldProjection::Kind::kNull) {
212212
ICEBERG_ARROW_RETURN_NOT_OK(field_builder->AppendNull());
213213
} else if (field_projection.kind == FieldProjection::Kind::kDefault) {
214-
ICEBERG_RETURN_UNEXPECTED(arrow::AppendDefaultToBuilder(
214+
ICEBERG_RETURN_UNEXPECTED(AppendDefaultToBuilder(
215215
std::get<Literal>(field_projection.from), field_builder));
216216
} else if (field_projection.kind == FieldProjection::Kind::kMetadata) {
217217
int32_t field_id = expected_field.field_id();

‎src/iceberg/test/avro_data_test.cc‎

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

2020
#include <ranges>
2121

22+
#include <arrow/array/builder_primitive.h>
2223
#include <arrow/c/bridge.h>
2324
#include <arrow/json/from_string.h>
2425
#include <arrow/util/decimal.h>
@@ -700,6 +701,31 @@ TEST(AppendDatumToBuilderTest, StructWithMissingDefaultFields) {
700701
avro_data, expected_json));
701702
}
702703

704+
TEST(AppendDefaultToBuilderTest, AppendsValue) {
705+
::arrow::Int64Builder builder;
706+
ASSERT_THAT(AppendDefaultToBuilder(Literal::Long(42), &builder), IsOk());
707+
ASSERT_THAT(AppendDefaultToBuilder(Literal::Long(42), &builder), IsOk());
708+
709+
std::shared_ptr<::arrow::Array> array;
710+
ASSERT_TRUE(builder.Finish(&array).ok());
711+
ASSERT_EQ(array->length(), 2);
712+
const auto& long_array = static_cast<const ::arrow::Int64Array&>(*array);
713+
ASSERT_EQ(long_array.Value(0), 42);
714+
ASSERT_EQ(long_array.Value(1), 42);
715+
}
716+
717+
TEST(AppendDefaultToBuilderTest, CastsToBuilderType) {
718+
// The literal's natural type (int32) differs from the builder type (int64); the value
719+
// is cast to the builder type.
720+
::arrow::Int64Builder builder;
721+
ASSERT_THAT(AppendDefaultToBuilder(Literal::Int(7), &builder), IsOk());
722+
723+
std::shared_ptr<::arrow::Array> array;
724+
ASSERT_TRUE(builder.Finish(&array).ok());
725+
ASSERT_EQ(array->length(), 1);
726+
ASSERT_EQ(static_cast<const ::arrow::Int64Array&>(*array).Value(0), 7);
727+
}
728+
703729
TEST(AppendDatumToBuilderTest, NestedStructWithMissingOptionalFields) {
704730
Schema iceberg_schema({
705731
SchemaField::MakeRequired(1, "id", iceberg::int32()),

‎src/iceberg/test/literal_util_test.cc‎

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
#include <vector>
2626

2727
#include <arrow/array.h>
28-
#include <arrow/array/builder_primitive.h>
2928
#include <arrow/buffer.h>
3029
#include <arrow/extension/uuid.h>
3130
#include <arrow/scalar.h>
@@ -187,31 +186,6 @@ TEST(LiteralUtilTest, MakeDefaultArrayWrapsExtensionType) {
187186
}
188187
}
189188

190-
TEST(LiteralUtilTest, AppendDefaultToBuilderAppendsValue) {
191-
::arrow::Int64Builder builder;
192-
ASSERT_THAT(AppendDefaultToBuilder(Literal::Long(42), &builder), IsOk());
193-
ASSERT_THAT(AppendDefaultToBuilder(Literal::Long(42), &builder), IsOk());
194-
195-
std::shared_ptr<::arrow::Array> array;
196-
ASSERT_TRUE(builder.Finish(&array).ok());
197-
ASSERT_EQ(array->length(), 2);
198-
const auto& long_array = static_cast<const ::arrow::Int64Array&>(*array);
199-
ASSERT_EQ(long_array.Value(0), 42);
200-
ASSERT_EQ(long_array.Value(1), 42);
201-
}
202-
203-
TEST(LiteralUtilTest, AppendDefaultToBuilderCastsToBuilderType) {
204-
// The literal's natural type (int32) differs from the builder type (int64); the value
205-
// is cast to the builder type.
206-
::arrow::Int64Builder builder;
207-
ASSERT_THAT(AppendDefaultToBuilder(Literal::Int(7), &builder), IsOk());
208-
209-
std::shared_ptr<::arrow::Array> array;
210-
ASSERT_TRUE(builder.Finish(&array).ok());
211-
ASSERT_EQ(array->length(), 1);
212-
ASSERT_EQ(static_cast<const ::arrow::Int64Array&>(*array).Value(0), 7);
213-
}
214-
215189
} // namespace
216190

217191
} // namespace iceberg::arrow

0 commit comments

Comments
 (0)