feat(transaction): record the Iceberg-Rust writer version information in snapshot summaries by RussellSpitzer · Pull Request #3363 · apache/iceberg-rust

RussellSpitzer · 2026-10-06T18:39:20Z

Which issue does this PR close?

What changes are included in this PR?

Snapshots committed by iceberg-rust now record the library that wrote them:

iceberg-version = Apache Iceberg Rust <crate version>, always

engine-name and engine-version are left to the caller. Those name the embedding engine, which this library does not know. A value supplied through set_snapshot_properties is kept. This matches Java and Go, whose default environment context contains only iceberg-version. PyIceberg also writes engine-name and engine-version (apache/iceberg-python#3441).

Other caller properties are unchanged. Existing snapshots are left alone.

Are these changes tested?

Yes. A unit test on FastAppendAction checks that a plain append records iceberg-version.

cargo test -p iceberg --lib writer_identity and cargo clippy -p iceberg --lib --tests -- -D warnings pass.

AI Disclosure

Grok 4.7 in Cursor wrote the implementation, tests, and this description. The change has not been reviewed by a human yet.

A commit from iceberg-rust was indistinguishable from any other client that omits writer identity. Stamp engine-name, engine-version, and iceberg-version on every snapshot so mixed-writer tables can attribute the commit. Generated-by: Cursor Grok 4.7

Generated-by: Cursor Grok 4.7

iceberg-version still identifies this library. engine-name and engine-version are filled only when the caller did not set them, so an embedding engine can record itself through snapshot properties. Generated-by: Cursor Grok 4.7

A data file and an unrelated property do not affect those keys. Also list the new summary constants alphabetically. Generated-by: Cursor Grok 4.7

RussellSpitzer · 2026-10-06T19:12:50Z

+        let tx = Transaction::new(&table);
+
+        let mut snapshot_properties = HashMap::new();
+        snapshot_properties.insert("key".to_string(), "val".to_string());


Exists to force a commit

RussellSpitzer · 2026-10-06T19:24:09Z

+const ICEBERG_VERSION_PROP: &str = "iceberg-version";
+
+fn apply_writer_identity(properties: &mut HashMap<String, String>) {
+    let version = env!("CARGO_PKG_VERSION");


I don't know the convention here, but the LLMs tell me that this could be a crate level constant but recommended not doing that if we are only using it here.

This is correct, when release a new version we will bump versions for all crates and they should be consistent: https://github.com/apache/iceberg-rust/blob/main/Cargo.toml#L91-L98

Copilot

Copilot review overview

🟢 Approval recommended

The focused implementation and tests match the requested metadata precedence, with no blocking issues identified.

Review effort: Balanced
Findings: None

What changed in this PR

Adds writer identity to snapshot summaries so clients can identify snapshots created by iceberg-rust.

Changes:

Defaults engine identity while preserving caller overrides; always records the library version.
Adds tests for default identity and override precedence.

File	Description
crates/iceberg/src/transaction/snapshot.rs	Applies writer identity after summary aggregation.
crates/iceberg/src/transaction/append.rs	Tests default metadata and caller override handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

CTTY · 2026-10-06T22:59:07Z

+const ICEBERG_VERSION_PROP: &str = "iceberg-version";
+
+fn apply_writer_identity(properties: &mut HashMap<String, String>) {
+    let version = env!("CARGO_PKG_VERSION");


This is correct, when release a new version we will bump versions for all crates and they should be consistent: https://github.com/apache/iceberg-rust/blob/main/Cargo.toml#L91-L98

engine-name and engine-version name the embedding engine, which this library does not know. Callers can still set them. iceberg-version remains the library identity, matching Java and Go. Generated-by: Cursor Grok 4.7

Those keys are ordinary snapshot properties now, so preserving a caller value is already covered by the existing property test. Generated-by: Cursor Grok 4.7

The snapshot summary no longer writes those keys, so the test only checks iceberg-version. Generated-by: Cursor Grok 4.7

mbutrovich

Thanks @RussellSpitzer, just minor stuff at this point from me.

mbutrovich · 2026-10-07T15:48:21Z

 use crate::transaction::ActionCommit;
 use crate::{Error, ErrorKind, TableRequirement, TableUpdate};

+const ICEBERG_VERSION_PROP: &str = "iceberg-version";


Should ICEBERG_VERSION_PROP live in spec/snapshot_summary.rs with the other summary keys (snapshot_summary.rs#L27-L50)? That module defines every other key the library writes into additional_properties.

Co-authored-by: Matt Butrovich <mbutrovich@users.noreply.github.com>

The transaction path still writes the value. The property name now lives with the rest of the summary keys the library defines. Generated-by: Cursor Grok 4.7

The key stays private next to the other summary keys, and the commit path calls set_iceberg_version after the totals are computed. Generated-by: Cursor Grok 4.7

mbutrovich

Thanks @RussellSpitzer!

anoopj

LGTM. Left a couple of comments.

laskoviymishka

lgtm — nice that set_iceberg_version sits in SnapshotProducer::summary(), so every producer-based action picks it up for free. And overwriting a caller-supplied iceberg-version is the right call: Java (SnapshotProducer.summary applies EnvironmentContext.get() last) and Go (rebaseSummary) both apply environment context after user properties, so the library value wins.

A few notes, none blocking:

The PR description says a value supplied through set_snapshot_properties is kept, but the code (and the new test) overwrite it. The code's doing the right thing here — it's the description that's backwards, so I'd just fix the body, and maybe add a /// on the helper noting it always overwrites.
The version string puts Rust where Java/Go put the version number, which breaks an Apache Iceberg <ver> prefix match across clients — left an inline with a suggestion.
One stray blank-line drop in the imports — minor, flagged inline.

CTTY

LGTM!

Could you restore the deleted blank like mentioned above? Would love to keep the format clean before merging

mbutrovich

LGTM!

Could you restore the deleted blank like mentioned above? Would love to keep the format clean before merging

If a committer applies a suggestion that's one way to resolve it without the author. I think this would work.

Resolve the snapshot.rs conflict with the truncate_full_table() change, and commit the iceberg-version test through commit_with_fresh_state since TransactionAction is stateful after apache#3265. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

kevinjqliu

LGTM

i tested this locally by writing and it works!

{
  "operation": "append",
  "iceberg-version": "Apache Iceberg Rust 0.10.1",
  "added-data-files": "1",
  "added-records": "3",
  "added-files-size": "890",
  "total-data-files": "1",
  "total-records": "3",
  "total-files-size": "890",
  "total-delete-files": "0",
  "total-position-deletes": "0",
  "total-equality-deletes": "0"
}

kevinjqliu · 2026-10-20T03:01:28Z

thanks for the reviews, everyone!

RussellSpitzer added 4 commits October 6, 2026 13:38

Remove the writer-identity comments.

bc2db59

Generated-by: Cursor Grok 4.7

Keep a caller-supplied engine name and version.

0742543

iceberg-version still identifies this library. engine-name and engine-version are filled only when the caller did not set them, so an embedding engine can record itself through snapshot properties. Generated-by: Cursor Grok 4.7

Keep the writer-identity tests on snapshot properties.

a7dc0ce

A data file and an unrelated property do not affect those keys. Also list the new summary constants alphabetically. Generated-by: Cursor Grok 4.7

RussellSpitzer changed the title ~~feat(transaction): record the Rust writer in snapshot summaries~~ feat(transaction): record the Iceberg-Rust writer version information in snapshot summaries Oct 6, 2026

RussellSpitzer commented Oct 6, 2026

View reviewed changes

RussellSpitzer marked this pull request as ready for review October 6, 2026 19:22

Copilot AI balanced review requested due to automatic review settings October 6, 2026 19:22

Copilot started reviewing on behalf of RussellSpitzer October 6, 2026 19:23

View session

RussellSpitzer commented Oct 6, 2026

View reviewed changes

Copilot AI reviewed Oct 6, 2026

View reviewed changes

CTTY reviewed Oct 6, 2026

View reviewed changes

RussellSpitzer added 3 commits October 7, 2026 10:23

Write only iceberg-version into snapshot summaries.

78a92e8

engine-name and engine-version name the embedding engine, which this library does not know. Callers can still set them. iceberg-version remains the library identity, matching Java and Go. Generated-by: Cursor Grok 4.7

Drop the caller engine-identity snapshot test.

ba05028

Those keys are ordinary snapshot properties now, so preserving a caller value is already covered by the existing property test. Generated-by: Cursor Grok 4.7

Stop asserting that engine identity keys are absent.

3105575

The snapshot summary no longer writes those keys, so the test only checks iceberg-version. Generated-by: Cursor Grok 4.7

mbutrovich reviewed Oct 7, 2026

View reviewed changes

RussellSpitzer and others added 4 commits October 7, 2026 11:07

Update crates/iceberg/src/transaction/append.rs

dc2086e

Co-authored-by: Matt Butrovich <mbutrovich@users.noreply.github.com>

Move the iceberg-version summary key next to the other keys.

42578ee

The transaction path still writes the value. The property name now lives with the rest of the summary keys the library defines. Generated-by: Cursor Grok 4.7

Merge the accepted iceberg-version test suggestion.

5b9c042

Set iceberg-version from the snapshot summary module.

0679bba

The key stays private next to the other summary keys, and the commit path calls set_iceberg_version after the totals are computed. Generated-by: Cursor Grok 4.7

RussellSpitzer commented Oct 7, 2026

View reviewed changes

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

mbutrovich approved these changes Oct 7, 2026

View reviewed changes

anoopj reviewed Oct 9, 2026

View reviewed changes

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

anoopj reviewed Oct 9, 2026

View reviewed changes

Comment thread crates/iceberg/src/spec/snapshot_summary.rs

anoopj reviewed Oct 9, 2026

View reviewed changes

anoopj approved these changes Oct 9, 2026

View reviewed changes

laskoviymishka approved these changes Oct 10, 2026

View reviewed changes

Comment thread crates/iceberg/src/spec/snapshot_summary.rs

Comment thread crates/iceberg/src/spec/snapshot_summary.rs

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

CTTY approved these changes Oct 13, 2026

View reviewed changes

mbutrovich reviewed Oct 19, 2026

View reviewed changes

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

Restore the blank line before the SnapshotProduceOperation docs.

0fd484e

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

kevinjqliu approved these changes Oct 20, 2026

View reviewed changes

kevinjqliu enabled auto-merge October 20, 2026 03:01

Conversation

RussellSpitzer commented Oct 6, 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!

RussellSpitzer Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

CTTY Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Uh oh!

CTTY Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

mbutrovich Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

anoopj left a comment

Choose a reason for hiding this comment

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CTTY left a comment • 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!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu commented Oct 20, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RussellSpitzer commented Oct 6, 2026 •

edited

Loading

CTTY left a comment •

edited

Loading