resolve more review comments

Author: zhjwpku

src/iceberg/snapshot.cc

@@ -25,98 +25,38 @@
 
 namespace iceberg {
 
-namespace {
-/// \brief Get the relative Operation name
-constexpr std::string_view ToString(Summary::Operation operation) {
-  switch (operation) {
-    case Summary::Operation::kAppend:
-      return "append";
-    case Summary::Operation::kOverwrite:
-      return "overwrite";
-    case Summary::Operation::kReplace:
-      return "replace";
-    case Summary::Operation::kDelete:
-      return "delete";
-    default:
-      return "invalid";
+std::optional<std::string> Snapshot::operation() const {
+  auto it = summary.find(std::string(SnapshotSummaryFields::kOperation));
+  if (it != summary.end()) {
+    return it->second;
   }
+  return std::nullopt;
 }
-}  // namespace
 
-Summary::Summary(Operation op, std::unordered_map<std::string, std::string> props)
-    : operation_(op), additional_properties_(std::move(props)) {}
-
-Summary::Operation Summary::operation() const { return operation_; }
-
-const std::unordered_map<std::string, std::string>& Summary::properties() const {
-  return additional_properties_;
-}
-
-std::string Summary::ToString() const {
-  std::string repr = std::format("summary<operation: {}", iceberg::ToString(operation_));
-  for (const auto& [key, value] : additional_properties_) {
-    std::format_to(std::back_inserter(repr), ", {}: {}", key, value);
+std::optional<std::reference_wrapper<const Snapshot::manifest_list_t>>
+Snapshot::ManifestList() const {
+  if (std::holds_alternative<manifest_list_t>(manifest_list)) {
+    return std::cref(std::get<manifest_list_t>(manifest_list));
   }
-  repr += ">";
-  return repr;
-}
-
-Snapshot::Snapshot(int64_t snapshot_id, std::optional<int64_t> parent_snapshot_id,
-                   int64_t sequence_number, int64_t timestamp_ms,
-                   std::string manifest_list, Summary summary,
-                   std::optional<int64_t> schema_id)
-    : 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) {}
-
-int64_t Snapshot::snapshot_id() const { return snapshot_id_; }
-
-std::optional<int64_t> Snapshot::parent_snapshot_id() const {
-  return parent_snapshot_id_;
+  return std::nullopt;
 }
 
-int64_t Snapshot::sequence_number() const { return sequence_number_; }
-
-int64_t Snapshot::timestamp_ms() const { return timestamp_ms_; }
-
-const std::string& Snapshot::manifest_list() const { return manifest_list_; }
-
-const Summary& Snapshot::summary() const { return summary_; }
-
-std::optional<int32_t> Snapshot::schema_id() const { return schema_id_; }
-
-std::string Snapshot::ToString() const {
-  std::string repr;
-  std::format_to(std::back_inserter(repr), "snapshot<\n  id: {}\n", snapshot_id_);
-  if (parent_snapshot_id_.has_value()) {
-    std::format_to(std::back_inserter(repr), "  parent_id: {}\n",
-                   parent_snapshot_id_.value());
-  }
-  std::format_to(std::back_inserter(repr), "  sequence_number: {}\n", sequence_number_);
-  std::format_to(std::back_inserter(repr), "  timestamp_ms: {}\n", timestamp_ms_);
-  std::format_to(std::back_inserter(repr), "  manifest_list: {}\n", manifest_list_);
-  std::format_to(std::back_inserter(repr), "  summary: {}\n", summary_);
-
-  if (schema_id_.has_value()) {
-    std::format_to(std::back_inserter(repr), "  schema_id: {}\n", schema_id_.value());
+std::optional<std::reference_wrapper<const Snapshot::manifests_t>> Snapshot::Manifests()
+    const {
+  if (std::holds_alternative<manifests_t>(manifest_list)) {
+    return std::cref(std::get<manifests_t>(manifest_list));
   }
-
-  repr += ">";
-  return repr;
+  return std::nullopt;
 }
 
 bool Snapshot::Equals(const Snapshot& other) const {
   if (this == &other) {
     return true;
   }
-  return snapshot_id_ == other.snapshot_id_ &&
-         parent_snapshot_id_ == other.parent_snapshot_id_ &&
-         sequence_number_ == other.sequence_number_ &&
-         timestamp_ms_ == other.timestamp_ms_ && schema_id_ == other.schema_id_;
+  return snapshot_id == other.snapshot_id &&
+         parent_snapshot_id == other.parent_snapshot_id &&
+         sequence_number == other.sequence_number && timestamp_ms == other.timestamp_ms &&
+         schema_id == other.schema_id;
 }
 
 }  // namespace iceberg

src/iceberg/snapshot.h

@@ -22,12 +22,44 @@
 #include <optional>
 #include <string>
 #include <unordered_map>
+#include <variant>
+#include <vector>
 
 #include "iceberg/iceberg_export.h"
 #include "iceberg/util/formattable.h"
 
 namespace iceberg {
 
+/// \brief The type of snapshot reference
+enum class SnapshotRefType {
+  /// Branches are mutable named references that can be updated by committing a new
+  /// snapshot as the branch’s referenced snapshot using the Commit Conflict Resolution
+  /// and Retry procedures.
+  kBranch,
+  /// Tags are labels for individual snapshots
+  kTag,
+};
+
+/// \brief A reference to a snapshot, either a branch or a tag.
+struct ICEBERG_EXPORT SnapshotRef {
+  /// A reference's snapshot ID. The tagged snapshot or latest snapshot of a branch.
+  int64_t snapshot_id;
+  /// Type of the reference, tag or branch
+  SnapshotRefType type;
+  /// For branch type only, a positive number for the minimum number of snapshots to keep
+  /// in a branch while expiring snapshots. Defaults to table property
+  /// history.expire.min-snapshots-to-keep.
+  std::optional<int32_t> min_snapshots_to_keep;
+  /// For branch type only, a positive number for the max age of snapshots to keep when
+  /// expiring, including the latest snapshot. Defaults to table property
+  /// history.expire.max-snapshot-age-ms.
+  std::optional<int64_t> max_snapshot_age_ms;
+  /// For snapshot references except the main branch, a positive number for the max age of
+  /// the snapshot reference to keep while expiring snapshots. Defaults to table property
+  /// history.expire.max-ref-age-ms. The main branch never expires.
+  std::optional<int64_t> max_ref_age_ms;
+};
+
 /// \brief Optional Snapshot Summary Fields
 struct SnapshotSummaryFields {
   /// \brief The operation field key
@@ -109,40 +141,23 @@ struct SnapshotSummaryFields {
   constexpr static std::string_view kEngineVersion = "engine-version";
 };
 
-/// \brief Summarises the changes in the snapshot.
-class ICEBERG_EXPORT Summary : public iceberg::util::Formattable {
- public:
-  /// \brief The operation field is used by some operations, like snapshot expiration, to
-  /// skip processing certain snapshots.
-  enum class Operation {
-    /// Only data files were added and no files were removed.
-    kAppend,
-    /// Data and delete files were added and removed without changing table data; i.e.
-    /// compaction, change the data file format, or relocating data files.
-    kReplace,
-    /// Data and delete files were added and removed in a logical overwrite operation.
-    kOverwrite,
-    /// Data files were removed and their contents logically deleted and/or delete files
-    /// were added to delete rows.
-    kDelete,
-  };
-  Summary() = default;
-  /// \brief Construct a summary with the given operation and properties.
-  Summary(Operation op, std::unordered_map<std::string, std::string> props);
-
-  /// \brief Get the operation type of the snapshot.
-  Operation operation() const;
-
-  /// \brief Get the additional properties of the snapshot.
-  const std::unordered_map<std::string, std::string>& properties() const;
-
-  std::string ToString() const override;
-
- private:
-  /// The type of operation in the snapshot
-  Operation operation_{Operation::kAppend};
-  /// Other summary data.
-  std::unordered_map<std::string, std::string> additional_properties_;
+/// \brief Data operation that produce snapshots.
+///
+/// A snapshot can return the operation that created the snapshot to help other components
+/// ignore snapshots that are not needed for some tasks. For example, snapshot expiration
+/// does not need to clean up deleted files for appends, which have no deleted files.
+struct ICEBERG_EXPORT DataOperation {
+  /// \brief Only data files were added and no files were removed.
+  static constexpr std::string_view kAppend = "append";
+  /// \brief Data and delete files were added and removed without changing table data;
+  /// i.e. compaction, change the data file format, or relocating data files.
+  static constexpr std::string_view kReplace = "replace";
+  /// \brief Data and delete files were added and removed in a logical overwrite
+  /// operation.
+  static constexpr std::string_view kOverwrite = "overwrite";
+  /// \brief Data files were removed and their contents logically deleted and/or delete
+  /// files were added to delete rows.
+  static constexpr std::string_view kDelete = "delete";
 };
 
 /// \brief A snapshot of the data in a table at a point in time.
@@ -151,63 +166,59 @@ class ICEBERG_EXPORT Summary : public iceberg::util::Formattable {
 /// the union of all the data files in those manifests.
 ///
 /// Snapshots are created by table operations.
-class ICEBERG_EXPORT Snapshot : public iceberg::util::Formattable {
- public:
-  Snapshot(int64_t snapshot_id, std::optional<int64_t> parent_snapshot_id,
-           int64_t sequence_number, int64_t timestamp_ms, std::string manifest_list,
-           Summary summary, std::optional<int64_t> schema_id);
-
-  /// \brief Get the id of the snapshot.
-  int64_t snapshot_id() const;
-
-  /// \brief Get parent snapshot id.
-  std::optional<int64_t> parent_snapshot_id() const;
+struct ICEBERG_EXPORT Snapshot {
+  using manifest_list_t = std::string;
+  using manifests_t = std::vector<std::string>;
 
-  /// \brief Get the sequence number of the snapshot.
-  int64_t sequence_number() const;
-
-  /// \brief Get the timestamp of the snapshot.
-  int64_t timestamp_ms() const;
-
-  /// \brief Get the manifest list of the snapshot.
-  const std::string& manifest_list() const;
-
-  /// \brief Get the summary of the snapshot.
-  const Summary& summary() const;
-
-  /// \brief Get the schema ID of the snapshot.
-  std::optional<int32_t> schema_id() const;
-
-  std::string ToString() const override;
+  /// A unqiue long ID.
+  int64_t snapshot_id;
+  /// The snapshot ID of the snapshot's parent. Omitted for any snapshot with no parent.
+  std::optional<int64_t> parent_snapshot_id;
+  /// A monotonically increasing long that tracks the order of changes to a table.
+  int64_t sequence_number;
+  /// A timestamp when the snapshot was created, used for garbage collection and table
+  /// inspection.
+  int64_t timestamp_ms;
+  /// The location of a manifest list for this snapshot that tracks manifest files with
+  /// additional metadata.
+  std::variant<manifest_list_t, manifests_t> manifest_list;
+  /// A string map that summaries the snapshot changes, including operation.
+  std::unordered_map<std::string, std::string> summary;
+  /// ID of the table's current schema when the snapshot was created.
+  std::optional<int32_t> schema_id;
+
+  /// \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> operation() const;
+
+  /// \brief Get the manifest list for this snapshot.
+  ///
+  /// \return the manifest list for this snapshot, or nullopt if the snapshot has no
+  /// manifest list.
+  std::optional<std::reference_wrapper<const manifest_list_t>> ManifestList() const;
+
+  /// \brief Get the manifests for this snapshot.
+  ///
+  /// \return the manifests for this snapshot, or nullopt if the snapshot has no
+  /// manifests.
+  std::optional<std::reference_wrapper<const manifests_t>> Manifests() const;
 
+  /// \brief Compare two snapshots for equality.
   friend bool operator==(const Snapshot& lhs, const Snapshot& rhs) {
     return lhs.Equals(rhs);
   }
 
+  /// \brief Compare two snapshots for inequality.
   friend bool operator!=(const Snapshot& lhs, const Snapshot& rhs) {
     return !(lhs == rhs);
   }
 
  private:
   /// \brief Compare two snapshots for equality.
   bool Equals(const Snapshot& other) const;
-
-  /// A unqiue long ID.
-  int64_t snapshot_id_;
-  /// The snapshot ID of the snapshot's parent. Omitted for any snapshot with no parent.
-  std::optional<int64_t> parent_snapshot_id_;
-  /// A monotonically increasing long that tracks the order of changes to a table.
-  int64_t sequence_number_;
-  /// A timestamp when the snapshot was created, used for garbage collection and table
-  /// inspection.
-  int64_t timestamp_ms_;
-  /// The location of a manifest list for this snapshot that tracks manifest files with
-  /// additional metadata.
-  std::string manifest_list_;
-  /// A string map that summaries the snapshot changes, including operation.
-  Summary summary_;
-  /// ID of the table's current schema when the snapshot was created.
-  std::optional<int32_t> schema_id_;
 };
 
 }  // namespace iceberg