wgtmac commented on code in PR #408:
URL: https://github.com/apache/iceberg-cpp/pull/408#discussion_r2661572173
##########
src/iceberg/util/string_util.h:
##########
@@ -54,6 +56,16 @@ class ICEBERG_EXPORT StringUtils {
}
return count;
}
+
+ template <typename T>
+ static std::optional<T> ParseInt(std::string_view str) {
Review Comment:
Should we return `Result<T>` and convert all errors to `InvalidArgument`
with detail message for `std::errc::invalid_argument` and
`std::errc::result_out_of_range`?
##########
src/iceberg/table_metadata.cc:
##########
@@ -982,6 +994,158 @@ void
TableMetadataBuilder::Impl::SetLocation(std::string_view location) {
changes_.push_back(std::make_unique<table::SetLocation>(std::string(location)));
}
+Status TableMetadataBuilder::Impl::AddSnapshot(std::shared_ptr<Snapshot>
snapshot) {
+ if (snapshot == nullptr) {
+ // change is a noop
+ return {};
+ }
+ ICEBERG_CHECK(!metadata_.schemas.empty(),
+ "Attempting to add a snapshot before a schema is added");
+ ICEBERG_CHECK(!metadata_.partition_specs.empty(),
+ "Attempting to add a snapshot before a partition spec is
added");
+ ICEBERG_CHECK(!metadata_.sort_orders.empty(),
+ "Attempting to add a snapshot before a sort order is added");
+
+ ICEBERG_CHECK(!snapshots_by_id_.contains(snapshot->snapshot_id),
+ "Snapshot already exists for id: {}", snapshot->snapshot_id);
+
+ ICEBERG_CHECK(
+ metadata_.format_version == 1 ||
+ snapshot->sequence_number > metadata_.last_sequence_number ||
+ !snapshot->parent_snapshot_id.has_value(),
+ "Cannot add snapshot with sequence number {} older than last sequence
number {}",
+ snapshot->sequence_number, metadata_.last_sequence_number);
+
+ metadata_.last_updated_ms = snapshot->timestamp_ms;
+ metadata_.last_sequence_number = snapshot->sequence_number;
+
+ metadata_.snapshots.push_back(snapshot);
+ snapshots_by_id_.emplace(snapshot->snapshot_id, snapshot);
+
+ changes_.push_back(std::make_unique<table::AddSnapshot>(snapshot));
+
+ // Handle row lineage for format version >= 3
+ if (metadata_.format_version >= TableMetadata::kMinFormatVersionRowLineage) {
+ auto first_row_id = snapshot->FirstRowId();
+ ICEBERG_CHECK(first_row_id.has_value(),
+ "Cannot add a snapshot: first-row-id is null");
+ ICEBERG_CHECK(
+ first_row_id.value() >= metadata_.next_row_id,
+ "Cannot add a snapshot, first-row-id is behind table next-row-id: {} <
{}",
+ first_row_id.value(), metadata_.next_row_id);
+
+ metadata_.next_row_id += snapshot->AddedRows().value_or(0);
+ }
+
+ return {};
+}
+
+Status TableMetadataBuilder::Impl::SetBranchSnapshot(int64_t snapshot_id,
Review Comment:
Java impl has a convenience method `setBranchSnapshot(Snapshot snapshot,
String branch)` which we don't have. Do we need it?
##########
src/iceberg/util/uuid.h:
##########
@@ -78,6 +78,9 @@ class ICEBERG_EXPORT Uuid : public util::Formattable {
return lhs.data_ == rhs.data_;
}
+ int64_t highbits() const;
+ int64_t lowbits() const;
Review Comment:
```suggestion
int64_t high_bits() const;
int64_t low_bits() const;
```
##########
src/iceberg/snapshot.cc:
##########
@@ -75,6 +78,24 @@ std::optional<std::string_view> Snapshot::operation() const {
return std::nullopt;
}
+std::optional<int64_t> Snapshot::FirstRowId() const {
+ auto it = summary.find("first-row-id");
+ if (it == summary.end()) {
+ return std::nullopt;
+ }
+
+ return StringUtils::ParseInt<int64_t>(it->second);
+}
+
+std::optional<int64_t> Snapshot::AddedRows() const {
Review Comment:
Same question here.
##########
src/iceberg/table_metadata.cc:
##########
@@ -982,6 +994,158 @@ void
TableMetadataBuilder::Impl::SetLocation(std::string_view location) {
changes_.push_back(std::make_unique<table::SetLocation>(std::string(location)));
}
+Status TableMetadataBuilder::Impl::AddSnapshot(std::shared_ptr<Snapshot>
snapshot) {
+ if (snapshot == nullptr) {
+ // change is a noop
+ return {};
+ }
+ ICEBERG_CHECK(!metadata_.schemas.empty(),
+ "Attempting to add a snapshot before a schema is added");
+ ICEBERG_CHECK(!metadata_.partition_specs.empty(),
+ "Attempting to add a snapshot before a partition spec is
added");
+ ICEBERG_CHECK(!metadata_.sort_orders.empty(),
+ "Attempting to add a snapshot before a sort order is added");
+
+ ICEBERG_CHECK(!snapshots_by_id_.contains(snapshot->snapshot_id),
+ "Snapshot already exists for id: {}", snapshot->snapshot_id);
+
+ ICEBERG_CHECK(
+ metadata_.format_version == 1 ||
+ snapshot->sequence_number > metadata_.last_sequence_number ||
+ !snapshot->parent_snapshot_id.has_value(),
+ "Cannot add snapshot with sequence number {} older than last sequence
number {}",
+ snapshot->sequence_number, metadata_.last_sequence_number);
+
+ metadata_.last_updated_ms = snapshot->timestamp_ms;
+ metadata_.last_sequence_number = snapshot->sequence_number;
+
+ metadata_.snapshots.push_back(snapshot);
+ snapshots_by_id_.emplace(snapshot->snapshot_id, snapshot);
+
+ changes_.push_back(std::make_unique<table::AddSnapshot>(snapshot));
+
+ // Handle row lineage for format version >= 3
+ if (metadata_.format_version >= TableMetadata::kMinFormatVersionRowLineage) {
+ auto first_row_id = snapshot->FirstRowId();
+ ICEBERG_CHECK(first_row_id.has_value(),
+ "Cannot add a snapshot: first-row-id is null");
+ ICEBERG_CHECK(
+ first_row_id.value() >= metadata_.next_row_id,
+ "Cannot add a snapshot, first-row-id is behind table next-row-id: {} <
{}",
+ first_row_id.value(), metadata_.next_row_id);
+
+ metadata_.next_row_id += snapshot->AddedRows().value_or(0);
+ }
+
+ return {};
+}
+
+Status TableMetadataBuilder::Impl::SetBranchSnapshot(int64_t snapshot_id,
+ const std::string&
branch) {
+ // Check if ref already exists with the same snapshot ID
+ auto ref_it = metadata_.refs.find(branch);
+ if (ref_it != metadata_.refs.end() && ref_it->second->snapshot_id ==
snapshot_id) {
+ return {};
+ }
+
+ auto snapshot_it = snapshots_by_id_.find(snapshot_id);
+ ICEBERG_PRECHECK(snapshot_it != snapshots_by_id_.end(),
+ "Cannot set {} to unknown snapshot: {}", branch,
snapshot_id);
+ const auto& snapshot = snapshot_it->second;
+
+ // If ref exists, validate it's a branch and check if snapshot ID matches
+ if (ref_it != metadata_.refs.end()) {
+ const auto& ref = ref_it->second;
+ ICEBERG_CHECK(ref->type() == SnapshotRefType::kBranch,
+ "Cannot update branch: {} is a tag", branch);
+ if (ref->snapshot_id == snapshot_id) {
+ return {};
+ }
+ }
+
+ ICEBERG_CHECK(
+ metadata_.format_version == 1 ||
+ snapshot->sequence_number <= metadata_.last_sequence_number,
+ "Last sequence number {} is less than existing snapshot sequence number
{}",
+ metadata_.last_sequence_number, snapshot->sequence_number);
+
+ // Create new ref: either from existing ref or create new branch ref
+ std::shared_ptr<SnapshotRef> new_ref;
+ if (ref_it != metadata_.refs.end()) {
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto ref_result,
+ SnapshotRef::Builder::BuilderFrom(*ref_it->second,
snapshot_id).Build());
Review Comment:
It seems that `SnapshotRef::Builder::Build()` should return
`Result<std::unique_ptr<SnapshotRef>>`.
##########
src/iceberg/snapshot.h:
##########
@@ -119,6 +120,67 @@ struct ICEBERG_EXPORT SnapshotRef {
return lhs.Equals(rhs);
}
+ /// \brief Builder class for constructing SnapshotRef objects
+ class ICEBERG_EXPORT Builder : public ErrorCollector {
+ public:
+ /// \brief Create a builder for a tag reference
+ /// \param snapshot_id The snapshot ID for the tag
+ /// \return A new Builder instance for a tag
+ static Builder TagBuilder(int64_t snapshot_id);
+
+ /// \brief Create a builder for a branch reference
+ /// \param snapshot_id The snapshot ID for the branch
+ /// \return A new Builder instance for a branch
+ static Builder BranchBuilder(int64_t snapshot_id);
+
+ /// \brief Create a builder from an existing SnapshotRef
+ /// \param ref The existing reference to copy properties from
+ /// \return A new Builder instance with properties from the existing ref
+ static Builder BuilderFrom(const SnapshotRef& ref);
+
+ /// \brief Create a builder from an existing SnapshotRef with a new
snapshot ID
+ /// \param ref The existing reference to copy properties from
+ /// \param snapshot_id The new snapshot ID to use
+ /// \return A new Builder instance with properties from the existing ref
but new
+ /// snapshot ID
+ static Builder BuilderFrom(const SnapshotRef& ref, int64_t snapshot_id);
+
+ /// \brief Create a builder for a specific type
+ /// \param snapshot_id The snapshot ID
+ /// \param type The type of reference (branch or tag)
+ /// \return A new Builder instance
+ static Builder BuilderFor(int64_t snapshot_id, SnapshotRefType type);
+
+ /// \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 Reference to this builder for method chaining
+ Builder& 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 Reference to this builder for method chaining
+ Builder& 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 Reference to this builder for method chaining
+ Builder& MaxRefAgeMs(std::optional<int64_t> value);
+
+ /// \brief Build the SnapshotRef
+ /// \return A Result containing the SnapshotRef instance, or an error if
validation
+ /// failed
+ Result<SnapshotRef> Build() const;
Review Comment:
As I have commented in other comment, it is better to return
`Result<std::unique_ptr<SnapshotRef>>`.
##########
src/iceberg/transaction.cc:
##########
@@ -158,6 +176,15 @@ Result<std::shared_ptr<Table>> Transaction::Commit() {
ICEBERG_ASSIGN_OR_RAISE(auto updated_table, table_->catalog()->UpdateTable(
table_->name(),
requirements, updates));
+ for (const auto& update : pending_updates_) {
+ if (auto update_ptr = update.lock()) {
+ if (auto snapshot_update =
+ internal::checked_cast<SnapshotUpdate*>(update_ptr.get())) {
+ std::ignore = snapshot_update->Finalize();
Review Comment:
cc @dongxiao1198 as `ExpireSnapshots` may need to deal with the same issue.
##########
src/iceberg/table_metadata.h:
##########
@@ -145,6 +145,8 @@ struct ICEBERG_EXPORT TableMetadata {
Result<std::shared_ptr<iceberg::Snapshot>> Snapshot() const;
/// \brief Get the snapshot of this table with the given id
Result<std::shared_ptr<iceberg::Snapshot>> SnapshotById(int64_t snapshot_id)
const;
+ /// \brief Get the next sequence number for
Review Comment:
```suggestion
/// \brief Get the next sequence number
```
##########
src/iceberg/snapshot.cc:
##########
@@ -75,6 +78,24 @@ std::optional<std::string_view> Snapshot::operation() const {
return std::nullopt;
}
+std::optional<int64_t> Snapshot::FirstRowId() const {
Review Comment:
Should we return `Result<std::optional<int64_t>>` and error out on parsing
failure?
##########
src/iceberg/table_metadata.cc:
##########
@@ -982,6 +994,158 @@ void
TableMetadataBuilder::Impl::SetLocation(std::string_view location) {
changes_.push_back(std::make_unique<table::SetLocation>(std::string(location)));
}
+Status TableMetadataBuilder::Impl::AddSnapshot(std::shared_ptr<Snapshot>
snapshot) {
+ if (snapshot == nullptr) {
+ // change is a noop
+ return {};
+ }
+ ICEBERG_CHECK(!metadata_.schemas.empty(),
+ "Attempting to add a snapshot before a schema is added");
+ ICEBERG_CHECK(!metadata_.partition_specs.empty(),
+ "Attempting to add a snapshot before a partition spec is
added");
+ ICEBERG_CHECK(!metadata_.sort_orders.empty(),
+ "Attempting to add a snapshot before a sort order is added");
+
+ ICEBERG_CHECK(!snapshots_by_id_.contains(snapshot->snapshot_id),
+ "Snapshot already exists for id: {}", snapshot->snapshot_id);
+
+ ICEBERG_CHECK(
+ metadata_.format_version == 1 ||
+ snapshot->sequence_number > metadata_.last_sequence_number ||
+ !snapshot->parent_snapshot_id.has_value(),
+ "Cannot add snapshot with sequence number {} older than last sequence
number {}",
+ snapshot->sequence_number, metadata_.last_sequence_number);
+
+ metadata_.last_updated_ms = snapshot->timestamp_ms;
+ metadata_.last_sequence_number = snapshot->sequence_number;
+
+ metadata_.snapshots.push_back(snapshot);
+ snapshots_by_id_.emplace(snapshot->snapshot_id, snapshot);
+
+ changes_.push_back(std::make_unique<table::AddSnapshot>(snapshot));
+
+ // Handle row lineage for format version >= 3
+ if (metadata_.format_version >= TableMetadata::kMinFormatVersionRowLineage) {
+ auto first_row_id = snapshot->FirstRowId();
+ ICEBERG_CHECK(first_row_id.has_value(),
+ "Cannot add a snapshot: first-row-id is null");
+ ICEBERG_CHECK(
+ first_row_id.value() >= metadata_.next_row_id,
+ "Cannot add a snapshot, first-row-id is behind table next-row-id: {} <
{}",
+ first_row_id.value(), metadata_.next_row_id);
+
+ metadata_.next_row_id += snapshot->AddedRows().value_or(0);
+ }
+
+ return {};
+}
+
+Status TableMetadataBuilder::Impl::SetBranchSnapshot(int64_t snapshot_id,
+ const std::string&
branch) {
+ // Check if ref already exists with the same snapshot ID
+ auto ref_it = metadata_.refs.find(branch);
+ if (ref_it != metadata_.refs.end() && ref_it->second->snapshot_id ==
snapshot_id) {
+ return {};
+ }
+
+ auto snapshot_it = snapshots_by_id_.find(snapshot_id);
+ ICEBERG_PRECHECK(snapshot_it != snapshots_by_id_.end(),
+ "Cannot set {} to unknown snapshot: {}", branch,
snapshot_id);
+ const auto& snapshot = snapshot_it->second;
+
+ // If ref exists, validate it's a branch and check if snapshot ID matches
+ if (ref_it != metadata_.refs.end()) {
+ const auto& ref = ref_it->second;
+ ICEBERG_CHECK(ref->type() == SnapshotRefType::kBranch,
+ "Cannot update branch: {} is a tag", branch);
+ if (ref->snapshot_id == snapshot_id) {
+ return {};
+ }
+ }
+
+ ICEBERG_CHECK(
+ metadata_.format_version == 1 ||
+ snapshot->sequence_number <= metadata_.last_sequence_number,
+ "Last sequence number {} is less than existing snapshot sequence number
{}",
+ metadata_.last_sequence_number, snapshot->sequence_number);
+
+ // Create new ref: either from existing ref or create new branch ref
+ std::shared_ptr<SnapshotRef> new_ref;
+ if (ref_it != metadata_.refs.end()) {
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto ref_result,
+ SnapshotRef::Builder::BuilderFrom(*ref_it->second,
snapshot_id).Build());
+ new_ref = std::make_shared<SnapshotRef>(std::move(ref_result));
+ } else {
+ ICEBERG_ASSIGN_OR_RAISE(auto ref_result,
+
SnapshotRef::Builder::BranchBuilder(snapshot_id).Build());
+ new_ref = std::make_shared<SnapshotRef>(std::move(ref_result));
+ }
+
+ return SetRef(branch, std::move(new_ref));
+}
+
+Status TableMetadataBuilder::Impl::SetRef(const std::string& name,
+ std::shared_ptr<SnapshotRef> ref) {
+ auto existing_ref_it = metadata_.refs.find(name);
+ if (existing_ref_it != metadata_.refs.end() && *existing_ref_it->second ==
*ref) {
+ return {};
+ }
+
+ int64_t snapshot_id = ref->snapshot_id;
+ auto snapshot_it = snapshots_by_id_.find(snapshot_id);
+ ICEBERG_CHECK(snapshot_it != snapshots_by_id_.end(),
+ "Cannot set {} to unknown snapshot: {}", name, snapshot_id);
+ const auto& snapshot = snapshot_it->second;
+
+ // If snapshot was added in this set of changes, update last_updated_ms
+ if (std::ranges::any_of(changes_, [snapshot_id](const auto& change) {
+ if (change->kind() != TableUpdate::Kind::kAddSnapshot) {
+ return false;
+ }
+ const auto* add_snapshot =
+ internal::checked_cast<const table::AddSnapshot*>(change.get());
+ return add_snapshot->snapshot()->snapshot_id == snapshot_id;
+ })) {
+ metadata_.last_updated_ms = snapshot->timestamp_ms;
+ }
+
+ // If it's MAIN_BRANCH, update currentSnapshotId and add to snapshotLog
+ if (name == SnapshotRef::kMainBranch) {
+ metadata_.current_snapshot_id = ref->snapshot_id;
+ if (metadata_.last_updated_ms == kInvalidLastUpdatedMs) {
+ metadata_.last_updated_ms =
+ TimePointMs{std::chrono::duration_cast<std::chrono::milliseconds>(
Review Comment:
Perhaps we can add an utility function to iceberg/util/timepoint.h to return
now since this is too long.
##########
src/iceberg/table_metadata.cc:
##########
@@ -982,6 +994,158 @@ void
TableMetadataBuilder::Impl::SetLocation(std::string_view location) {
changes_.push_back(std::make_unique<table::SetLocation>(std::string(location)));
}
+Status TableMetadataBuilder::Impl::AddSnapshot(std::shared_ptr<Snapshot>
snapshot) {
+ if (snapshot == nullptr) {
+ // change is a noop
+ return {};
+ }
+ ICEBERG_CHECK(!metadata_.schemas.empty(),
+ "Attempting to add a snapshot before a schema is added");
+ ICEBERG_CHECK(!metadata_.partition_specs.empty(),
+ "Attempting to add a snapshot before a partition spec is
added");
+ ICEBERG_CHECK(!metadata_.sort_orders.empty(),
+ "Attempting to add a snapshot before a sort order is added");
+
+ ICEBERG_CHECK(!snapshots_by_id_.contains(snapshot->snapshot_id),
+ "Snapshot already exists for id: {}", snapshot->snapshot_id);
+
+ ICEBERG_CHECK(
+ metadata_.format_version == 1 ||
+ snapshot->sequence_number > metadata_.last_sequence_number ||
+ !snapshot->parent_snapshot_id.has_value(),
+ "Cannot add snapshot with sequence number {} older than last sequence
number {}",
+ snapshot->sequence_number, metadata_.last_sequence_number);
+
+ metadata_.last_updated_ms = snapshot->timestamp_ms;
+ metadata_.last_sequence_number = snapshot->sequence_number;
+
+ metadata_.snapshots.push_back(snapshot);
+ snapshots_by_id_.emplace(snapshot->snapshot_id, snapshot);
+
+ changes_.push_back(std::make_unique<table::AddSnapshot>(snapshot));
+
+ // Handle row lineage for format version >= 3
+ if (metadata_.format_version >= TableMetadata::kMinFormatVersionRowLineage) {
+ auto first_row_id = snapshot->FirstRowId();
+ ICEBERG_CHECK(first_row_id.has_value(),
+ "Cannot add a snapshot: first-row-id is null");
+ ICEBERG_CHECK(
+ first_row_id.value() >= metadata_.next_row_id,
+ "Cannot add a snapshot, first-row-id is behind table next-row-id: {} <
{}",
+ first_row_id.value(), metadata_.next_row_id);
+
+ metadata_.next_row_id += snapshot->AddedRows().value_or(0);
+ }
+
+ return {};
+}
+
+Status TableMetadataBuilder::Impl::SetBranchSnapshot(int64_t snapshot_id,
+ const std::string&
branch) {
+ // Check if ref already exists with the same snapshot ID
+ auto ref_it = metadata_.refs.find(branch);
+ if (ref_it != metadata_.refs.end() && ref_it->second->snapshot_id ==
snapshot_id) {
+ return {};
+ }
+
+ auto snapshot_it = snapshots_by_id_.find(snapshot_id);
+ ICEBERG_PRECHECK(snapshot_it != snapshots_by_id_.end(),
+ "Cannot set {} to unknown snapshot: {}", branch,
snapshot_id);
+ const auto& snapshot = snapshot_it->second;
+
+ // If ref exists, validate it's a branch and check if snapshot ID matches
+ if (ref_it != metadata_.refs.end()) {
+ const auto& ref = ref_it->second;
+ ICEBERG_CHECK(ref->type() == SnapshotRefType::kBranch,
+ "Cannot update branch: {} is a tag", branch);
+ if (ref->snapshot_id == snapshot_id) {
+ return {};
+ }
+ }
+
+ ICEBERG_CHECK(
+ metadata_.format_version == 1 ||
+ snapshot->sequence_number <= metadata_.last_sequence_number,
+ "Last sequence number {} is less than existing snapshot sequence number
{}",
+ metadata_.last_sequence_number, snapshot->sequence_number);
+
+ // Create new ref: either from existing ref or create new branch ref
+ std::shared_ptr<SnapshotRef> new_ref;
+ if (ref_it != metadata_.refs.end()) {
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto ref_result,
+ SnapshotRef::Builder::BuilderFrom(*ref_it->second,
snapshot_id).Build());
+ new_ref = std::make_shared<SnapshotRef>(std::move(ref_result));
+ } else {
+ ICEBERG_ASSIGN_OR_RAISE(auto ref_result,
+
SnapshotRef::Builder::BranchBuilder(snapshot_id).Build());
+ new_ref = std::make_shared<SnapshotRef>(std::move(ref_result));
+ }
+
+ return SetRef(branch, std::move(new_ref));
+}
+
+Status TableMetadataBuilder::Impl::SetRef(const std::string& name,
+ std::shared_ptr<SnapshotRef> ref) {
+ auto existing_ref_it = metadata_.refs.find(name);
+ if (existing_ref_it != metadata_.refs.end() && *existing_ref_it->second ==
*ref) {
+ return {};
+ }
+
+ int64_t snapshot_id = ref->snapshot_id;
+ auto snapshot_it = snapshots_by_id_.find(snapshot_id);
+ ICEBERG_CHECK(snapshot_it != snapshots_by_id_.end(),
+ "Cannot set {} to unknown snapshot: {}", name, snapshot_id);
+ const auto& snapshot = snapshot_it->second;
+
+ // If snapshot was added in this set of changes, update last_updated_ms
+ if (std::ranges::any_of(changes_, [snapshot_id](const auto& change) {
+ if (change->kind() != TableUpdate::Kind::kAddSnapshot) {
+ return false;
+ }
+ const auto* add_snapshot =
+ internal::checked_cast<const table::AddSnapshot*>(change.get());
+ return add_snapshot->snapshot()->snapshot_id == snapshot_id;
+ })) {
+ metadata_.last_updated_ms = snapshot->timestamp_ms;
+ }
+
+ // If it's MAIN_BRANCH, update currentSnapshotId and add to snapshotLog
+ if (name == SnapshotRef::kMainBranch) {
+ metadata_.current_snapshot_id = ref->snapshot_id;
+ if (metadata_.last_updated_ms == kInvalidLastUpdatedMs) {
+ metadata_.last_updated_ms =
+ TimePointMs{std::chrono::duration_cast<std::chrono::milliseconds>(
+ std::chrono::system_clock::now().time_since_epoch())};
+ }
+ metadata_.snapshot_log.emplace_back(metadata_.last_updated_ms,
ref->snapshot_id);
+ }
+
+ // Set the ref
+ metadata_.refs[name] = ref;
+
+ // Extract properties from SnapshotRef for the change
+ std::optional<int32_t> min_snapshots_to_keep = std::nullopt;
+ std::optional<int64_t> max_snapshot_age_ms = std::nullopt;
+ std::optional<int64_t> max_ref_age_ms = std::nullopt;
+
+ if (ref->type() == SnapshotRefType::kBranch) {
+ const auto& branch = std::get<SnapshotRef::Branch>(ref->retention);
+ min_snapshots_to_keep = branch.min_snapshots_to_keep;
+ max_snapshot_age_ms = branch.max_snapshot_age_ms;
+ max_ref_age_ms = branch.max_ref_age_ms;
+ } else {
+ const auto& tag = std::get<SnapshotRef::Tag>(ref->retention);
+ max_ref_age_ms = tag.max_ref_age_ms;
+ }
+
+ changes_.push_back(std::make_unique<table::SetSnapshotRef>(
Review Comment:
+1
##########
src/iceberg/snapshot.h:
##########
@@ -119,6 +120,67 @@ struct ICEBERG_EXPORT SnapshotRef {
return lhs.Equals(rhs);
}
+ /// \brief Builder class for constructing SnapshotRef objects
+ class ICEBERG_EXPORT Builder : public ErrorCollector {
Review Comment:
I'm not sure if it is simple to directly add
`Result<std::unique_ptr<SnapshotRef>> SnapshotRef::MakeBranch(...)` and
`Result<std::unique_ptr<SnapshotRef>> SnapshotRef::MakeTag(...)`? For
`BuildFrom`, is it simpler to add a `SnapshotRef::Clone` and directly modify
values on the cloned object? Introducing builder pattern seems an overkill here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
