fix review

Author: HeartLinked

src/iceberg/inheritable_metadata.cc

@@ -22,8 +22,6 @@
 #include <cassert>
 #include <utility>
 
-#include <iceberg/result.h>
-
 #include "iceberg/manifest_entry.h"
 
 namespace iceberg {
@@ -36,7 +34,7 @@ BaseInheritableMetadata::BaseInheritableMetadata(int32_t spec_id, int64_t snapsh
       sequence_number_(sequence_number),
       manifest_location_(std::move(manifest_location)) {}
 
-Result<ManifestEntry> BaseInheritableMetadata::Apply(ManifestEntry entry) {
+Result<ManifestEntry> BaseInheritableMetadata::Apply(ManifestEntry& entry) {
   if (!entry.snapshot_id.has_value()) {
     entry.snapshot_id = snapshot_id_;
   }
@@ -64,14 +62,14 @@ Result<ManifestEntry> BaseInheritableMetadata::Apply(ManifestEntry entry) {
   return entry;
 }
 
-Result<ManifestEntry> EmptyInheritableMetadata::Apply(ManifestEntry entry) {
+Result<ManifestEntry> EmptyInheritableMetadata::Apply(ManifestEntry& entry) {
   return entry;
 }
 
 CopyInheritableMetadata::CopyInheritableMetadata(int64_t snapshot_id)
     : snapshot_id_(snapshot_id) {}
 
-Result<ManifestEntry> CopyInheritableMetadata::Apply(ManifestEntry entry) {
+Result<ManifestEntry> CopyInheritableMetadata::Apply(ManifestEntry& entry) {
   if (!entry.snapshot_id.has_value()) {
     entry.snapshot_id = snapshot_id_;
   }

src/iceberg/inheritable_metadata.h

@@ -44,9 +44,9 @@ class ICEBERG_EXPORT InheritableMetadata {
   virtual ~InheritableMetadata() = default;
 
   /// \brief Apply inheritable metadata to a manifest entry.
-  /// \param entry The manifest entry to modify.
+  /// \param entry The manifest entry to modify in-place.
   /// \return The modified manifest entry with inherited metadata applied.
-  virtual Result<ManifestEntry> Apply(ManifestEntry entry) = 0;
+  virtual Result<ManifestEntry> Apply(ManifestEntry& entry) = 0;
 };
 
 /// \brief Base implementation of InheritableMetadata that handles standard inheritance
@@ -62,9 +62,9 @@ class ICEBERG_EXPORT BaseInheritableMetadata : public InheritableMetadata {
                           std::string manifest_location);
 
   /// \brief Apply inheritance rules to a manifest entry.
-  /// \param entry The manifest entry to modify.
+  /// \param entry The manifest entry to modify in-place.
   /// \return The modified manifest entry.
-  Result<ManifestEntry> Apply(ManifestEntry entry) override;
+  Result<ManifestEntry> Apply(ManifestEntry& entry) override;
 
  private:
   int32_t spec_id_;
@@ -77,7 +77,9 @@ class ICEBERG_EXPORT BaseInheritableMetadata : public InheritableMetadata {
 class ICEBERG_EXPORT EmptyInheritableMetadata : public InheritableMetadata {
  public:
   /// \brief Apply no inheritance - returns the entry unchanged.
-  Result<ManifestEntry> Apply(ManifestEntry entry) override;
+  /// \param entry The manifest entry (unchanged).
+  /// \return The manifest entry.
+  Result<ManifestEntry> Apply(ManifestEntry& entry) override;
 };
 
 /// \brief Metadata inheritance for copying manifests before commit.
@@ -88,7 +90,9 @@ class ICEBERG_EXPORT CopyInheritableMetadata : public InheritableMetadata {
   explicit CopyInheritableMetadata(int64_t snapshot_id);
 
   /// \brief Apply copy inheritance rules.
-  Result<ManifestEntry> Apply(ManifestEntry entry) override;
+  /// \param entry The manifest entry to modify in-place.
+  /// \return The modified manifest entry.
+  Result<ManifestEntry> Apply(ManifestEntry& entry) override;
 
  private:
   int64_t snapshot_id_;

src/iceberg/manifest_reader.cc

@@ -19,7 +19,8 @@
 
 #include "iceberg/manifest_reader.h"
 
-#include "iceberg/file_reader.h"
+#include <iceberg/schema_internal.h>
+
 #include "iceberg/manifest_entry.h"
 #include "iceberg/manifest_list.h"
 #include "iceberg/manifest_reader_internal.h"
@@ -31,16 +32,10 @@ namespace iceberg {
 Result<std::unique_ptr<ManifestReader>> ManifestReader::Make(
     const ManifestFile& manifest, std::shared_ptr<FileIO> file_io,
     std::shared_ptr<Schema> partition_schema) {
-  // Validate manifest content type - only DATA manifests are supported by ManifestReader
-  if (manifest.content != ManifestFile::Content::kData) {
-    return InvalidArgument("Cannot read a delete manifest with a ManifestReader: {}",
-                           manifest.manifest_path);
-  }
-
   auto manifest_entry_schema = ManifestEntry::TypeFromPartitionType(partition_schema);
-  auto fields_span = manifest_entry_schema->fields();
-  std::vector<SchemaField> fields(fields_span.begin(), fields_span.end());
-  auto schema = std::make_shared<Schema>(fields);
+  auto schema_unique = FromStructType(std::move(*manifest_entry_schema), std::nullopt);
+  auto schema = std::shared_ptr<Schema>(std::move(schema_unique));
+
   ICEBERG_ASSIGN_OR_RAISE(
       auto reader,
       ReaderFactoryRegistry::Open(FileFormatType::kAvro, {.path = manifest.manifest_path,

src/iceberg/manifest_reader.h

@@ -25,7 +25,6 @@
 #include <memory>
 #include <vector>
 
-#include "iceberg/file_reader.h"
 #include "iceberg/iceberg_export.h"
 #include "iceberg/manifest_list.h"
 #include "iceberg/type_fwd.h"

src/iceberg/manifest_reader_internal.cc

@@ -19,8 +19,6 @@
 
 #include "manifest_reader_internal.h"
 
-#include <array>
-
 #include <nanoarrow/nanoarrow.h>
 
 #include "iceberg/arrow_c_data_guard_internal.h"
@@ -30,7 +28,6 @@
 #include "iceberg/schema.h"
 #include "iceberg/type.h"
 #include "iceberg/util/macros.h"
-#include "iceberg/util/visit_type.h"
 
 namespace iceberg {
 
@@ -547,7 +544,7 @@ Result<std::vector<ManifestEntry>> ManifestReaderImpl::Entries() const {
 
   // Apply inheritance to all entries
   for (auto& entry : manifest_entries) {
-    ICEBERG_ASSIGN_OR_RAISE(entry, inheritable_metadata_->Apply(std::move(entry)));
+    ICEBERG_ASSIGN_OR_RAISE(entry, inheritable_metadata_->Apply(entry));
   }
 
   return manifest_entries;

src/iceberg/manifest_reader_internal.h

@@ -38,8 +38,6 @@ class ManifestReaderImpl : public ManifestReader {
         reader_(std::move(reader)),
         inheritable_metadata_(std::move(inheritable_metadata)) {}
 
-  ManifestReaderImpl() = default;
-
   Result<std::vector<ManifestEntry>> Entries() const override;
 
  private:
@@ -54,7 +52,6 @@ class ManifestListReaderImpl : public ManifestListReader {
   explicit ManifestListReaderImpl(std::unique_ptr<Reader> reader,
                                   std::shared_ptr<Schema> schema)
       : schema_(std::move(schema)), reader_(std::move(reader)) {}
-  ManifestListReaderImpl() = default;
 
   Result<std::vector<ManifestFile>> Files() const override;