get rid of manifests per Fokko's suggestion

Author: zhjwpku

src/iceberg/snapshot.cc

@@ -34,8 +34,8 @@ const std::string SnapshotSummaryFields::kAddedPosDeleteFiles =
     "added-position-delete-files";
 const std::string SnapshotSummaryFields::kRemovedPosDeleteFiles =
     "removed-position-delete-files";
-const std::string SnapshotSummaryFields::kAddedDVS = "added-dvs";
-const std::string SnapshotSummaryFields::kRemovedDVS = "removed-dvs";
+const std::string SnapshotSummaryFields::kAddedDVs = "added-dvs";
+const std::string SnapshotSummaryFields::kRemovedDVs = "removed-dvs";
 const std::string SnapshotSummaryFields::kRemovedDeleteFiles = "removed-delete-files";
 const std::string SnapshotSummaryFields::kTotalDeleteFiles = "total-delete-files";
 const std::string SnapshotSummaryFields::kAddedRecords = "added-records";
@@ -69,34 +69,6 @@ std::optional<std::string_view> Snapshot::operation() const {
   return std::nullopt;
 }
 
-std::optional<std::reference_wrapper<const ManifestList>> Snapshot::ManifestList() const {
-  return std::visit(
-      [&](const auto& manifest_list)
-          -> std::optional<std::reference_wrapper<const struct ManifestList>> {
-        using T = std::decay_t<decltype(manifest_list)>;
-        if constexpr (std::is_same_v<T, struct ManifestList>) {
-          return std::cref(manifest_list);
-        } else {
-          return std::nullopt;
-        }
-      },
-      manifest_list);
-}
-
-std::optional<std::reference_wrapper<const Manifests>> Snapshot::Manifests() const {
-  return std::visit(
-      [&](const auto& manifest_list)
-          -> std::optional<std::reference_wrapper<const struct Manifests>> {
-        using T = std::decay_t<decltype(manifest_list)>;
-        if constexpr (std::is_same_v<T, struct Manifests>) {
-          return std::cref(manifest_list);
-        } else {
-          return std::nullopt;
-        }
-      },
-      manifest_list);
-}
-
 bool Snapshot::Equals(const Snapshot& other) const {
   if (this == &other) {
     return true;

src/iceberg/snapshot.h

@@ -85,9 +85,9 @@ struct SnapshotSummaryFields {
   /// \brief Number of position delete files removed in the snapshot
   static const std::string kRemovedPosDeleteFiles;
   /// \brief Number of deletion vectors added in the snapshot
-  static const std::string kAddedDVS;
+  static const std::string kAddedDVs;
   /// \brief Number of deletion vectors removed in the snapshot
-  static const std::string kRemovedDVS;
+  static const std::string kRemovedDVs;
   /// \brief Number of positional/equality delete files and deletion vectors removed in
   /// the snapshot
   static const std::string kRemovedDeleteFiles;
@@ -157,17 +157,6 @@ struct ICEBERG_EXPORT DataOperation {
   static constexpr std::string kDelete = "delete";
 };
 
-/// \brief The location of a manifest list for this snapshot that tracks manifest files
-/// with additional metadata
-struct ICEBERG_EXPORT ManifestList {
-  std::string manifest_list_path;
-};
-
-/// \brief A list of manifest file locations.
-struct ICEBERG_EXPORT Manifests {
-  std::vector<std::string> manifest_paths;
-};
-
 /// \brief A snapshot of the data in a table at a point in time.
 ///
 /// A snapshot consist of one or more file manifests, and the complete table contents is
@@ -185,8 +174,8 @@ struct ICEBERG_EXPORT Snapshot {
   /// inspection.
   int64_t timestamp_ms;
   /// The location of a manifest list for this snapshot that tracks manifest files with
-  /// additional metadata(v2) or a list of manifest file locations(v1).
-  std::variant<std::monostate, ManifestList, Manifests> manifest_list;
+  /// additional metadata.
+  std::string 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.
@@ -199,18 +188,6 @@ struct ICEBERG_EXPORT Snapshot {
   /// unknown.
   std::optional<std::string_view> 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 ManifestList>> 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>> Manifests() const;
-
   /// \brief Compare two snapshots for equality.
   friend bool operator==(const Snapshot& lhs, const Snapshot& rhs) {
     return lhs.Equals(rhs);

test/snapshot_test.cc

@@ -48,7 +48,7 @@ TEST_F(SnapshotTest, ConstructionAndFieldAccess) {
                     .parent_snapshot_id = 54321,
                     .sequence_number = 1,
                     .timestamp_ms = 1615569200000,
-                    .manifest_list = ManifestList("s3://example/manifest_list.avro"),
+                    .manifest_list = "s3://example/manifest_list.avro",
                     .summary = summary1,
                     .schema_id = 10};
 
@@ -57,8 +57,7 @@ TEST_F(SnapshotTest, ConstructionAndFieldAccess) {
   EXPECT_EQ(*snapshot.parent_snapshot_id, 54321);
   EXPECT_EQ(snapshot.sequence_number, 1);
   EXPECT_EQ(snapshot.timestamp_ms, 1615569200000);
-  EXPECT_EQ(snapshot.ManifestList()->get().manifest_list_path,
-            "s3://example/manifest_list.avro");
+  EXPECT_EQ(snapshot.manifest_list, "s3://example/manifest_list.avro");
   EXPECT_EQ(snapshot.operation().value(), DataOperation::kAppend);
   EXPECT_EQ(snapshot.summary.at(std::string(SnapshotSummaryFields::kAddedDataFiles)),
             "101");
@@ -70,14 +69,14 @@ TEST_F(SnapshotTest, ConstructionAndFieldAccess) {
 
 TEST_F(SnapshotTest, EqualityComparison) {
   // Test the == and != operators
-  Snapshot snapshot1(12345, {}, 1, 1615569200000,
-                     ManifestList("s3://example/manifest_list.avro"), summary1, {});
+  Snapshot snapshot1(12345, {}, 1, 1615569200000, "s3://example/manifest_list.avro",
+                     summary1, {});
 
-  Snapshot snapshot2(12345, {}, 1, 1615569200000,
-                     ManifestList("s3://example/manifest_list.avro"), summary2, {});
+  Snapshot snapshot2(12345, {}, 1, 1615569200000, "s3://example/manifest_list.avro",
+                     summary2, {});
 
-  Snapshot snapshot3(67890, {}, 1, 1615569200000,
-                     ManifestList("s3://example/manifest_list.avro"), summary3, {});
+  Snapshot snapshot3(67890, {}, 1, 1615569200000, "s3://example/manifest_list.avro",
+                     summary3, {});
 
   EXPECT_EQ(snapshot1, snapshot2);
   EXPECT_NE(snapshot1, snapshot3);