refactor: simplify Schema and StructType constructors and remove copy/move operations

nullccxsy · nullccxsy · commit 4f5bd35d2f78 · 2025-09-16T00:05:34.000+08:00
- Removed copy and move constructors and assignment operators from Schema and StructType to streamline resource management.
diff --git a/src/iceberg/manifest_list.h b/src/iceberg/manifest_list.h
@@ -185,7 +185,9 @@ struct ICEBERG_EXPORT ManifestFile {
       507, "partitions",
       std::make_shared<ListType>(SchemaField::MakeRequired(
           508, std::string(ListType::kElementName),
-          std::make_shared<StructType>(PartitionFieldSummary::Type()))),
+          struct_(
+              {PartitionFieldSummary::kContainsNull, PartitionFieldSummary::kContainsNaN,
+               PartitionFieldSummary::kLowerBound, PartitionFieldSummary::kUpperBound}))),
       "Summary for each partition");
   inline static const SchemaField kKeyMetadata = SchemaField::MakeOptional(
       519, "key_metadata", iceberg::binary(), "Encryption key metadata blob");
diff --git a/src/iceberg/schema.cc b/src/iceberg/schema.cc
@@ -71,42 +71,6 @@ class NameToIdVisitor {
 Schema::Schema(std::vector<SchemaField> fields, std::optional<int32_t> schema_id)
     : StructType(std::move(fields)), schema_id_(schema_id) {}
 
-Schema::Schema(Schema&& other) noexcept
-    : StructType(std::move(other)),
-      schema_id_(other.schema_id_),
-      id_to_field_(std::move(other.id_to_field_)),
-      name_to_id_(std::move(other.name_to_id_)),
-      lowercase_name_to_id_(std::move(other.lowercase_name_to_id_)) {}
-
-Schema& Schema::operator=(const Schema& other) {
-  if (this != &other) {
-    StructType::operator=(other);
-    schema_id_ = other.schema_id_;
-    id_to_field_ = other.id_to_field_;
-    name_to_id_ = other.name_to_id_;
-    lowercase_name_to_id_ = other.lowercase_name_to_id_;
-  }
-  return *this;
-}
-
-Schema::Schema(const Schema& other)
-    : StructType(other),
-      schema_id_(other.schema_id_),
-      id_to_field_(other.id_to_field_),
-      name_to_id_(other.name_to_id_),
-      lowercase_name_to_id_(other.lowercase_name_to_id_) {}
-
-Schema& Schema::operator=(Schema&& other) noexcept {
-  if (this != &other) {
-    StructType::operator=(std::move(other));
-    schema_id_ = other.schema_id_;
-    id_to_field_ = std::move(other.id_to_field_);
-    name_to_id_ = std::move(other.name_to_id_);
-    lowercase_name_to_id_ = std::move(other.lowercase_name_to_id_);
-  }
-  return *this;
-}
-
 std::optional<int32_t> Schema::schema_id() const { return schema_id_; }
 
 std::string Schema::ToString() const {
diff --git a/src/iceberg/schema.h b/src/iceberg/schema.h
@@ -49,10 +49,6 @@ class ICEBERG_EXPORT Schema : public StructType {
   explicit Schema(std::vector<SchemaField> fields,
                   std::optional<int32_t> schema_id = std::nullopt);
 
-  Schema(const Schema& other);
-  Schema(Schema&& other) noexcept;
-  Schema& operator=(const Schema& other);
-  Schema& operator=(Schema&& other) noexcept;
   /// \brief Get the schema ID.
   ///
   /// A schema is identified by a unique ID for the purposes of schema
diff --git a/src/iceberg/type.cc b/src/iceberg/type.cc
@@ -48,34 +48,6 @@ std::string StructType::ToString() const {
   return repr;
 }
 std::span<const SchemaField> StructType::fields() const { return fields_; }
-StructType::StructType(const StructType& other)
-    : fields_(other.fields_),
-      field_by_id_(other.field_by_id_),
-      field_by_name_(other.field_by_name_),
-      field_by_lowercase_name_(other.field_by_lowercase_name_) {}
-StructType::StructType(StructType&& other) noexcept
-    : fields_(std::move(other.fields_)),
-      field_by_id_(std::move(other.field_by_id_)),
-      field_by_name_(std::move(other.field_by_name_)),
-      field_by_lowercase_name_(std::move(other.field_by_lowercase_name_)) {}
-StructType& StructType::operator=(const StructType& other) {
-  if (*this != other) {
-    fields_ = other.fields_;
-    field_by_id_ = other.field_by_id_;
-    field_by_name_ = other.field_by_name_;
-    field_by_lowercase_name_ = other.field_by_lowercase_name_;
-  }
-  return *this;
-}
-StructType& StructType::operator=(StructType&& other) noexcept {
-  if (*this != other) {
-    fields_ = std::move(other.fields_);
-    field_by_id_ = std::move(other.field_by_id_);
-    field_by_name_ = std::move(other.field_by_name_);
-    field_by_lowercase_name_ = std::move(other.field_by_lowercase_name_);
-  }
-  return *this;
-}
 Result<std::optional<NestedType::SchemaFieldConstRef>> StructType::GetFieldById(
     int32_t field_id) const {
   ICEBERG_RETURN_UNEXPECTED(
diff --git a/src/iceberg/type.h b/src/iceberg/type.h
@@ -117,10 +117,6 @@ class ICEBERG_EXPORT StructType : public NestedType {
   constexpr static TypeId kTypeId = TypeId::kStruct;
   explicit StructType(std::vector<SchemaField> fields);
   ~StructType() override = default;
-  StructType(const StructType&);
-  StructType(StructType&&) noexcept;
-  StructType& operator=(const StructType&);
-  StructType& operator=(StructType&&) noexcept;
 
   TypeId type_id() const override;
   std::string ToString() const override;
diff --git a/test/avro_data_test.cc b/test/avro_data_test.cc
@@ -1195,16 +1195,16 @@ TEST(ExtractDatumFromArrayTest, NullHandling) {
 
 struct RoundTripParam {
   std::string name;
-  Schema iceberg_schema;
+  std::shared_ptr<Schema> iceberg_schema;
   std::string arrow_json;
 };
 
 void VerifyRoundTripConversion(const RoundTripParam& test_case) {
   ::avro::NodePtr avro_node;
-  ASSERT_THAT(ToAvroNodeVisitor{}.Visit(test_case.iceberg_schema, &avro_node), IsOk());
+  ASSERT_THAT(ToAvroNodeVisitor{}.Visit(*test_case.iceberg_schema, &avro_node), IsOk());
 
   ArrowSchema arrow_c_schema;
-  ASSERT_THAT(ToArrowSchema(test_case.iceberg_schema, &arrow_c_schema), IsOk());
+  ASSERT_THAT(ToArrowSchema(*test_case.iceberg_schema, &arrow_c_schema), IsOk());
   auto arrow_schema = ::arrow::ImportSchema(&arrow_c_schema).ValueOrDie();
   auto arrow_struct_type = std::make_shared<::arrow::StructType>(arrow_schema->fields());
 
@@ -1221,14 +1221,14 @@ void VerifyRoundTripConversion(const RoundTripParam& test_case) {
   }
 
   auto projection_result =
-      Project(test_case.iceberg_schema, avro_node, /*prune_source=*/false);
+      Project(*test_case.iceberg_schema, avro_node, /*prune_source=*/false);
   ASSERT_THAT(projection_result, IsOk());
   auto projection = std::move(projection_result.value());
 
   auto builder = ::arrow::MakeBuilder(arrow_struct_type).ValueOrDie();
   for (const auto& datum : extracted_data) {
     ASSERT_THAT(AppendDatumToBuilder(avro_node, datum, projection,
-                                     test_case.iceberg_schema, builder.get()),
+                                     *test_case.iceberg_schema, builder.get()),
                 IsOk());
   }
 
@@ -1249,7 +1249,7 @@ TEST_P(AvroRoundTripConversionTest, ConvertTypes) {
 const std::vector<RoundTripParam> kRoundTripTestCases = {
     {
         .name = "SimpleStruct",
-        .iceberg_schema = Schema({
+        .iceberg_schema = std::make_shared<Schema>(std::vector<SchemaField>{
             SchemaField::MakeRequired(1, "id", int32()),
             SchemaField::MakeRequired(2, "name", string()),
             SchemaField::MakeOptional(3, "age", int32()),
@@ -1262,7 +1262,7 @@ const std::vector<RoundTripParam> kRoundTripTestCases = {
     },
     {
         .name = "PrimitiveTypes",
-        .iceberg_schema = Schema({
+        .iceberg_schema = std::make_shared<Schema>(std::vector<SchemaField>{
             SchemaField::MakeRequired(1, "bool_field", boolean()),
             SchemaField::MakeRequired(2, "int_field", int32()),
             SchemaField::MakeRequired(3, "long_field", int64()),
@@ -1277,7 +1277,7 @@ const std::vector<RoundTripParam> kRoundTripTestCases = {
     },
     {
         .name = "NestedStruct",
-        .iceberg_schema = Schema({
+        .iceberg_schema = std::make_shared<Schema>(std::vector<SchemaField>{
             SchemaField::MakeRequired(1, "id", int32()),
             SchemaField::MakeRequired(
                 2, "person",
@@ -1293,7 +1293,7 @@ const std::vector<RoundTripParam> kRoundTripTestCases = {
     },
     {
         .name = "ListOfIntegers",
-        .iceberg_schema = Schema({
+        .iceberg_schema = std::make_shared<Schema>(std::vector<SchemaField>{
             SchemaField::MakeRequired(
                 1, "numbers",
                 std::make_shared<ListType>(
@@ -1307,7 +1307,7 @@ const std::vector<RoundTripParam> kRoundTripTestCases = {
     },
     {
         .name = "MapStringToInt",
-        .iceberg_schema = Schema({
+        .iceberg_schema = std::make_shared<Schema>(std::vector<SchemaField>{
             SchemaField::MakeRequired(
                 1, "scores",
                 std::make_shared<MapType>(
@@ -1322,7 +1322,7 @@ const std::vector<RoundTripParam> kRoundTripTestCases = {
     },
     {
         .name = "ComplexNested",
-        .iceberg_schema = Schema({
+        .iceberg_schema = std::make_shared<Schema>(std::vector<SchemaField>{
             SchemaField::MakeRequired(
                 1, "data",
                 std::make_shared<StructType>(std::vector<SchemaField>{
@@ -1345,7 +1345,7 @@ const std::vector<RoundTripParam> kRoundTripTestCases = {
     },
     {
         .name = "NullablePrimitives",
-        .iceberg_schema = Schema({
+        .iceberg_schema = std::make_shared<Schema>(std::vector<SchemaField>{
             SchemaField::MakeOptional(1, "optional_bool", boolean()),
             SchemaField::MakeOptional(2, "optional_int", int32()),
             SchemaField::MakeOptional(3, "optional_long", int64()),
@@ -1361,7 +1361,7 @@ const std::vector<RoundTripParam> kRoundTripTestCases = {
     },
     {
         .name = "NullableNestedStruct",
-        .iceberg_schema = Schema({
+        .iceberg_schema = std::make_shared<Schema>(std::vector<SchemaField>{
             SchemaField::MakeRequired(1, "id", int32()),
             SchemaField::MakeOptional(
                 2, "person",
@@ -1381,7 +1381,7 @@ const std::vector<RoundTripParam> kRoundTripTestCases = {
     },
     {
         .name = "NullableListElements",
-        .iceberg_schema = Schema({
+        .iceberg_schema = std::make_shared<Schema>(std::vector<SchemaField>{
             SchemaField::MakeRequired(1, "id", int32()),
             SchemaField::MakeOptional(
                 2, "numbers",
@@ -1401,7 +1401,7 @@ const std::vector<RoundTripParam> kRoundTripTestCases = {
     },
     {
         .name = "NullableMapValues",
-        .iceberg_schema = Schema({
+        .iceberg_schema = std::make_shared<Schema>(std::vector<SchemaField>{
             SchemaField::MakeRequired(1, "id", int32()),
             SchemaField::MakeOptional(
                 2, "scores",
@@ -1423,7 +1423,7 @@ const std::vector<RoundTripParam> kRoundTripTestCases = {
     },
     {
         .name = "DeeplyNestedWithNulls",
-        .iceberg_schema = Schema({
+        .iceberg_schema = std::make_shared<Schema>(std::vector<SchemaField>{
             SchemaField::MakeRequired(
                 1, "root",
                 std::make_shared<StructType>(std::vector<SchemaField>{
@@ -1452,7 +1452,7 @@ const std::vector<RoundTripParam> kRoundTripTestCases = {
     },
     {
         .name = "AllNullsVariations",
-        .iceberg_schema = Schema({
+        .iceberg_schema = std::make_shared<Schema>(std::vector<SchemaField>{
             SchemaField::MakeOptional(1, "always_null", string()),
             SchemaField::MakeOptional(2, "sometimes_null", int32()),
             SchemaField::MakeOptional(
diff --git a/test/schema_test.cc b/test/schema_test.cc
@@ -559,26 +559,3 @@ TEST_F(SchemaThreadSafetyTest, MixedConcurrentOperations) {
     thread.join();
   }
 }
-
-TEST_F(SchemaThreadSafetyTest, CopyAndConcurrentAccess) {
-  const int num_threads = 5;
-  const int iterations_per_thread = 20;
-  std::vector<std::thread> threads;
-  auto schema_copy = *schema_;
-
-  for (int i = 0; i < num_threads; ++i) {
-    threads.emplace_back([this, &schema_copy, iterations_per_thread, i]() {
-      for (int j = 0; j < iterations_per_thread; ++j) {
-        if (i % 2 == 0) {
-          ASSERT_THAT(schema_->FindFieldById(1), ::testing::Optional(*field1_));
-        } else {
-          ASSERT_THAT(schema_copy.FindFieldById(1), ::testing::Optional(*field1_));
-        }
-      }
-    });
-  }
-
-  for (auto& thread : threads) {
-    thread.join();
-  }
-}
diff --git a/test/type_test.cc b/test/type_test.cc
@@ -604,27 +604,3 @@ TEST_F(StructTypeThreadSafetyTest, MixedConcurrentOperations) {
     thread.join();
   }
 }
-
-TEST_F(StructTypeThreadSafetyTest, CopyAndConcurrentAccess) {
-  const int num_threads = 5;
-  const int iterations_per_thread = 20;
-  std::vector<std::thread> threads;
-
-  auto struct_copy = *struct_type_;
-
-  for (int i = 0; i < num_threads; ++i) {
-    threads.emplace_back([this, &struct_copy, iterations_per_thread, i]() {
-      for (int j = 0; j < iterations_per_thread; ++j) {
-        if (i % 2 == 0) {
-          ASSERT_THAT(struct_type_->GetFieldById(1), ::testing::Optional(*field1_));
-        } else {
-          ASSERT_THAT(struct_copy.GetFieldById(1), ::testing::Optional(*field1_));
-        }
-      }
-    });
-  }
-
-  for (auto& thread : threads) {
-    thread.join();
-  }
-}
