feat(transaction): Add simple snapshot producer, and wire fast append by CTTY · Pull Request #3474 · apache/iceberg-rust

CTTY · 2026-10-16T05:17:57Z

Which issue does this PR close?

Closes **2. Persistent simple snapshot production** — extract stateless helpers, introduce SimpleSnapshotProducer, migrate FastAppendAction, remove the attempt-scoped SnapshotProducer and hook traits #3418 (milestone 2 of [EPIC] Stateful transactions and snapshot production (RFC-0003) #3266)

What changes are included in this PR?

Replaces the attempt-scoped SnapshotProducer and its hook traits with:

snapshot_helpers.rs: stateless functions for snapshot production (ids, paths, manifest writer, summary, manifest list + snapshot assembly). Shared with MergingSnapshotProducer later.
SimpleSnapshotProducer: retry-persistent state for FastAppendAction. Commit UUID and snapshot id are fixed across attempts, the added manifest is reused when schema and format version are unchanged, and each attempt writes its own manifest list. Previously every retry generated a new UUID and rewrote the manifest, orphaning the old one.

Are these changes tested?

Unit tests for manifest reuse across attempts and invalidation on schema/format change, plus a transaction-level test where a concurrent commit forces a rebase and the manifest is reused.

AI Disclosure

Used claude for the code, vetted by human eyes with a pair of glasses

CTTY · 2026-10-20T22:36:45Z

+use crate::{Error, ErrorKind};
+
+/// Retry-persistent state for an action that appends data files in a single
+/// new manifest.


I don't think this is accurate, there can be multiple new manifests if the manifest is too large(rolling), or there are files from different partition spec (grouping).

Looks like currently we don't support the above mechanism in rust? Will need to double check

the rolling mechanism can be added later, and I'm more concerned about the spec id grouping.

Looks like we just assume all new data files belong to the same partition spec (current default spec):

iceberg-rust/crates/iceberg/src/transaction/snapshot.rs

Line 339 in 219571a

self.table.metadata().default_partition_spec().clone(),

I think we can fix it, rather than just blocking them in the validation: https://github.com/apache/iceberg-rust/blame/main/crates/iceberg/src/transaction/snapshot.rs#L155-L164

Investigated this a bit and I think it's better to defer this. We have open multiple manifest writers using runtime if implemented correctly.

Rolling manifest writer: #3499
Group files by their spec ids: #3500

CTTY · 2026-10-21T00:52:49Z

cc @JanKaul

blackmwk

Requesting changes for the stale inherited sequence numbers described inline. All 104 existing transaction tests pass, but a focused manifest-reuse regression test returns sequence number 35 from the cache while a fresh read correctly returns 36. Including the inherited sequence number in the manifest cache key makes that test pass.

laskoviymishka

This needs some work, but it's close. Keeping the commit UUID and snapshot id fixed across attempts is the right direction for #3266. Two things need to land first. One is @blackmwk's cache-key fix (+1 to that thread). The other is a reuse check in added_manifests that covers the files and spec its manifest was written from. Today that check relies on the caller passing the same files and on the default-spec check that #3500 will remove.

blackmwk

Re-reviewed at 4f597f1. The expanded manifest cache key fixes my previous finding: the original warmed-cache/rebase reproduction now passes. The new v3 rebase test also covers moving the snapshot's row range after a concurrent append.

Validation: all 105 transaction tests, all 6 object-cache tests, and the original regression reproduction pass. I found no remaining blocking correctness issue. One non-blocking wording correction is noted inline.

Approved by mistake

CTTY · 2026-10-23T02:44:08Z

+    /// with sequence number 0 even when the manifest list entry referencing them
+    /// carries a nonzero one.
+    #[tokio::test]
+    async fn test_read_v1_manifest_defaults_sequence_number_to_zero() {


I was thinking about some cases that format version changes from V1 to V2 during retry attempts. Didn't find any issue, just think this test can be helpful to clarify things

blackmwk · 2026-10-23T06:42:58Z

nit: Could we have a snapshot dir mod, and rename this simple.rs in the snapshot mod?

blackmwk · 2026-10-23T07:25:22Z


-        // validate added files
-        snapshot_producer.validate_added_data_files()?;
+    async fn commit(&self, state: &mut Self::State, table: &Table) -> Result<ActionCommit> {


Suggested change

async fn commit(&self, state: &mut Self::State, table: &Table) -> Result<ActionCommit> {

async fn commit(&self, state: &mut SimpleSnapshotProducer, table: &Table) -> Result<ActionCommit> {

blackmwk · 2026-10-23T07:41:58Z

-        // validate added files
-        snapshot_producer.validate_added_data_files()?;
+    async fn commit(&self, state: &mut Self::State, table: &Table) -> Result<ActionCommit> {
+        let data_files = self.dedupe_added_files();


I think this is incorrect, why we need to do this in each retry? It only need once.

Yes I think the deduplication can be moved to the action

blackmwk · 2026-10-23T07:42:34Z

+    type State = SimpleSnapshotProducer;
+
+    fn new_state(&self) -> Self::State {
+        SimpleSnapshotProducer::new(self.commit_uuid)


I think you should pass all info to snapshot producer?

blackmwk · 2026-10-23T07:46:05Z

+    ) -> Result<()> {
+        let metadata = table.metadata();
+        for data_file in data_files {
+            if data_file.content_type() != DataContentType::Data {


This should only do once.

Currently I'm copying the existing logic. How about revisiting this in #3500? These logic can be refactored and optimized at one go

We can move every validation that does NOT depend on the current table to actions themselves

blackmwk · 2026-10-23T07:56:43Z

+        // TODO: Allowing snapshot property setup with no added data files is a workaround.
+        // We should clean it up after all necessary actions are supported.
+        // For details, please refer to https://github.com/apache/iceberg-rust/issues/1548
+        if data_files.is_empty() && properties.is_empty() {


Similarly, it should only do once.

CTTY commented Oct 20, 2026

View reviewed changes

This was referenced Oct 21, 2026

Roll added manifests at target size and write them in parallel #3499

Open

Group added data files by partition spec in SimpleSnapshotProducer #3500

Open

CTTY marked this pull request as ready for review October 21, 2026 00:36

CTTY added 3 commits October 20, 2026 17:38

Add simple snapshot producer, and wire fast append

b0d4485

Leave some todo

a95e716

port changes from 3363 to this PR

a06cb28

CTTY force-pushed the ctty/simple-sp branch from 7e77f8b to a06cb28 Compare October 21, 2026 00:43

CTTY requested review from blackmwk and laskoviymishka and removed request for laskoviymishka October 21, 2026 00:43

delete snapshot

f1361c5

blackmwk requested changes Oct 21, 2026

View reviewed changes

Comment thread crates/iceberg/src/transaction/simple_snapshot_producer.rs

laskoviymishka requested changes Oct 21, 2026

View reviewed changes

Comment thread crates/iceberg/src/transaction/append.rs Outdated

Comment thread crates/iceberg/src/transaction/simple_snapshot_producer.rs Outdated

Comment thread crates/iceberg/src/transaction/simple_snapshot_producer.rs

Comment thread crates/iceberg/src/transaction/snapshot_helpers.rs Outdated

CTTY mentioned this pull request Oct 21, 2026

Potential Cleanup for stateful transactions #3439

Open

CTTY added 2 commits October 21, 2026 15:26

Validate seq num and snapshot id in manifest cache

e86750e

address comments

4f597f1

CTTY requested review from blackmwk and laskoviymishka October 21, 2026 22:56

blackmwk previously approved these changes Oct 22, 2026

View reviewed changes

Comment thread crates/iceberg/src/transaction/simple_snapshot_producer.rs Outdated

CTTY added 4 commits October 22, 2026 19:03

update comments

6c71862

address v1 manifest seq num issue, remove AddedManifests

24fd624

cleanup

10c6b2e

clean up

57b984f

CTTY commented Oct 23, 2026

View reviewed changes

blackmwk reviewed Oct 23, 2026

View reviewed changes

	async fn commit(&self, state: &mut Self::State, table: &Table) -> Result<ActionCommit> {
	async fn commit(&self, state: &mut SimpleSnapshotProducer, table: &Table) -> Result<ActionCommit> {

Conversation

CTTY commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

Uh oh!

CTTY Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CTTY Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CTTY commented Oct 21, 2026

Uh oh!

blackmwk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blackmwk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CTTY commented Oct 16, 2026 •

edited

Loading

CTTY Oct 20, 2026 •

edited

Loading

CTTY Oct 20, 2026 •

edited

Loading