Conversation
The snapshot summary could only count what the caller asked to remove, so an operation that skips a path it never had, or is handed the same path twice, inflated deleted-data-files and deleted-records. SnapshotProduceOperation now reports the entries it actually marked Deleted, each with the schema and partition spec of the manifest that recorded it, and the summary counts those. Two changes follow from it. The manifests are produced before the summary, since the counts are only known once the operation has run, which means the added data files are cloned rather than taken. And the full-table truncate that drops the previous totals is gated on a new operation hook instead of on the operation being Overwrite: a partial overwrite keeps the totals, and with truncate off update_totals needs a saturating subtraction so removing more than the previous total does not underflow.
The manifest name is built inline in new_manifest_writer from the metadata location, the commit uuid and a counter, so anything else that needs to write a manifest for the same commit has to reinvent it, and a name invented elsewhere is neither unique within the commit nor findable by orphan cleanup after a failed one. Pull the name out into new_manifest_path, and make the counter an AtomicU64 so it can be bumped behind a shared reference.
new_manifest_writer hardcoded the table's current schema and default partition spec, so an operation that rewrites an existing manifest could not keep the schema and spec that manifest's entries were recorded under, and building a writer by hand instead means losing the encryption manager the method already branches on: on an encrypted table the manifest would go out in plaintext while its key metadata said otherwise. The schema and partition spec become arguments, and the added-files path passes the table's current ones.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #3252.
What changes are included in this PR?
Three producer-level changes that #2185 needs, split out so they can land on their own (#3046 currently carries its own copy of the manifest-naming part on its separate producer):
SnapshotProduceOperationcan report the data files it actually removed, each with the schema and spec of the manifest that recorded it; the summary counts those instead of the caller's delete list, andupdate_totalsuses saturating subtraction (fix(spec): use saturating_sub in snapshot summary to prevent overflow panic #2144 tried this part). Manifests are produced before the summary so the operation has run by then.new_manifest_pathis derived from the commit uuid and that counter, so every manifest written during one commit gets a distinct name.new_manifest_writertakes the schema and partition spec to write under and goes through the table's encryption manager, so a rewritten manifest keeps the schema and spec of the manifest it replaces.FastAppendpasses the current schema and default spec as before.No public API change. #2620 discusses where retry-safe state like the counter should live long term; this keeps it on the producer as today.
Are these changes tested?
Three unit tests: the summary counts an operation's reported removals, totals do not underflow, and a manifest written with a given schema and spec reads back with them while two writers get distinct paths.
cargo test -p iceberg, fmt and clippy with-D warningsare clean.