lucasfang commented on code in PR #357:
URL: https://github.com/apache/paimon-cpp/pull/357#discussion_r4023944427
##########
src/paimon/core/operation/file_store_commit.cpp:
##########
@@ -272,11 +295,12 @@ Result<std::unique_ptr<FileStoreCommit>>
FileStoreCommit::Create(
options.LegacyPartitionNameEnabled(), external_paths,
global_index_external_path,
options.IndexFileInDataFileDir(), ctx->GetMemoryPool()));
- auto snapshot_manager =
std::make_shared<SnapshotManager>(options.GetFileSystem(), root_path);
+ auto snapshot_manager =
Review Comment:
`FileStoreCommit::Create()` passes a branch-scoped `SnapshotManager`
(`file_store_commit.cpp:298`, `:330`) to `ExpireSnapshots`, whose protection
sets (`GetManifestSkippingSet` / `GetDataFileSkippingSet`) come only from that
branch's `retained_snapshots`. Expiration never consults a tag and never scans
other branches, while data/manifest files are shared across branches. So
expiring `dev` deletes a file that has left `dev`'s retained set even when
`main` still references it, and `main` then fails to read.
**Repro:** create `dev` from a `main` tag, compact/overwrite on `dev`, then
`Expire()`.
The docs claim this matches Java's `ExpireSnapshotsImpl`, but that is only
half true. Java collects tagged snapshots *before* deleting files
(apache/paimon#9220), and a branch created from a tag carries that tag — which
is exactly what protects the shared base files. C++ already has a branch-aware
`TagManager` but expiration ignores it. Java is equivalent only for files
referenced by another branch's *untagged* live snapshots.
1. This PR's `expire_snapshots.cpp` diff is log-only, so the missing tag
protection is a pre-existing gap that also hits `main`; the PR newly makes it
reachable on branches.
2. `TestBranchExpireLeavesMainSnapshotMetadataAlone` cannot catch it —
`main` and `dev` commit the same append fixtures, so no shared file is actually
deleted, and it only checks snapshot JSON/hints, not read integrity.
Suggested fix (in order):
1. Fold `TagManager`'s tagged snapshots into the skipping sets, matching
Java's `ExpireSnapshotsImpl`.
2. Otherwise, refuse or hard-warn at runtime on branch expiration until that
lands.
3. Add a regression test that compacts/overwrites on `dev` so a file truly
leaves its retained set while `main` still references it, then reads `main`
back in full.
A documentation warning alone is not enough for a silent data-loss path.
--
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]
