Repository navigation
fix(commit): close remaining TableCommit parity gaps - #915
Conversation
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Reviewed d0948d2. The core changes and added regressions look sound: cargo test --offline --locked -p paimon --all-targets passed (3,331 passed, 6 ignored, default features), including all 100 TableCommit tests. An additional review-only recovery/publication-race probe passed all four scenarios (empty/nonempty base, duplicate/unrelated concurrent append), with snapshot row counts and retry behavior checked. Formatting checks passed.
One integration-test update is still needed for the stricter DV invariant, as noted inline; the current DataFusion CI fails there, so I am not approving this revision yet.
The separate Python CI failure in test_close_cleans_unprepared_output_but_preserves_prepared_files[-2] also reproduces with the pre-PR #912 native runtime and identical test source (one pass followed by the same failure at the unprepared-file assertion). I therefore do not attribute that intermittent failure to this PR.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Re-reviewed dc5c4fb. The previous P2 finding is addressed: the stale-DV reader fixture now removes the data file and DV through a valid commit, then constructs the legacy snapshot metadata directly, preserving the count/read assertions. The Python close test now checks cleanup after close completes instead of racing background file closure. No remaining blocking findings.
Validation on this revision:
- DataFusion partition_count_pushdown integration suite: 14 passed.
- Python test_table_commit.py with a wheel freshly built from this revision: 36 passed.
- The close/cleanup regression in all three bucket modes repeated 10 times: all 30 executions passed.
- cargo fmt --all -- --check: passed.
Some GitHub checks are still queued/in progress; this approval is based on the local validation above.
2 participants
Purpose
Close the remaining TableCommit correctness gaps found after #913, including the batch recovery API added by #912. Replaying an expired commit identity can currently add an already active file again and inflate snapshot row counts. Deletion-vector commits also reject unrelated writes and cannot prepare a DELETE when a REST catalog supplies snapshots without physical snapshot files.
Brief change log
Tests
cargo test --locked -p paimon --all-targets --features fulltext,vortex: 3429 passed, 6 ignored.pytest -q bindings/python/tests/test_table_commit.py, using a wheel built from this branch: 36 passed.cargo clippy --locked -p paimon --all-targets --features fulltext,vortex -- -D warnings: passed.cargo fmt --all -- --checkand explicit rustfmt check of the included recovery test file: passed.The workspace-wide Clippy command is blocked by an existing
clippy::collapsible_matchwarning atcrates/integrations/datafusion/src/partition_count_pushdown.rs:165, which is unchanged from main.API and Format
No public signatures or storage formats change. Recovery rejects missing or already active files and duplicate checkpoint identifiers; valid recovery and unrelated concurrent DELETE operations succeed.
Documentation
No new configuration or documentation changes are required.