Expose OverwriteAction.set_snapshot_properties through the FFI by gbrgr · Pull Request #113 · RelationalAI/RustyIceberg.jl

gbrgr · 2026-09-14T07:32:33Z

Summary

Expose iceberg-rust's existing OverwriteAction::set_snapshot_properties through the FFI, so commits can carry custom snapshot summary tags (e.g. attempt_id, job_id) for reconciliation. No new iceberg-rust capability needed — the gap was entirely in this crate's FFI surface.
Added table_current_snapshot_summary to read summary properties back (needed for the tests to check actual values, not just absence of errors).
Bumped 0.9.3 → 0.9.4 (Project.toml + Cargo.toml).
Fixed this repo's own CI: docker/docker-compose.yml pulled minio/minio/minio/mc from Docker Hub, which no longer hosts them. Switched to quay.io/minio.

Usage

updated_table = with_transaction(table, catalog) do tx
    with_overwrite(tx) do action
        add_data_files(action, new_files)
        set_snapshot_properties(action, Dict("attempt_id" => attempt_id))
    end
end
table_current_snapshot_summary(updated_table)["attempt_id"]  # "..."

Test plan

New tests in test/overwrite_tests.jl cover write + read-back round-trip and error handling.
Full overwrite_tests.jl suite passes; no regressions in the rest of the suite.
CI green (Rust + both Julia versions).

Not yet done

Consuming this from raicode needs either a local Pkg.develop override or a Yggdrasil/JLL rebuild once merged.

🤖 Generated with Claude Code

Expose iceberg-rust's existing OverwriteAction::set_snapshot_properties (HashMap<String,String>) through the FFI, so callers can tag a commit's snapshot summary with identifiers like attempt_id / job_id / fencing_token at commit time -- needed for reconciliation (JC verifying a commit belongs to a given job) per the platform's Option B design. No new iceberg-rust capability needed -- OverwriteAction already supports this upstream. The gap was entirely in this crate's FFI surface: IcebergOverwriteAction only accumulated added/deleted files. - Rust: new snapshot_properties field on IcebergOverwriteAction, a new iceberg_overwrite_action_set_snapshot_properties FFI function (reusing the existing PropertyEntry/parse_properties pattern already used for catalog properties elsewhere in this crate), threaded into set_snapshot_properties(...) in the apply() builder chain. - Julia: set_snapshot_properties(action, Dict{String,String}) usable inside with_overwrite(tx) do action ... end, following the same PropertyEntry-array + GC.@preserve pattern as existing property-passing functions. - Verified caller-supplied keys survive a real commit (in-memory catalog) without disturbing iceberg-rust's own computed summary properties, which win on key collision (matches iceberg-java's SnapshotProducer.summary semantics -- verified directly in iceberg-rust's own transaction/snapshot.rs). - Bumped both Project.toml and iceberg_rust_ffi/Cargo.toml 0.9.3 -> 0.9.4, matching this repo's convention for FFI-surface-affecting changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

table_current_snapshot_summary(table) reads back a snapshot's summary properties (both caller-supplied ones like attempt_id and iceberg-rust's own computed metrics), mirroring table_schema's existing JSON-string FFI pattern. Lets the new set_snapshot_properties test actually assert the value landed correctly instead of only checking the commit didn't throw, and is generally useful for anything that needs to verify a commit's tags later (e.g. disambiguating a CAS conflict from a self-conflict). Also drop the `::T` return-type annotations on table_current_snapshot_summary and table_current_snapshot_id -- in Julia this isn't just documentation, it inserts an implicit convert(T, result) on every return, which is easy to forget and generally unidiomatic here since the union types are already what the function bodies naturally return. Separately: docker/docker-compose.yml pulled minio/minio:latest and minio/mc:latest straight from Docker Hub, which no longer hosts these images (same root cause as raicode's iceberg-up breakage) -- this was failing this repo's own CI on every PR. Fixed to pull from quay.io/minio instead (verified locally: full stack comes up healthy). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

bunjj

Checked this against the pinned rev: snapshot.rs:434 clones the caller props then extends with the collector, so the precedence claim holds. test.yml builds the FFI from source and overrides the JLL path, so green CI here actually covers the new symbols.

Two questions, neither blocking. The accessor only reads current_snapshot(), so "did attempt X land" stops working once anyone commits on top, which is the crash case. Is a by-snapshot-id lookup coming, or does the JC path not need it? And transaction.rs:238 is still bare add_data_files, so fast-append can't carry a tag. Deliberate?

bunjj · 2026-09-14T09:27:30Z

+function table_current_snapshot_summary(table::Table)
+    ptr = @ccall rust_lib.iceberg_table_current_snapshot_summary(table::Table)::Ptr{Cchar}
+    if ptr == C_NULL
+        return nothing


This returns nothing for two cases: no snapshot, and null/freed table pointer. table_schema right above throws STATE_RESOURCE_FREED for the second one. Can we match it? Otherwise a freed handle looks like "never committed", which is the one thing reconciliation shouldn't get wrong.

Fixed — added the null-table check, matching table_schema. Also rewrote the corresponding test to pass C_NULL directly instead of a freed table: Table is a raw Ptr{Cvoid}, so free_table can't null the caller's copy, making freed-then-reused UB throughout this file (not just here) rather than something this check can catch.

bunjj · 2026-09-14T09:27:30Z

+    let Some(snapshot) = table_ref.table.metadata().current_snapshot() else {
+        return ptr::null_mut();
+    };
+    match serde_json::to_string(&snapshot.summary().additional_properties) {


Summary is { operation, #[serde(flatten)] additional_properties }, so to_string(snapshot.summary()) gets you the same map plus "operation". Any reason to drop it? Seems useful for the same check.

No good reason, fixed — now serializing the whole summary() so operation is included.

bunjj · 2026-09-14T09:27:30Z

 }

+/// Get the current snapshot's summary properties (the caller-supplied ones set via
+/// `iceberg_overwrite_action_set_snapshot_properties`, e.g. attempt_id -- NOT


Nit: says it's the caller properties and not the computed metrics, then says the computed metrics are in there too. The Julia docstring has it right.

Fixed, trimmed to match the Julia docstring.

bunjj · 2026-09-14T09:27:30Z

+            @test !isnothing(summary)
+            @test summary["attempt_id"] == "test-attempt-123"
+            @test summary["added-data-files"] == "1"
+            @test summary["total-records"] == "2"


Nothing tests the collision behavior, and it rests on extend ordering in SnapshotProducer::summary, so a bump could flip it quietly. Worth passing "added-data-files" => "999" and asserting it's still "1".

Added — round-trip test now sets added-data-files => 999 and asserts the real computed value (1) wins.

…n test - Serialize the whole snapshot summary (including `operation`), not just additional_properties. - Add a null-table check to table_current_snapshot_summary, matching table_schema's precedent. - Test the null check against C_NULL directly, not a freed table -- Table is a raw Ptr{Cvoid} so free_table can't null the caller's copy, making freed-then-reused UB (same as every other Table-consuming function here), not something this check can catch. - Add a colliding key to the round-trip test to confirm computed values win over caller-supplied ones. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

gbrgr added the dismiss-release-notes label Sep 14, 2026

gbrgr marked this pull request as ready for review September 14, 2026 07:57

gbrgr requested a review from bunjj September 14, 2026 07:57

Trim comments to essentials

6ec94fb

bunjj approved these changes Sep 14, 2026

View reviewed changes

gbrgr enabled auto-merge (squash) September 14, 2026 09:48

gbrgr merged commit 6ccdd36 into main Sep 14, 2026
6 checks passed

gbrgr deleted the gb/overwrite-snapshot-properties branch September 14, 2026 09:52

gbrgr mentioned this pull request Sep 14, 2026

[iceberg_rust_ffi] Bump to 0.9.4 JuliaPackaging/Yggdrasil#14794

Merged

gbrgr mentioned this pull request Oct 20, 2026

Add assert_requirements bindings for pinned-base commit assertions #116

Open

3 tasks done

Navigation Menu

Repository navigation

Expose OverwriteAction.set_snapshot_properties through the FFI - #113

Expose OverwriteAction.set_snapshot_properties through the FFI#113
gbrgr merged 4 commits into
mainfrom
gb/overwrite-snapshot-properties

gbrgr commented Sep 14, 2026 •

edited

Loading

Uh oh!

bunjj left a comment

Uh oh!

bunjj Sep 14, 2026

Uh oh!

gbrgr Sep 14, 2026

Uh oh!

bunjj Sep 14, 2026

Uh oh!

gbrgr Sep 14, 2026

Uh oh!

bunjj Sep 14, 2026

Uh oh!

gbrgr Sep 14, 2026

Uh oh!

bunjj Sep 14, 2026

Uh oh!

gbrgr Sep 14, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

gbrgr commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Usage

Test plan

Not yet done

Uh oh!

bunjj left a comment

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!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gbrgr commented Sep 14, 2026 •

edited

Loading