zhang-arvin opened a new pull request, #18268:
URL: https://github.com/apache/iceberg/pull/18268
`mergeDVs` writes a new Puffin file every time it is called. The delete
manifests that reference that file are cached in `cachedNewDeleteManifests`,
but neither code path that discards those manifests deletes the Puffin file:
- **Cache invalidation**: when a subsequent `addDeletes` call sets
`hasNewDeleteFiles = true`, `newDeleteFilesAsManifests` deletes the cached
delete manifests but leaves the Puffin file written by the previous `mergeDVs`
call orphaned on disk.
- **Commit failure**: `cleanUncommittedAppends` deletes the uncommitted
delete manifests but never the Puffin files they reference.
Repro:
```java
RowDelta.addDeletes(dv1, dv2) -> apply()
// writes merged-dvs-...-1.puffin, caches delete manifests
-> addDeletes(dv3)
// invalidates cache: deletes manifest, but the puffin file is orphaned
-> commit()
```
The orphaned Puffin files accumulate in the table's data directory and can
only be cleaned up with `remove_orphan_files`. Committed data and read
correctness are not affected.
## Fix
- Track the Puffin locations written by `mergeDVs` in a new
`cachedMergedDVLocations` field, and delete them both when the delete manifest
cache is invalidated and when uncommitted appends are cleaned up after a failed
commit.
- The output location is resolved through the `FileIO` so that its scheme
matches `DeleteFile.location()`, which may differ from the raw output location
(e.g. a stripped `file:` scheme). The location is only tracked when `mergeDVs`
actually returned a DV backed by the Puffin file, so no delete is issued when
no merge was required.
## Tests
Added two regression tests in `TestRowDelta`:
- `testMergedDVPuffinFileCleanedUpOnCacheInvalidation` — reproduces the
repro sequence above and asserts the first Puffin file no longer exists after
the cache is invalidated, while the committed DV still deletes all 6 positions.
- `testMergedDVPuffinFileCleanedUpOnCommitFailure` — asserts the Puffin file
is cleaned up when the commit fails.
Both tests fail without the fix (`AssertionFailedError: [Merged DV Puffin
file should be deleted on ...]`) and pass with it.
## Credit
This revives the approach from #17309 by @wangyum, which was closed by the
stale bot without review. Credit for the original analysis and implementation
goes to @wangyum.
Fixes #17307
---
**AI Disclosure**
- Model: deepseek-v4.1-flash
- Platform/Tool: Hermes Agent (Nous Research)
- Human Oversight: partially reviewed
- Prompt Summary: Delete orphaned merged DV Puffin files on delete-manifest
cache invalidation and on commit failure
--
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]
