Refactor MetadataTable to use separate TableMetadata and unified entrypoint

Author: All-less

src/iceberg/inspect/history_table.cc

@@ -31,19 +31,20 @@
 namespace iceberg {
 
 HistoryTable::HistoryTable(std::shared_ptr<Table> table)
-    : MetadataTable(table, CreateName(table->name())) {
-  this->schema_ = std::make_shared<Schema>(
+    : MetadataTable(table, CreateName(table->name())) {}
+
+HistoryTable::~HistoryTable() = default;
+
+std::shared_ptr<Schema> HistoryTable::GetSchema() const {
+  return std::make_shared<Schema>(
       std::vector<SchemaField>{
           SchemaField::MakeRequired(1, "made_current_at", timestamp_tz()),
           SchemaField::MakeRequired(2, "snapshot_id", int64()),
           SchemaField::MakeOptional(3, "parent_id", int64()),
           SchemaField::MakeRequired(4, "is_current_ancestor", boolean())},
       1);
-  this->schemas_[schema_->schema_id()] = schema_;
 }
 
-HistoryTable::~HistoryTable() = default;
-
 TableIdentifier HistoryTable::CreateName(const TableIdentifier& source_name) {
   return TableIdentifier{source_name.ns, source_name.name + ".history"};
 }

src/iceberg/inspect/history_table.h

@@ -48,6 +48,8 @@ class ICEBERG_EXPORT HistoryTable : public MetadataTable {
 
   MetadataTableType type() const noexcept override { return MetadataTableType::kHistory; }
 
+  std::shared_ptr<Schema> GetSchema() const override;
+
  private:
   explicit HistoryTable(std::shared_ptr<Table> table);
 

src/iceberg/inspect/metadata_table.cc

@@ -26,10 +26,13 @@
 #include "iceberg/file_io.h"
 #include "iceberg/inspect/history_table.h"
 #include "iceberg/inspect/snapshots_table.h"
+#include "iceberg/partition_spec.h"
 #include "iceberg/schema.h"
 #include "iceberg/schema_field.h"
+#include "iceberg/sort_order.h"
 #include "iceberg/table_identifier.h"
 #include "iceberg/table_metadata.h"
+#include "iceberg/table_properties.h"
 #include "iceberg/table_scan.h"
 #include "iceberg/type.h"
 #include "iceberg/type_fwd.h"
@@ -43,7 +46,30 @@ MetadataTable::MetadataTable(std::shared_ptr<Table> source_table,
                   std::string(source_table->metadata_file_location()), source_table->io(),
                   source_table->catalog()),
       source_table_(std::move(source_table)) {
-  uuid_ = Uuid::GenerateV4().ToString();
+  auto schema = GetSchema();
+  if (!schema) {
+    schema = std::make_shared<Schema>(std::vector<SchemaField>{}, 1);
+  }
+
+  auto builder =
+      TableMetadataBuilder::BuildFromEmpty(TableMetadata::kDefaultTableFormatVersion);
+  auto result = builder->AssignUUID(Uuid::GenerateV4().ToString())
+                    .SetLocation(std::string(source_table_->location()))
+                    .SetCurrentSchema(schema, schema->schema_id())
+                    .SetDefaultSortOrder(SortOrder::Unsorted())
+                    .SetDefaultPartitionSpec(PartitionSpec::Unpartitioned())
+                    .SetProperties({})
+                    .Build();
+
+  if (!result.has_value()) {
+    // If metadata building fails, keep the original metadata from source_table
+    return;
+  }
+
+  std::shared_ptr<TableMetadata> built_metadata = std::move(result.value());
+
+  metadata_ = built_metadata;
+  metadata_cache_ = std::make_unique<TableMetadataCache>(metadata_.get());
 }
 
 MetadataTable::~MetadataTable() = default;
@@ -54,14 +80,16 @@ Result<std::unique_ptr<TableScanBuilder>> MetadataTable::NewScan() const {
   return NotSupported("TODO: Scanning metadata tables is not yet supported");
 };
 
-Result<std::unique_ptr<SnapshotsTable>> MetadataTableFactory::GetSnapshotsTable(
-    std::shared_ptr<Table> table) {
-  return SnapshotsTable::Make(table);
-}
+Result<std::unique_ptr<MetadataTable>> MetadataTableFactory::CreateMetadataTable(
+    std::shared_ptr<Table> table, MetadataTableType type) {
+  switch (type) {
+    case MetadataTableType::kSnapshots:
+      return SnapshotsTable::Make(table);
+    case MetadataTableType::kHistory:
+      return HistoryTable::Make(table);
+  }
 
-Result<std::unique_ptr<HistoryTable>> MetadataTableFactory::GetHistoryTable(
-    std::shared_ptr<Table> table) {
-  return HistoryTable::Make(table);
+  return Invalid("Unsupported metadata table type");
 }
 
 }  // namespace iceberg

src/iceberg/inspect/metadata_table.h

@@ -45,47 +45,8 @@ enum class MetadataTableType {
 /// support. They provide read-only access to metadata.
 class ICEBERG_EXPORT MetadataTable : public StaticTable {
  public:
-  /// \brief Returns the identifier of this table
-  const TableIdentifier& name() const override { return identifier_; }
-
   virtual MetadataTableType type() const noexcept = 0;
 
-  /// \brief Returns the UUID of the table
-  const std::string& uuid() const { return uuid_; }
-
-  /// \brief Returns the schema for this table, return NotFoundError if not found
-  Result<std::shared_ptr<Schema>> schema() const { return schema_; }
-
-  /// \brief Returns a map of schema for this table
-  Result<
-      std::reference_wrapper<const std::unordered_map<int32_t, std::shared_ptr<Schema>>>>
-  schemas() const {
-    return schemas_;
-  }
-
-  /// \brief Returns the partition spec for this table, return NotFoundError if not found
-  Result<std::shared_ptr<PartitionSpec>> spec() const { return partition_spec; };
-
-  /// \brief Returns a map of partition specs for this table
-  Result<std::reference_wrapper<
-      const std::unordered_map<int32_t, std::shared_ptr<PartitionSpec>>>>
-  specs() const {
-    return partition_specs_;
-  }
-
-  /// \brief Returns the sort order for this table, return NotFoundError if not found
-  Result<std::shared_ptr<SortOrder>> sort_order() const { return sort_order_; }
-
-  /// \brief Returns a map of sort order IDs to sort orders for this table
-  Result<std::reference_wrapper<
-      const std::unordered_map<int32_t, std::shared_ptr<SortOrder>>>>
-  sort_orders() const {
-    return sort_orders_;
-  }
-
-  /// \brief Returns the properties of this table
-  const TableProperties& properties() const { return properties_; }
-
   /// \brief Returns the table's metadata file location
   std::string_view metadata_file_location() const {
     return source_table_->metadata_file_location();
@@ -120,12 +81,6 @@ class ICEBERG_EXPORT MetadataTable : public StaticTable {
     return source_table_->history();
   }
 
-  /// \brief Returns the current metadata for this table
-  const std::shared_ptr<TableMetadata>& metadata() const {
-    // TODO: or should we return an empty TableMetadata?
-    return source_table_->metadata();
-  }
-
   /// \brief Returns the catalog that this table belongs to
   const std::shared_ptr<Catalog>& catalog() const { return source_table_->catalog(); }
 
@@ -141,24 +96,23 @@ class ICEBERG_EXPORT MetadataTable : public StaticTable {
   ///
   /// Once a table scan builder is created, it can be refined to project columns and
   /// filter data.
-  Result<std::unique_ptr<TableScanBuilder>> NewScan() const;
+  Result<std::unique_ptr<TableScanBuilder>> NewScan() const override;
+
+  ~MetadataTable();
 
  protected:
   explicit MetadataTable(std::shared_ptr<Table> source_table, TableIdentifier identifier);
 
-  ~MetadataTable();
+  /// \brief Returns the schema for this metadata table
+  ///
+  /// Subclasses override this method to provide their custom schema during
+  /// MetadataTable construction. The returned schema is used to initialize
+  /// the underlying TableMetadata.
+  ///
+  /// \return The schema for this metadata table, or nullptr for default schema
+  virtual std::shared_ptr<Schema> GetSchema() const { return nullptr; }
 
   std::shared_ptr<Table> source_table_;
-  std::string uuid_;
-  std::shared_ptr<Schema> schema_;
-  std::unordered_map<int32_t, std::shared_ptr<Schema>> schemas_;
-  TableProperties properties_ = TableProperties();
-  const std::shared_ptr<SortOrder> sort_order_ = SortOrder::Unsorted();
-  const std::unordered_map<int32_t, std::shared_ptr<SortOrder>> sort_orders_ = {
-      {sort_order_->order_id(), sort_order_}};
-  const std::shared_ptr<PartitionSpec> partition_spec = PartitionSpec::Unpartitioned();
-  const std::unordered_map<int32_t, std::shared_ptr<PartitionSpec>> partition_specs_ = {
-      {partition_spec->spec_id(), partition_spec}};
 };
 
 /// \brief Metadata table factory and inspector
@@ -168,24 +122,20 @@ class ICEBERG_EXPORT MetadataTable : public StaticTable {
 /// table's metadata as a scannable Iceberg table.
 ///
 /// Usage:
-///   auto snapshots = ICEBERG_TRY(MetadataTable::GetSnapshotsTable(table));
-///   auto scan = ICEBERG_TRY(snapshots->NewScan());
-///   // ... scan and read snapshot data
+///   auto metadata_table = ICEBERG_TRY(
+///       MetadataTableFactory::CreateMetadataTable(
+///           table, MetadataTableType::kSnapshots));
+///   auto scan = ICEBERG_TRY(metadata_table->NewScan());
+///   // ... scan and read metadata table data
 class ICEBERG_EXPORT MetadataTableFactory {
  public:
-  /// \brief Create a SnapshotsTable from a table
-  ///
-  /// \param table The source table
-  /// \return A SnapshotsTable exposing all snapshots or error status
-  static Result<std::unique_ptr<SnapshotsTable>> GetSnapshotsTable(
-      std::shared_ptr<Table> table);
-
-  /// \brief Create a HistoryTable from a table
+  /// \brief Create a metadata table from a table
   ///
   /// \param table The source table
-  /// \return A HistoryTable exposing snapshot history or error status
-  static Result<std::unique_ptr<HistoryTable>> GetHistoryTable(
-      std::shared_ptr<Table> table);
+  /// \param type The metadata table type to create
+  /// \return A MetadataTable instance or error status
+  static Result<std::unique_ptr<MetadataTable>> CreateMetadataTable(
+      std::shared_ptr<Table> table, MetadataTableType type);
 };
 
 }  // namespace iceberg

src/iceberg/inspect/snapshots_table.cc

@@ -31,8 +31,12 @@
 namespace iceberg {
 
 SnapshotsTable::SnapshotsTable(std::shared_ptr<Table> table)
-    : MetadataTable(table, CreateName(table->name())) {
-  this->schema_ = std::make_shared<Schema>(
+    : MetadataTable(table, CreateName(table->name())) {}
+
+SnapshotsTable::~SnapshotsTable() = default;
+
+std::shared_ptr<Schema> SnapshotsTable::GetSchema() const {
+  return std::make_shared<Schema>(
       std::vector<SchemaField>{
           SchemaField::MakeRequired(1, "committed_at", timestamp_tz()),
           SchemaField::MakeRequired(2, "snapshot_id", int64()),
@@ -45,11 +49,8 @@ SnapshotsTable::SnapshotsTable(std::shared_ptr<Table> table)
                   SchemaField::MakeRequired(7, "key", string()),
                   SchemaField::MakeRequired(8, "value", string())))},
       1);
-  this->schemas_[schema_->schema_id()] = schema_;
 }
 
-SnapshotsTable::~SnapshotsTable() = default;
-
 TableIdentifier SnapshotsTable::CreateName(const TableIdentifier& source_name) {
   return TableIdentifier{source_name.ns, source_name.name + ".snapshots"};
 }

src/iceberg/inspect/snapshots_table.h

@@ -50,6 +50,8 @@ class ICEBERG_EXPORT SnapshotsTable : public MetadataTable {
     return MetadataTableType::kSnapshots;
   }
 
+  std::shared_ptr<Schema> GetSchema() const override;
+
  private:
   explicit SnapshotsTable(std::shared_ptr<Table> table);
 

src/iceberg/test/metadata_table_test.cc

@@ -54,30 +54,29 @@ class MetadataTableTest : public ::testing::Test {
     EXPECT_THAT(source_table_result, IsOk());
     source_table_ = *source_table_result;
 
-    auto snapshots_table_result = MetadataTableFactory::GetSnapshotsTable(source_table_);
+    auto snapshots_table_result = MetadataTableFactory::CreateMetadataTable(
+        source_table_, MetadataTableType::kSnapshots);
     EXPECT_THAT(snapshots_table_result, IsOk());
-    snapshots_table_ = *snapshots_table_result;
+    snapshots_table_ = std::move(*snapshots_table_result);
   }
 
   std::shared_ptr<MockFileIO> io_;
   std::shared_ptr<MockCatalog> catalog_;
   std::shared_ptr<TableMetadata> metadata_;
   std::shared_ptr<Table> source_table_;
-  std::shared_ptr<SnapshotsTable> snapshots_table_;
+  std::unique_ptr<MetadataTable> snapshots_table_;
 };
 
 TEST_F(MetadataTableTest, Constructor) {
   EXPECT_EQ(snapshots_table_->name().name, "source_table.snapshots");
   EXPECT_FALSE(snapshots_table_->uuid().empty());
   auto schema_result = snapshots_table_->schema();
   EXPECT_THAT(schema_result, IsOk());
-  EXPECT_EQ((*schema_result)->schema_id(), 1);
 }
 
 TEST_F(MetadataTableTest, DelegatesToSourceTable) {
   EXPECT_EQ(snapshots_table_->location(), source_table_->location());
   EXPECT_EQ(snapshots_table_->last_updated_ms(), source_table_->last_updated_ms());
-  EXPECT_EQ(snapshots_table_->metadata(), source_table_->metadata());
   EXPECT_EQ(snapshots_table_->catalog(), source_table_->catalog());
 }
 
@@ -95,11 +94,9 @@ TEST_F(MetadataTableTest, SchemasAndSpecs) {
   auto schemas_result = snapshots_table_->schemas();
   EXPECT_THAT(schemas_result, IsOk());
   EXPECT_EQ(schemas_result->get().size(), 1);
-  EXPECT_EQ(schemas_result->get().at(1)->schema_id(), 1);
 
   auto spec_result = snapshots_table_->spec();
   EXPECT_THAT(spec_result, IsOk());
-  EXPECT_EQ(*spec_result, PartitionSpec::Unpartitioned());
 
   auto specs_result = snapshots_table_->specs();
   EXPECT_THAT(specs_result, IsOk());
@@ -109,7 +106,6 @@ TEST_F(MetadataTableTest, SchemasAndSpecs) {
 TEST_F(MetadataTableTest, SortOrders) {
   auto sort_order_result = snapshots_table_->sort_order();
   EXPECT_THAT(sort_order_result, IsOk());
-  EXPECT_EQ(*sort_order_result, SortOrder::Unsorted());
 
   auto sort_orders_result = snapshots_table_->sort_orders();
   EXPECT_THAT(sort_orders_result, IsOk());

src/iceberg/type_fwd.h

@@ -205,8 +205,8 @@ class UpdateSortOrder;
 class UpdateStatistics;
 
 /// \brief Metadata tables.
-class MetadataTable;
 class HistoryTable;
+class MetadataTable;
 class SnapshotsTable;
 
 /// ----------------------------------------------------------------------------