vishnuprakaz opened a new pull request, #17630:
URL: https://github.com/apache/iceberg/pull/17630
If a conversion cycle fails after it has resolved an equality delete, the
rows that delete was hiding come back and they stay back. The task reports
success, so nothing looks wrong.
To reproduce it the table has two rows. I commit an equality delete for
each of them in one snapshot, so both rows should be invisible, then remove row
two's delete file from disk. The cycle resolves row one's delete and aborts
when it cannot read row two's, so nothing is committed. I put the missing file
back and trigger again. The cycle succeeds this time, but row one is visible
again:
```
TestConvertEqualityDeletes > testDeleteResolvedBeforeFailureIsRetained()
FAILED
org.opentest4j.AssertionFailedError:
expected: []
but was: [Record(1, a)]
```
Row one's delete had already resolved during the failed cycle, and that is
the one that gets lost. Row two's delete never got that far, and it converts
correctly on the retry. That asymmetry is the bug. Resolving an equality delete
consumes the index entries it matched, and a failed cycle leaves them consumed.
Nothing puts them back, because the target branch never moved, and both the
per-key clear and the planner's reindex only trigger when it does. So the retry
resolves the same
delete against an index that no longer holds those rows, writes no
deletion vector, and the committer then removes the equality delete file and
marks the snapshot converted.
The planner already rebuilds the index when an external commit has moved
the target branch. This is that situation without the move, so the planner now
remembers which staging snapshot its last plan covered. If it picks the same
one again, that cycle never committed, and the index is rebuilt before the
delete phase.
The snapshot id is checkpointed so a restore taken mid-cycle is covered
too, and it is recorded only once the staging inputs have validated.
Two planner tests ran several triggers without simulating the converter's
own commit, so their no-re-emission assertions no longer hold once the index is
rebuilton a replan. Both now simulate that commit, the way
`noMainReEmitAfterOwnCommit` does, which made one of them identical to it, so I
folded it in happy to restore it if you would rather keep both.
Also updates two statements in the maintenance docs that this change makes
out of date. Needs a backport to 2.0 and 1.20.
The alternative I considered was making resolution non-destructive in the
index, which changes what three of the PKIndex tests assert.
This is the smallest fix I found that leaves the index semantics as they
are.
Happy to rework on this if there is a better way.
--
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]
