zhuxiangyi commented on PR #965: URL: https://github.com/apache/paimon-rust/pull/965#issuecomment-5954969075
Thanks for the detailed review and the probes. All three findings are fixed in e1d75d2, and the branch is rebased onto main (2d0dda1), with the `table/mod.rs` conflict resolved. **[P1] Branch references.** Before deleting anything, expiration now reads the snapshots and tags of every branch, and the long-lived changelogs of main and every branch. Everything these owners reference is kept: - the live data files they read; - their manifests, including changelog manifest lists and changelog manifests; - the changelog files of an expired snapshot whose changelog list an owner still names. If any of them cannot be read, the call fails before anything is changed, as you suggested. - `test_branch_keeps_the_files_it_shares_with_main` follows your reproduction. Snapshot 1 gets tag t1 and branch b1; t1 is deleted; main is overwritten and then expired. Main reads `[2]` and b1 still reads `[1]`. - `test_unreadable_branch_aborts_before_any_deletion` checks that if a branch's manifest cannot be read, the call fails and no snapshot, manifest, or data file is touched. **[P2] Persisted long-lived changelogs.** I chose to protect these owners rather than reject the run. `changelog/changelog-<id>` owners get the same treatment as branches above. `test_long_lived_changelog_keeps_its_files` uses an input-changelog table, persists `changelog-1`, commits snapshot 2, and expires. The changelog manifest list and every file it references remain. Applying the changelogs' own retention policy is still out of scope. **[P2] Legacy deletion-vector locations.** Before deleting index files, expiration now resolves DV entries with `resolve_legacy_deletion_vector_entries`, as reads do. `test_legacy_deletion_vector_location_is_expired` moves a DV to `table/index/`, checks that reads still find it, supersedes it, expires, and checks that the obsolete file is gone. Each new test fails when the protection it covers is disabled. The file-set invariant used by the existing tests now also counts branch and changelog owners. `cargo test -p paimon --all-targets --features fulltext,vortex` passes, and so do clippy with `-D warnings` and the DataFusion procedure tests. -- 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]
