vgkowski opened a new pull request, #17514:
URL: https://github.com/apache/iceberg/pull/17514
Closes #17498.
This targets `1.11.x` only. `main` is not affected: there,
`positionDeletesToRewrite()` collects the files to rewrite from live manifest
entries, so the bug does not exist and there is no corresponding `main` PR to
backport from.
## Problem
`RewriteTablePathUtil.writeDeleteFileEntry` applies a filter when building
the copy plan: only entries that are live and added within the requested delta
are copied. Entries that are `DELETED` (history markers) or outside the delta
are still written into the rewritten manifest, but excluded from the copy plan.
The physical-rewrite set did not apply the same filter — every position
delete entry was queued unconditionally:
```java
if (entry.isLive() && snapshotIds.contains(entry.snapshotId())) {
result.copyPlan().add(...);
}
result.toRewrite().add(file.copy()); // unconditional
```
Position delete files must be physically rewritten because they embed
absolute data file paths, so `RewriteTablePathSparkAction` opens every file in
`toRewrite`. When the entry is a `DELETED` marker whose underlying file was
already removed by `expire_snapshots`, the open fails and the whole procedure
aborts with `NotFoundException` / `NoSuchKey` — even though that file was never
going to be copied.
Because the `DELETED` entry is baked into the current snapshot's delete
manifest, the failure is not transient: every subsequent `rewrite_table_path`
run fails the same way, breaking both full rewrites of such a table and
incremental rewrites whose delta spans the expiry.
## Fix
Align the physical-rewrite set with the copy plan: an entry that is not
copied is kept in the rewritten manifest exactly as the source has it, and is
no longer opened. This makes the position-delete branch consistent with the
data-file and equality-delete branches, which already behave this way.
This is safe because `DELETED` entries are never opened by readers (they
exist only so the manifest history stays consistent), and out-of-delta
`EXISTING` entries point at files the target already has from a previous
incremental run. A live, still-needed delete file cannot be skipped as a result
of expiry, since `expire_snapshots` never deletes a file that remains reachable
from a retained snapshot.
Side effect worth noting: `rewrittenDeleteFilePathsCount` in the action
result now only counts files that are actually copied. It previously also
counted dead entries and entries outside the delta, which were staged and then
discarded.
## Tests
Two regression tests, added to all Spark versions (3.4, 3.5, 4.0, 4.1),
which share the fixed core code path but run their CI independently:
- `rewriteAfterExpiringDeletedPositionDeleteFile` — a deletion vector is
dropped by compaction, leaving a `DELETED` entry in the current snapshot's
delete manifest, and `expire_snapshots` then removes the file from storage. A
full rewrite must not attempt to physically rewrite that entry. Without the fix
this fails with `NotFoundException` from `PuffinReader`.
- `incrementalRewriteAfterExpiringDeletedPositionDeleteFile` — covers the
second failure mode: an incremental rewrite whose `start..end` delta spans
compaction and expiry of a position delete file must keep the surviving entry
out of the physical-rewrite set.
Both assert `rewrittenDeleteFilePathsCount()` and compare target rows
against source rows. They are limited to format version 3 and later: in v2,
compaction does not remove the position delete file from storage, so the
scenario does not arise.
--
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]
