Copilot commented on code in PR #1104:
URL: https://github.com/apache/iceberg-cpp/pull/1104#discussion_r4508809002
##########
src/iceberg/transaction.cc:
##########
@@ -222,6 +281,7 @@ Status Transaction::ReplayUpdates() {
ICEBERG_CHECK(state_ == TransactionState::kReady,
"Replay requires a ready transaction");
for (const auto& update : pending_updates_) {
+ ICEBERG_RETURN_UNEXPECTED(update->PrepareForReplay());
ICEBERG_RETURN_UNEXPECTED(ApplyUpdate(*update));
Review Comment:
If concurrent partition-spec evolution and `CleanExpiredMetadata(true)`
remove a spec used by a staged append, replay now reuses its cached manifests
without repeating the spec lookup. `FastAppend` and unmerged
`MergingSnapshotUpdate` outputs can therefore commit manifests referencing a
missing spec, causing subsequent scans to fail. Validate cached manifests'
partition-spec IDs against the refreshed metadata before reuse, either in both
append implementations or in shared snapshot validation.
##########
src/iceberg/transaction.cc:
##########
@@ -99,6 +104,60 @@ std::string FormatCommittedSnapshots(
return detail;
}
+/// Collect files that must survive transaction finalization.
+///
+/// The set includes manifests and manifest lists from all snapshots created by
+/// this transaction. It also includes owned content files still referenced by
+/// retained delete manifests.
+///
+/// \param metadata The metadata after the transaction commit.
+/// \param io FileIO used to read manifest lists and delete manifests.
+/// \param snapshot_ids Snapshot IDs in the committed AddSnapshot changes.
+/// \param content_paths Owned content files that may need live-entry checks.
+Result<std::unordered_set<std::string>> CommittedTransactionFiles(
+ const TableMetadata& metadata, const std::shared_ptr<FileIO>& io,
+ std::unordered_set<int64_t> snapshot_ids,
+ const std::unordered_set<std::string>& content_paths) {
+ std::unordered_set<std::string> committed;
+ for (const auto& snapshot : metadata.snapshots) {
+ if (!snapshot || snapshot_ids.erase(snapshot->snapshot_id) == 0) {
+ continue;
+ }
+ committed.insert(snapshot->manifest_list);
+ SnapshotReader snapshot_reader(snapshot.get());
+ ICEBERG_ASSIGN_OR_RAISE(auto manifests, snapshot_reader.Manifests(io));
+ for (const auto& manifest : manifests) {
+ if (!committed.insert(manifest.manifest_path).second ||
content_paths.empty() ||
+ manifest.content != ManifestContent::kDeletes) {
+ continue;
+ }
+ // A merged DV can be shared by several retained snapshots.
+ ICEBERG_ASSIGN_OR_RAISE(auto schema, metadata.Schema());
+ ICEBERG_ASSIGN_OR_RAISE(auto spec,
+
metadata.PartitionSpecById(manifest.partition_spec_id));
+ ICEBERG_ASSIGN_OR_RAISE(auto reader,
+ ManifestReader::Make(manifest, io, schema,
spec));
+ ICEBERG_ASSIGN_OR_RAISE(auto entries, reader->LiveEntriesStream());
+ while (true) {
+ ICEBERG_ASSIGN_OR_RAISE(auto entry, entries->Next());
+ if (!entry) {
+ break;
+ }
+ ICEBERG_CHECK(entry->data_file != nullptr,
+ "Missing content file in committed manifest {}",
+ manifest.manifest_path);
+ if (content_paths.contains(entry->data_file->file_path)) {
+ committed.insert(entry->data_file->file_path);
Review Comment:
Please add a commit/replay regression for the owned-content keep-set. The
existing duplicate-DV tests in `merging_snapshot_update_test.cc` stop at
`ApplyForTest`, so they never exercise this discovery or terminal deletion.
Cover a merged Puffin file referenced only by a retained intermediate or
staged-only snapshot, and assert that it remains readable while abandoned
replay outputs are cleaned up. This is the new protection against deleting live
deletion vectors.
##########
src/iceberg/update/merging_snapshot_update.cc:
##########
@@ -921,6 +919,12 @@ Result<std::vector<ManifestFile>>
MergingSnapshotUpdate::WriteNewDeleteManifests
return result;
}
+Status MergingSnapshotUpdate::PrepareForReplay() {
+ // The same path can inherit new sequence numbers and row IDs on replay.
+ // Drop derived caches, but keep their files staged for terminal cleanup.
+ return CleanUncommittedManagers({});
Review Comment:
After a `MergeAppend` compacts manifests, a later explicit
`RewriteManifests` replacement still fails on a property-only replay: clearing
the merge cache generates a new path, so the declared source is no longer in
the replayed snapshot. The same issue affects chained rewrites through
`RewriteManifests::PrepareForReplay()` in
`src/iceberg/update/rewrite_manifests.cc:201–203`. Preserve derived outputs
when their inputs and inherited metadata are unchanged, and add replay
regressions for both dependency chains.
--
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]
