vbhanuchander-lang opened a new pull request, #17643:
URL: https://github.com/apache/iceberg/pull/17643
Closes #16662.
### The bug
`rewrite_table_path` fails with `FileNotFoundException` on a table where
position delete files have
been replaced and then expired — for example `rewrite_position_delete_files`
followed by
`expire_snapshots`.
In `RewriteTablePathUtil.writeDeleteFileEntry`, the `POSITION_DELETES`
branch adds every entry to
`toRewrite()` regardless of status:
```java
if (entry.isLive() && snapshotIds.contains(entry.snapshotId())) {
result.copyPlan().add(...); // correctly skips deleted entries
}
result.toRewrite().add(file.copy()); // unconditional
```
`toRewrite()` feeds `RewriteTablePathSparkAction.rewritePositionDeletes`,
which **opens each file**
to rewrite the data file paths embedded in it. A DELETED entry is a
tombstone: after
`expire_snapshots` the underlying `.parquet` is gone, so the read throws.
### The fix
Guard the `toRewrite()` add on `entry.isLive()`. The entry is still written
into the rewritten
manifest by `appendEntryWithFile` above, so the tombstone is preserved in
metadata — only the
attempt to read and rewrite its contents is skipped.
This makes the branch consistent with what the surrounding code already
assumes:
- the comment directly above it says to "keep the following entries in
metadata but exclude them
from copyPlan: 1) deleted position delete files";
- the size fallback added in #15470 says the original size is kept "for
entries that were not
rewritten (e.g. deleted entries not copied to the target)";
- and the existing test
`testRewriteDeleteManifestFallsBackToOriginalSizeForDeletedEntries` is
written around the same expectation.
So the deleted entry was already treated as "not rewritten" everywhere
except the line that puts it
on the rewrite list.
### Scope
I guarded on `entry.isLive()` only, not on the full `entry.isLive() &&
snapshotIds.contains(...)`
condition used by `copyPlan()`. Matching `copyPlan()` exactly would also
drop live entries belonging
to snapshots outside the requested range, which is arguably wasted work —
but those files do still
exist on disk, so they are not the reported failure, and skipping them would
change which sizes land
in `rewrittenDeleteFileSizes` for incremental rewrites. That felt like a
separate decision. Happy to
tighten it if you would prefer the two conditions to match.
### Test
One case added to `TestRewriteTablePathUtil`, reusing the existing
`deleteManifestWithLiveAndDeletedEntry` fixture: after rewriting a delete
manifest holding one live
and one deleted entry, `toRewrite()` must contain only the live file.
It fails without the production change and passes with it, across all
parameterised format versions:
```
TestRewriteTablePathUtil >
testRewriteDeleteManifestExcludesDeletedEntriesFromRewrite() > formatVersion =
2 FAILED
TestRewriteTablePathUtil >
testRewriteDeleteManifestExcludesDeletedEntriesFromRewrite() > formatVersion =
3 FAILED
TestRewriteTablePathUtil >
testRewriteDeleteManifestExcludesDeletedEntriesFromRewrite() > formatVersion =
4 FAILED
```
The existing delete-manifest tests are unaffected: the tombstone is still
written to the rewritten
manifest, so
`testRewriteDeleteManifestFallsBackToOriginalSizeForDeletedEntries` still sees
both
entries.
I have not added a Spark-level test reproducing the full
`rewrite_position_delete_files` → `expire_snapshots` → `rewrite_table_path`
sequence. The core test
pins the actual defect directly and cheaply; say the word if you would
rather have the end-to-end
version as well.
--
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]
