vbhanuchander-lang commented on PR #16669:
URL: https://github.com/apache/iceberg/pull/16669#issuecomment-5287970064
@venkateshwaracholan this was closed by the stale bot, not on merit — it had
@wombatu-kun's approval
and @mukund-thakur's "Seems like a correct fix to me". It is worth reopening.
I reached the same fix independently before finding this PR (I have since
closed mine, #17643), so
for whoever picks it up, here is the verification from a second pair of eyes:
- `writeDeleteFileEntry` guards `copyPlan()` with `entry.isLive() &&
snapshotIds.contains(...)` but
adds to `toRewrite()` unconditionally.
- `toRewrite()` is consumed by
`RewriteTablePathSparkAction.rewritePositionDeletes`, which **opens
each file** to rewrite the embedded data file paths. A DELETED entry is a
tombstone whose file may
already have been removed by `expire_snapshots`, hence the
`FileNotFoundException` in #16662.
- The surrounding code already assumes deleted entries are not rewritten:
the comment above says
they are "kept in metadata but excluded from copyPlan", and the size
fallback added in #15470
refers to "entries that were not rewritten (e.g. deleted entries not
copied to the target)". The
unconditional add is the odd one out.
- Guarding on `entry.isLive()` leaves `appendEntryWithFile` untouched, so
the tombstone is still
written into the rewritten manifest. I confirmed
`testRewriteDeleteManifestFallsBackToOriginalSize
ForDeletedEntries` still sees both entries.
On my branch a test over the `deleteManifestWithLiveAndDeletedEntry` fixture
asserting
`toRewrite()` holds only the live file failed without the change on format
versions 2, 3 and 4, and
the full `iceberg-core` suite passed with it (8124 tests). So the behaviour
is pinned and there is no
collateral damage in core.
One question a reviewer might still want answered, and the likely reason
this stalled: whether the
guard should be `entry.isLive()` alone or the full `entry.isLive() &&
snapshotIds.contains(...)` used
by `copyPlan()`. The narrow guard fixes the reported crash; matching
`copyPlan()` would additionally
skip live entries outside the requested snapshot range, which changes which
sizes land in
`rewrittenDeleteFileSizes` for incremental rewrites. Worth stating
explicitly either way.
@danielcweeks @mukund-thakur — this is a small, approved fix for a crash
that has now been reported
and independently rediscovered three times (#16669, #16755, and mine).
Reopening it seems better than
letting a fourth person write it.
--
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]
