ryanworl opened a new pull request, #1783: URL: https://github.com/apache/iceberg-go/pull/1783
## What `Snapshot.dataFiles` read manifest entries with `discardDeleted=false` and applied no status filter, so DELETED entries — tombstones recording that a file was removed — were yielded as if the files were still reachable. This PR flips that one flag to `true` (the manifest reader's existing `discardDeleted` support) and adds regression tests. ## Why Every caller of `dataFiles` wants live files only, so the leak made each one subtly wrong: - `AddFiles` / `AddDataFiles` duplicate walks reported false "already referenced" errors for a path whose previous incarnation was deleted (e.g. by a compaction), blocking a legitimate re-add. - The `ReplaceFiles`-family belong-to-table checks matched already-removed files, silently staging a redundant re-delete of a file the table no longer references. - `classifyFilesForDeletions`' full-table branch collected tombstoned files as deletion candidates. I audited every call site to make sure no caller wants tombstones (all are in `transaction.go`): | Call site | Wants | |---|---| | `ReplaceDataFiles` belong-to-table / already-referenced | live only | | `AddDataFiles` duplicate check | live only | | `ReplaceDataFilesWithDataFiles` belong-to-table | live only | | `ReplaceFiles` belong-to-table | live only | | `AddFiles` duplicate walk | live only | | `classifyFilesForDeletions` full-table branch | live only | Code that genuinely needs tombstones (conflict validation, `collectExistingDVs`) already uses the separate `Snapshot.entries` iterator and filters by status itself, so it's untouched. Scan planning already discards DELETED entries — this brings the transaction-side walks in line. This also matches Java, where the transaction/validation walks go through `ManifestReader.liveEntries()` / `ManifestGroup`, which filter DELETED entries by default. ## Behavior changes Both are bug-fix-shaped, but worth stating plainly: - Re-adding a path whose only head entry is a tombstone now succeeds instead of failing with a false "already referenced" error. - Asking the `ReplaceFiles` family to delete such a path now correctly fails with "cannot delete files that do not belong to the table" instead of silently staging a redundant re-delete. This matches Java's `BaseReplaceFiles` validation. ## Tests - `TestSnapshotDataFilesSkipsDeletedEntries` (new internal test): a snapshot whose data and delete manifests each carry ADDED, EXISTING, and DELETED entries yields only the live ones. - `TestAddFiles_ReAddsPathDeletedByRewrite`: a rewrite removes a file leaving a tombstone; re-adding the same path succeeds and the file is scannable again. Both fail on the pre-fix code. `go test ./...` and `golangci-lint run` are clean. Made with [Cursor](https://cursor.com) -- 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]
