attempt to fix some inconsistency and simplify logic

Author: wgtmac

src/iceberg/json_internal.cc

@@ -399,7 +399,7 @@ nlohmann::json ToJson(const Snapshot& snapshot) {
   json[kTimestampMs] = UnixMsFromTimePointMs(snapshot.timestamp_ms);
   json[kManifestList] = snapshot.manifest_list;
   // If there is an operation, write the summary map
-  if (snapshot.operation().has_value()) {
+  if (snapshot.Operation().has_value()) {
     json[kSummary] = snapshot.summary;
   }
   SetOptionalField(json, kSchemaId, snapshot.schema_id);

src/iceberg/snapshot.cc

@@ -19,6 +19,8 @@
 
 #include "iceberg/snapshot.h"
 
+#include <memory>
+
 #include "iceberg/file_io.h"
 #include "iceberg/manifest/manifest_list.h"
 #include "iceberg/manifest/manifest_reader.h"
@@ -50,67 +52,44 @@ SnapshotRefType SnapshotRef::type() const noexcept {
       retention);
 }
 
-Status SnapshotRef::MinSnapshotsToKeep(std::optional<int32_t> value) {
-  ICEBERG_PRECHECK(this->type() != SnapshotRefType::kTag,
-                   "Tags do not support setting minSnapshotsToKeep");
-  ICEBERG_PRECHECK(!value.has_value() || value.value() > 0,
-                   "Min snapshots to keep must be greater than 0");
-  std::get<Branch>(this->retention).min_snapshots_to_keep = value;
-  return {};
-}
-
-Status SnapshotRef::MaxSnapshotAgeMs(std::optional<int64_t> value) {
-  ICEBERG_PRECHECK(this->type() != SnapshotRefType::kTag,
-                   "Tags do not support setting maxSnapshotAgeMs");
-  ICEBERG_PRECHECK(!value.has_value() || value.value() > 0,
-                   "Max snapshot age must be greater than 0 ms");
-  std::get<Branch>(this->retention).max_snapshot_age_ms = value;
-  return {};
-}
-
-Status SnapshotRef::MaxRefAgeMs(std::optional<int64_t> value) {
-  ICEBERG_PRECHECK(!value.has_value() || value.value() > 0,
-                   "Max reference age must be greater than 0");
-  if (this->type() == SnapshotRefType::kBranch) {
-    std::get<Branch>(this->retention).max_ref_age_ms = value;
+Status SnapshotRef::Validate() const {
+  if (type() == SnapshotRefType::kBranch) {
+    const auto& branch = std::get<Branch>(this->retention);
+    ICEBERG_CHECK(!branch.min_snapshots_to_keep.has_value() ||
+                      branch.min_snapshots_to_keep.value() > 0,
+                  "Min snapshots to keep must be greater than 0");
+    ICEBERG_CHECK(
+        !branch.max_snapshot_age_ms.has_value() || branch.max_snapshot_age_ms.value() > 0,
+        "Max snapshot age must be greater than 0 ms");
+    ICEBERG_CHECK(!branch.max_ref_age_ms.has_value() || branch.max_ref_age_ms.value() > 0,
+                  "Max reference age must be greater than 0");
   } else {
-    std::get<Tag>(this->retention).max_ref_age_ms = value;
+    const auto& tag = std::get<Tag>(this->retention);
+    ICEBERG_CHECK(!tag.max_ref_age_ms.has_value() || tag.max_ref_age_ms.value() > 0,
+                  "Max reference age must be greater than 0");
   }
   return {};
 }
 
 Result<std::unique_ptr<SnapshotRef>> SnapshotRef::MakeBranch(
     int64_t snapshot_id, std::optional<int32_t> min_snapshots_to_keep,
     std::optional<int64_t> max_snapshot_age_ms, std::optional<int64_t> max_ref_age_ms) {
-  // Validate optional parameters
-  if (min_snapshots_to_keep.has_value() && min_snapshots_to_keep.value() <= 0) {
-    return InvalidArgument("Min snapshots to keep must be greater than 0");
-  }
-  if (max_snapshot_age_ms.has_value() && max_snapshot_age_ms.value() <= 0) {
-    return InvalidArgument("Max snapshot age must be greater than 0 ms");
-  }
-  if (max_ref_age_ms.has_value() && max_ref_age_ms.value() <= 0) {
-    return InvalidArgument("Max reference age must be greater than 0");
-  }
-
-  auto ref = std::make_unique<SnapshotRef>();
-  ref->snapshot_id = snapshot_id;
-  ref->retention = Branch{.min_snapshots_to_keep = min_snapshots_to_keep,
-                          .max_snapshot_age_ms = max_snapshot_age_ms,
-                          .max_ref_age_ms = max_ref_age_ms};
+  auto ref = std::make_unique<SnapshotRef>(
+      SnapshotRef{.snapshot_id = snapshot_id,
+                  .retention = Branch{
+                      .min_snapshots_to_keep = min_snapshots_to_keep,
+                      .max_snapshot_age_ms = max_snapshot_age_ms,
+                      .max_ref_age_ms = max_ref_age_ms,
+                  }});
+  ICEBERG_RETURN_UNEXPECTED(ref->Validate());
   return ref;
 }
 
 Result<std::unique_ptr<SnapshotRef>> SnapshotRef::MakeTag(
     int64_t snapshot_id, std::optional<int64_t> max_ref_age_ms) {
-  // Validate optional parameter
-  if (max_ref_age_ms.has_value() && max_ref_age_ms.value() <= 0) {
-    return InvalidArgument("Max reference age must be greater than 0");
-  }
-
-  auto ref = std::make_unique<SnapshotRef>();
-  ref->snapshot_id = snapshot_id;
-  ref->retention = Tag{.max_ref_age_ms = max_ref_age_ms};
+  auto ref = std::make_unique<SnapshotRef>(SnapshotRef{
+      .snapshot_id = snapshot_id, .retention = Tag{.max_ref_age_ms = max_ref_age_ms}});
+  ICEBERG_RETURN_UNEXPECTED(ref->Validate());
   return ref;
 }
 
@@ -140,7 +119,7 @@ bool SnapshotRef::Equals(const SnapshotRef& other) const {
   }
 }
 
-std::optional<std::string_view> Snapshot::operation() const {
+std::optional<std::string_view> Snapshot::Operation() const {
   auto it = summary.find(SnapshotSummaryFields::kOperation);
   if (it != summary.end()) {
     return it->second;
@@ -176,6 +155,37 @@ bool Snapshot::Equals(const Snapshot& other) const {
          schema_id == other.schema_id;
 }
 
+Result<std::unique_ptr<Snapshot>> Snapshot::Make(
+    int64_t sequence_number, int64_t snapshot_id,
+    std::optional<int64_t> parent_snapshot_id, TimePointMs timestamp_ms,
+    std::string operation, std::unordered_map<std::string, std::string> summary,
+    std::optional<int32_t> schema_id, std::string manifest_list,
+    std::optional<int64_t> first_row_id, std::optional<int64_t> added_rows) {
+  ICEBERG_PRECHECK(!operation.empty(), "Operation cannot be empty");
+  ICEBERG_PRECHECK(!first_row_id.has_value() || first_row_id.value() >= 0,
+                   "Invalid first-row-id (cannot be negative): {}", first_row_id.value());
+  ICEBERG_PRECHECK(!added_rows.has_value() || added_rows.value() >= 0,
+                   "Invalid added-rows (cannot be negative): {}", added_rows.value());
+  ICEBERG_PRECHECK(!first_row_id.has_value() || added_rows.has_value(),
+                   "Missing added-rows when first-row-id is set");
+  summary[SnapshotSummaryFields::kOperation] = operation;
+  if (first_row_id.has_value()) {
+    summary[SnapshotSummaryFields::kFirstRowId] = std::to_string(first_row_id.value());
+  }
+  if (added_rows.has_value()) {
+    summary[SnapshotSummaryFields::kAddedRows] = std::to_string(added_rows.value());
+  }
+  return std::make_unique<Snapshot>(Snapshot{
+      .snapshot_id = snapshot_id,
+      .parent_snapshot_id = parent_snapshot_id,
+      .sequence_number = sequence_number,
+      .timestamp_ms = timestamp_ms,
+      .manifest_list = std::move(manifest_list),
+      .summary = std::move(summary),
+      .schema_id = schema_id,
+  });
+}
+
 Result<SnapshotCache::ManifestsCache> SnapshotCache::InitManifestsCache(
     const Snapshot* snapshot, std::shared_ptr<FileIO> file_io) {
   if (file_io == nullptr) {

src/iceberg/snapshot.h

@@ -113,21 +113,6 @@ struct ICEBERG_EXPORT SnapshotRef {
 
   SnapshotRefType type() const noexcept;
 
-  /// \brief Set the minimum number of snapshots to keep (branch only)
-  /// \param value The minimum number of snapshots to keep, or nullopt for default
-  /// \return Status indicating success or failure
-  Status MinSnapshotsToKeep(std::optional<int32_t> value);
-
-  /// \brief Set the maximum snapshot age in milliseconds (branch only)
-  /// \param value The maximum snapshot age in milliseconds, or nullopt for default
-  /// \return Status indicating success or failure
-  Status MaxSnapshotAgeMs(std::optional<int64_t> value);
-
-  /// \brief Set the maximum reference age in milliseconds
-  /// \param value The maximum reference age in milliseconds, or nullopt for default
-  /// \return Status indicating success or failure
-  Status MaxRefAgeMs(std::optional<int64_t> value);
-
   /// \brief Create a branch reference
   ///
   /// \param snapshot_id The snapshot ID for the branch
@@ -158,6 +143,9 @@ struct ICEBERG_EXPORT SnapshotRef {
   std::unique_ptr<SnapshotRef> Clone(
       std::optional<int64_t> new_snapshot_id = std::nullopt) const;
 
+  /// \brief Validate the SnapshotRef
+  Status Validate() const;
+
   /// \brief Compare two snapshot refs for equality
   friend bool operator==(const SnapshotRef& lhs, const SnapshotRef& rhs) {
     return lhs.Equals(rhs);
@@ -169,13 +157,11 @@ struct ICEBERG_EXPORT SnapshotRef {
 };
 
 /// \brief Optional Snapshot Summary Fields
-struct SnapshotSummaryFields {
+struct ICEBERG_EXPORT SnapshotSummaryFields {
   /// \brief The operation field key
   inline static const std::string kOperation = "operation";
-
   /// \brief The first row id field key
   inline static const std::string kFirstRowId = "first-row-id";
-
   /// \brief The added rows field key
   inline static const std::string kAddedRows = "added-rows";
 
@@ -296,12 +282,21 @@ struct ICEBERG_EXPORT Snapshot {
   /// ID of the table's current schema when the snapshot was created.
   std::optional<int32_t> schema_id;
 
+  /// \brief Create a new Snapshot instance with validation on the inputs.
+  static Result<std::unique_ptr<Snapshot>> Make(
+      int64_t sequence_number, int64_t snapshot_id,
+      std::optional<int64_t> parent_snapshot_id, TimePointMs timestamp_ms,
+      std::string operation, std::unordered_map<std::string, std::string> summary,
+      std::optional<int32_t> schema_id, std::string manifest_list,
+      std::optional<int64_t> first_row_id = std::nullopt,
+      std::optional<int64_t> added_rows = std::nullopt);
+
   /// \brief Return the name of the DataOperations data operation that produced this
   /// snapshot.
   ///
   /// \return the operation that produced this snapshot, or nullopt if the operation is
   /// unknown.
-  std::optional<std::string_view> operation() const;
+  std::optional<std::string_view> Operation() const;
 
   /// \brief The row-id of the first newly added row in this snapshot.
   ///