ryanworl opened a new pull request, #1785: URL: https://github.com/apache/iceberg-go/pull/1785
> [!NOTE] > Stacked on #1783 and #1784 — the diff includes their commits until they merge. This PR's own commits are the last four (`Positions()` iterator, `RowDelta.RemoveDeletes`, and two review-feedback commits). ## What `RowDelta` gains `RemoveDeletes`, mirroring Java's `RowDelta#removeDeletes`: a v3 writer that replaces a data file's deletion vector can now remove the superseded DV entry in the same snapshot that adds the merged replacement. Supporting it, `RoaringPositionBitmap` gains a `Positions()` iterator (mirroring Java's `RoaringPositionBitmap#forEach`) so a writer can enumerate the previous DV's positions and merge them into the replacement. Happy to split `Positions()` into its own PR if preferred. ## Why The v3 spec makes supersession atomic: "there can be at most one deletion vector for a given data file in a snapshot. Writers must ensure that there is at most one deletion vector per data file and must merge new deletes with existing vectors or position delete files." Without `RemoveDeletes`, the public API can only supersede a DV in two snapshots — one adding the replacement, one removing the old entry. The intermediate snapshot has two live DVs for one data file, which the spec forbids: a reader entitled to assume at-most-one-DV can apply the stale one and resurrect deleted rows, and time travel to that snapshot is permanently wrong. A writer crash between the two commits leaves the table in that state for good. ## Design notes - **Resolve-then-validate.** Removals are resolved against the current snapshot's delete manifests, so validation and the produced DELETED entries work from the manifest's own entry (content type, referenced data file, spec ID, partition, sequence numbers) rather than the caller's copy, which may carry stale metadata. - **Validation.** Every removed file must be a live Puffin position-delete DV with an explicit `referenced_data_file`; every removed DV must have a replacement DV in the same delta; conversely, every live DV whose referenced data file gets a replacement must itself be removed; duplicate replacements for one data file are rejected; and a table already carrying duplicate live DVs for one data file (at the same path or different paths) is reported as corrupt rather than silently repaired. - **Producer routing.** A removal-carrying delta goes through the overwrite producer, which already knows how to drop entries from inherited delete manifests and record DELETED entries; its overwrite-specific default conflict validator is suppressed because `RowDelta` registers its own. Deltas without removals keep the fast-append path unchanged. - **Non-replayable.** Removal identity is snapshot-relative, so removal-carrying deltas set the no-replay flag from #1784: on a CAS conflict they fail with `ErrCommitFailed` instead of replaying, and the caller rebuilds the removals against a fresh table. ## Not in scope (deliberate) - Plain `AddDeletes` of a DV without removals still does not check for an existing live DV on the same data file: that would add a delete-manifest walk to every DV-adding commit, while the new check piggybacks on the walk that removals already pay for. - The replacement bitmap is not verified to be a position-superset of the removed DV — writer responsibility, matching Java. ## Tests - `TestRowDeltaDVSupersessionSingleSnapshot`: end-to-end happy path — one commit adds the merged DV and removes the superseded entry; exactly one live DV remains and the old positions carry over through `Positions()`. - `TestRowDeltaRemoveDeletesFailsInsteadOfReplaying`: a removal-carrying delta racing a peer makes exactly one commit attempt and fails with `ErrCommitFailed` (the no-replay behavior from #1784), leaving the peer's DV as the single live one. - Validation matrix: `RequiresReplacement` (including a removal-only delta), `RejectsSurvivingLiveDV` (replacement added without removing the live DV, and duplicate replacements for one data file), `CorruptDuplicateLiveDVs` (both the same-path and two-path corruption shapes), `RequiresV3`, `RejectsNonDV`, `StaleReference`, `SharedPuffin` (removing one of two DVs stored in the same Puffin file leaves the other live), `UnknownFile`. - `Positions()`: empty bitmap, a single position beyond the 32-bit boundary, ascending order across container keys, and early termination by the consumer. `go test ./table/...` 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]
