From 831938537c064ef5b01c33aa59ebba33f3f77df2 Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Mon, 14 Sep 2026 09:31:58 +0200 Subject: [PATCH 1/4] feat: OverwriteAction.set_snapshot_properties Expose iceberg-rust's existing OverwriteAction::set_snapshot_properties (HashMap) 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 --- Project.toml | 2 +- iceberg_rust_ffi/Cargo.lock | 2 +- iceberg_rust_ffi/Cargo.toml | 2 +- iceberg_rust_ffi/src/transaction.rs | 56 +++++++++++++++++++++++++++-- src/RustyIceberg.jl | 1 + src/transaction.jl | 32 +++++++++++++++++ test/overwrite_tests.jl | 44 +++++++++++++++++++++++ 7 files changed, 134 insertions(+), 5 deletions(-) diff --git a/Project.toml b/Project.toml index 8534986..a0d7a06 100644 --- a/Project.toml +++ b/Project.toml @@ -1,6 +1,6 @@ name = "RustyIceberg" uuid = "390bdf5b-b624-43dc-a846-0ef7a3405804" -version = "0.9.3" +version = "0.9.4" [deps] Arrow = "69666777-d1a9-59fb-9406-91d4454c9d45" diff --git a/iceberg_rust_ffi/Cargo.lock b/iceberg_rust_ffi/Cargo.lock index 2019a86..5d04ce0 100644 --- a/iceberg_rust_ffi/Cargo.lock +++ b/iceberg_rust_ffi/Cargo.lock @@ -1878,7 +1878,7 @@ dependencies = [ [[package]] name = "iceberg_rust_ffi" -version = "0.9.3" +version = "0.9.4" dependencies = [ "anyhow", "arrow-array", diff --git a/iceberg_rust_ffi/Cargo.toml b/iceberg_rust_ffi/Cargo.toml index 5c26cbb..3d8b6e0 100644 --- a/iceberg_rust_ffi/Cargo.toml +++ b/iceberg_rust_ffi/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "iceberg_rust_ffi" -version = "0.9.3" +version = "0.9.4" edition = "2021" [lib] diff --git a/iceberg_rust_ffi/src/transaction.rs b/iceberg_rust_ffi/src/transaction.rs index 4efa95e..c0f8d98 100644 --- a/iceberg_rust_ffi/src/transaction.rs +++ b/iceberg_rust_ffi/src/transaction.rs @@ -7,9 +7,11 @@ use crate::catalog::IcebergCatalog; use crate::error_codes::{classified_error, classify_iceberg, IcebergErrorCode}; use crate::response::IcebergBoxedResponse; use crate::table::IcebergTable; -use crate::IcebergDataFiles; +use crate::util::parse_properties; +use crate::{IcebergDataFiles, PropertyEntry}; use iceberg::spec::DataFile; use iceberg::transaction::{ApplyTransactionAction, Transaction}; +use std::collections::HashMap; // FFI exports use object_store_ffi::{ @@ -80,6 +82,11 @@ impl IcebergFastAppendAction { pub struct IcebergOverwriteAction { added_files: Vec, deleted_files: Vec, + /// Custom snapshot summary properties (e.g. attempt_id, job_id, fencing_token) + /// to be recorded on the commit for reconciliation. Merged with iceberg-rust's + /// own computed summary properties (added/removed file counts, etc.) -- this + /// map only ever adds caller-supplied keys, it never overrides computed ones. + snapshot_properties: HashMap, } unsafe impl Send for IcebergOverwriteAction {} @@ -363,6 +370,49 @@ pub extern "C" fn iceberg_overwrite_action_add_data_files( 0 } +/// Set custom snapshot summary properties (e.g. attempt_id, job_id, fencing_token) to be +/// recorded on the commit this action produces. Merged with iceberg-rust's own computed +/// summary properties, which win on key collision -- callers cannot use this to override +/// computed metrics such as `added-data-files`. +/// +/// Copies `properties`; the caller retains ownership of it. Safe to call multiple times +/// before `apply` -- later calls add to (and override, on key collision) earlier ones. +/// Returns 0 on success, non-zero on error. +#[no_mangle] +pub extern "C" fn iceberg_overwrite_action_set_snapshot_properties( + action: *mut IcebergOverwriteAction, + properties: *const PropertyEntry, + properties_len: usize, + error_message_out: *mut *mut std::ffi::c_char, +) -> i32 { + let set_error = |msg: &str, out: *mut *mut std::ffi::c_char| { + if !out.is_null() { + if let Ok(c_str) = std::ffi::CString::new(msg) { + unsafe { + *out = c_str.into_raw(); + } + } + } + }; + if action.is_null() { + set_error("Null action pointer provided", error_message_out); + return 1; + } + let props: HashMap = match parse_properties(properties, properties_len) { + Ok(props) => props, + Err(e) => { + set_error( + &format!("Failed to parse snapshot properties: {}", e), + error_message_out, + ); + return 1; + } + }; + let action_ref = unsafe { &mut *action }; + action_ref.snapshot_properties.extend(props); + 0 +} + /// Mark data files for deletion in the overwrite snapshot. /// /// The data_files handle is consumed — its files are moved into the action. @@ -431,6 +481,7 @@ pub extern "C" fn iceberg_overwrite_action_apply( let added = std::mem::take(&mut action_ref.added_files); let deleted = std::mem::take(&mut action_ref.deleted_files); + let snapshot_properties = std::mem::take(&mut action_ref.snapshot_properties); let tx = match tx_ref.take() { Some(t) => t, @@ -450,7 +501,8 @@ pub extern "C" fn iceberg_overwrite_action_apply( let overwrite_action = tx .overwrite() .add_data_files(added) - .delete_data_files(deleted); + .delete_data_files(deleted) + .set_snapshot_properties(snapshot_properties); match overwrite_action.apply(tx) { Ok(new_tx) => { diff --git a/src/RustyIceberg.jl b/src/RustyIceberg.jl index d75c336..ef2dc1c 100644 --- a/src/RustyIceberg.jl +++ b/src/RustyIceberg.jl @@ -49,6 +49,7 @@ export IcebergString, IcebergUuid, IcebergBinary, IcebergDecimal export Transaction, DataFiles, free_transaction!, free_data_files!, commit, transaction, data_file_info export FastAppendAction, free_fast_append_action!, add_data_files, apply, with_fast_append export OverwriteAction, free_overwrite_action!, delete_data_files, with_overwrite, list_data_files +export set_snapshot_properties export DataFileWriter, free_writer!, close_writer, set_encode_workers! export WriterConfig, CompressionCodec, UNCOMPRESSED, SNAPPY, GZIP, LZ4, ZSTD, LZ4_RAW export RowChunk, flush! diff --git a/src/transaction.jl b/src/transaction.jl index 1996be8..a2073d9 100644 --- a/src/transaction.jl +++ b/src/transaction.jl @@ -417,6 +417,38 @@ function add_data_files(action::OverwriteAction, data_files::DataFiles) return nothing end +""" + set_snapshot_properties(action::OverwriteAction, properties::Dict{String,String}) + +Set custom snapshot summary properties (e.g. `attempt_id`, `job_id`, `fencing_token`) to +be recorded on the commit this action produces, for reconciliation. Merged with +iceberg-rust's own computed summary properties, which win on key collision -- this cannot +be used to override computed metrics such as `added-data-files`. + +Safe to call multiple times before the action is applied; later calls add to (and +override, on key collision) earlier ones. +""" +function set_snapshot_properties(action::OverwriteAction, properties::Dict{String,String}) + if action.ptr == C_NULL + throw(IcebergException(STATE_RESOURCE_FREED, "Resource has been freed", "OverwriteAction has been freed")) + end + property_entries = [PropertyEntry(pointer(k), pointer(v)) for (k, v) in properties] + properties_len = length(property_entries) + error_message_ptr = Ref{Ptr{Cchar}}(C_NULL) + result = GC.@preserve properties property_entries begin + @ccall rust_lib.iceberg_overwrite_action_set_snapshot_properties( + action.ptr::Ptr{Cvoid}, + (properties_len > 0 ? pointer(property_entries) : C_NULL)::Ptr{PropertyEntry}, + properties_len::Csize_t, + error_message_ptr::Ref{Ptr{Cchar}} + )::Cint + end + if result != 0 + parse_and_throw(error_message_ptr[], "overwrite set_snapshot_properties") + end + return nothing +end + """ delete_data_files(action::OverwriteAction, data_files::DataFiles) diff --git a/test/overwrite_tests.jl b/test/overwrite_tests.jl index 98a82f4..73b92fa 100644 --- a/test/overwrite_tests.jl +++ b/test/overwrite_tests.jl @@ -406,3 +406,47 @@ end end println("✅ OverwriteAction error handling tests passed") end + +# --------------------------------------------------------------------------- +# set_snapshot_properties +# --------------------------------------------------------------------------- + +@testset "set_snapshot_properties round-trips through a real commit" begin + mktempdir() do warehouse + cat = catalog_create_memory(warehouse) + table = C_NULL + updated = C_NULL + try + create_namespace(cat, ["ns"]) + table = create_table(cat, ["ns"], "t", _ow_schema()) + + files = RustyIceberg.with_data_file_writer(table) do w + write(w, (id=Int64[1, 2], value=[1.0, 2.0])) + end + + # Must not throw, and must not prevent the commit from landing. + updated = RustyIceberg.with_transaction(table, cat) do tx + with_overwrite(tx) do action + add_data_files(action, files) + set_snapshot_properties(action, Dict("attempt_id" => "test-attempt-123")) + end + end + + @test !isnothing(table_current_snapshot_id(updated)) + data = read_table_data(updated) + @test sort(data.id) == [1, 2] + finally + table != C_NULL && free_table(table) + updated != C_NULL && free_table(updated) + free_catalog!(cat) + end + end + println("✅ set_snapshot_properties round-trips through a real commit") +end + +@testset "set_snapshot_properties on a freed action throws" begin + action = RustyIceberg.OverwriteAction() + free_overwrite_action!(action) + @test_throws RustyIceberg.IcebergException set_snapshot_properties(action, Dict("k" => "v")) + println("✅ set_snapshot_properties on a freed action throws") +end From 52a30ebdada5efa12e2e23fa081f4738259234e1 Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Mon, 14 Sep 2026 09:52:45 +0200 Subject: [PATCH 2/4] Add read-side accessor for snapshot summary; fix CI's own MinIO pull 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 --- docker/docker-compose.yml | 4 ++-- iceberg_rust_ffi/src/table.rs | 26 ++++++++++++++++++++++++++ src/RustyIceberg.jl | 25 +++++++++++++++++++++++-- test/overwrite_tests.jl | 9 ++++++++- 4 files changed, 59 insertions(+), 5 deletions(-) diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml index cb90a52..4e25d79 100644 --- a/docker/docker-compose.yml +++ b/docker/docker-compose.yml @@ -4,7 +4,7 @@ networks: services: minio: container_name: minio - image: minio/minio:latest + image: quay.io/minio/minio:latest ports: - "9000:9000" # MinIO API port - "9001:9001" # MinIO Console port @@ -24,7 +24,7 @@ services: retries: 5 setup_bucket: - image: minio/mc:latest + image: quay.io/minio/mc:latest depends_on: minio: condition: service_healthy diff --git a/iceberg_rust_ffi/src/table.rs b/iceberg_rust_ffi/src/table.rs index 56d337a..046aacf 100644 --- a/iceberg_rust_ffi/src/table.rs +++ b/iceberg_rust_ffi/src/table.rs @@ -453,6 +453,32 @@ pub extern "C" fn iceberg_table_schema(table: *mut IcebergTable) -> *mut c_char } } +/// Get the current snapshot's summary properties (the caller-supplied ones set via +/// `iceberg_overwrite_action_set_snapshot_properties`, e.g. attempt_id -- NOT +/// iceberg-rust's own computed metrics like `added-data-files`, `total-records`, etc, +/// which are also present in the same map and can only diverge from a caller-supplied +/// key if the caller tried to reuse one of those reserved names) as a JSON object +/// string (`{"key": "value", ...}`). +/// +/// Returns null if the table has no current snapshot. +#[no_mangle] +pub extern "C" fn iceberg_table_current_snapshot_summary(table: *mut IcebergTable) -> *mut c_char { + if table.is_null() { + return ptr::null_mut(); + } + let table_ref = unsafe { &*table }; + let Some(snapshot) = table_ref.table.metadata().current_snapshot() else { + return ptr::null_mut(); + }; + match serde_json::to_string(&snapshot.summary().additional_properties) { + Ok(json) => match std::ffi::CString::new(json) { + Ok(c_str) => c_str.into_raw(), + Err(_) => ptr::null_mut(), + }, + Err(_) => ptr::null_mut(), + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/RustyIceberg.jl b/src/RustyIceberg.jl index ef2dc1c..f374c79 100644 --- a/src/RustyIceberg.jl +++ b/src/RustyIceberg.jl @@ -23,7 +23,7 @@ export INTERNAL export new_incremental_scan, free_incremental_scan! export scan_incremental_nested!, nested_incremental_arrow_stream export table_open, free_table, new_scan, free_scan! -export table_location, table_uuid, table_format_version, table_last_sequence_number, table_last_updated_ms, table_current_snapshot_id, table_schema +export table_location, table_uuid, table_format_version, table_last_sequence_number, table_last_updated_ms, table_current_snapshot_id, table_schema, table_current_snapshot_summary export IcebergPerfConfig export select_columns!, with_snapshot_id! export with_file_column!, with_pos_column! @@ -641,7 +641,7 @@ Get the current snapshot ID of an Iceberg table. Returns the snapshot ID if the table has at least one committed snapshot, or `nothing` if the table has no snapshots yet (e.g. immediately after creation, before any commit). """ -function table_current_snapshot_id(table::Table)::Union{Int64,Nothing} +function table_current_snapshot_id(table::Table) id = @ccall rust_lib.iceberg_table_current_snapshot_id(table::Table)::Int64 return id == -1 ? nothing : id end @@ -668,4 +668,25 @@ function table_schema(table::Table) return result end +""" + table_current_snapshot_summary(table::Table)::Union{Dict{String,String},Nothing} + +Get the current snapshot's summary properties -- both caller-supplied ones (set via +`set_snapshot_properties` on the `OverwriteAction` that produced this snapshot, e.g. +`attempt_id`) and iceberg-rust's own computed metrics (e.g. `added-data-files`, +`total-records`). A caller-supplied key is only overridden if it collides with one of +these computed key names. + +Returns `nothing` if the table has no current snapshot yet. +""" +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 + end + json = unsafe_string(ptr) + @ccall rust_lib.iceberg_destroy_cstring(ptr::Ptr{Cchar})::Cint + return Dict{String,String}(JSON.parse(json)) +end + end # module RustyIceberg diff --git a/test/overwrite_tests.jl b/test/overwrite_tests.jl index 73b92fa..dea7cbc 100644 --- a/test/overwrite_tests.jl +++ b/test/overwrite_tests.jl @@ -424,7 +424,6 @@ end write(w, (id=Int64[1, 2], value=[1.0, 2.0])) end - # Must not throw, and must not prevent the commit from landing. updated = RustyIceberg.with_transaction(table, cat) do tx with_overwrite(tx) do action add_data_files(action, files) @@ -435,6 +434,14 @@ end @test !isnothing(table_current_snapshot_id(updated)) data = read_table_data(updated) @test sort(data.id) == [1, 2] + + # The property must actually be there, not just fail to throw -- and + # iceberg-rust's own computed metrics must still be present alongside it. + summary = table_current_snapshot_summary(updated) + @test !isnothing(summary) + @test summary["attempt_id"] == "test-attempt-123" + @test summary["added-data-files"] == "1" + @test summary["total-records"] == "2" finally table != C_NULL && free_table(table) updated != C_NULL && free_table(updated) From 6ec94fb6595233d18278e09753f76c04702c7572 Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Mon, 14 Sep 2026 11:23:02 +0200 Subject: [PATCH 3/4] Trim comments to essentials --- iceberg_rust_ffi/src/table.rs | 10 ++-------- iceberg_rust_ffi/src/transaction.rs | 16 +++++----------- src/RustyIceberg.jl | 9 ++------- src/transaction.jl | 9 ++------- test/overwrite_tests.jl | 2 -- 5 files changed, 11 insertions(+), 35 deletions(-) diff --git a/iceberg_rust_ffi/src/table.rs b/iceberg_rust_ffi/src/table.rs index 046aacf..4499aee 100644 --- a/iceberg_rust_ffi/src/table.rs +++ b/iceberg_rust_ffi/src/table.rs @@ -453,14 +453,8 @@ pub extern "C" fn iceberg_table_schema(table: *mut IcebergTable) -> *mut c_char } } -/// Get the current snapshot's summary properties (the caller-supplied ones set via -/// `iceberg_overwrite_action_set_snapshot_properties`, e.g. attempt_id -- NOT -/// iceberg-rust's own computed metrics like `added-data-files`, `total-records`, etc, -/// which are also present in the same map and can only diverge from a caller-supplied -/// key if the caller tried to reuse one of those reserved names) as a JSON object -/// string (`{"key": "value", ...}`). -/// -/// Returns null if the table has no current snapshot. +/// Current snapshot's summary properties (caller-supplied + iceberg-rust's computed +/// metrics) as a JSON object string. Null if the table has no current snapshot. #[no_mangle] pub extern "C" fn iceberg_table_current_snapshot_summary(table: *mut IcebergTable) -> *mut c_char { if table.is_null() { diff --git a/iceberg_rust_ffi/src/transaction.rs b/iceberg_rust_ffi/src/transaction.rs index c0f8d98..884bfa0 100644 --- a/iceberg_rust_ffi/src/transaction.rs +++ b/iceberg_rust_ffi/src/transaction.rs @@ -82,10 +82,8 @@ impl IcebergFastAppendAction { pub struct IcebergOverwriteAction { added_files: Vec, deleted_files: Vec, - /// Custom snapshot summary properties (e.g. attempt_id, job_id, fencing_token) - /// to be recorded on the commit for reconciliation. Merged with iceberg-rust's - /// own computed summary properties (added/removed file counts, etc.) -- this - /// map only ever adds caller-supplied keys, it never overrides computed ones. + /// Custom snapshot summary properties, merged with iceberg-rust's computed ones + /// (which win on key collision). snapshot_properties: HashMap, } @@ -370,14 +368,10 @@ pub extern "C" fn iceberg_overwrite_action_add_data_files( 0 } -/// Set custom snapshot summary properties (e.g. attempt_id, job_id, fencing_token) to be -/// recorded on the commit this action produces. Merged with iceberg-rust's own computed -/// summary properties, which win on key collision -- callers cannot use this to override -/// computed metrics such as `added-data-files`. +/// Set custom snapshot summary properties for the commit this action produces. Merged +/// with iceberg-rust's computed properties, which win on key collision. /// -/// Copies `properties`; the caller retains ownership of it. Safe to call multiple times -/// before `apply` -- later calls add to (and override, on key collision) earlier ones. -/// Returns 0 on success, non-zero on error. +/// Copies `properties`. Returns 0 on success, non-zero on error. #[no_mangle] pub extern "C" fn iceberg_overwrite_action_set_snapshot_properties( action: *mut IcebergOverwriteAction, diff --git a/src/RustyIceberg.jl b/src/RustyIceberg.jl index f374c79..ad1d6ac 100644 --- a/src/RustyIceberg.jl +++ b/src/RustyIceberg.jl @@ -671,13 +671,8 @@ end """ table_current_snapshot_summary(table::Table)::Union{Dict{String,String},Nothing} -Get the current snapshot's summary properties -- both caller-supplied ones (set via -`set_snapshot_properties` on the `OverwriteAction` that produced this snapshot, e.g. -`attempt_id`) and iceberg-rust's own computed metrics (e.g. `added-data-files`, -`total-records`). A caller-supplied key is only overridden if it collides with one of -these computed key names. - -Returns `nothing` if the table has no current snapshot yet. +Current snapshot's summary properties (caller-supplied + iceberg-rust's computed +metrics). `nothing` if the table has no current snapshot. """ function table_current_snapshot_summary(table::Table) ptr = @ccall rust_lib.iceberg_table_current_snapshot_summary(table::Table)::Ptr{Cchar} diff --git a/src/transaction.jl b/src/transaction.jl index a2073d9..364466a 100644 --- a/src/transaction.jl +++ b/src/transaction.jl @@ -420,13 +420,8 @@ end """ set_snapshot_properties(action::OverwriteAction, properties::Dict{String,String}) -Set custom snapshot summary properties (e.g. `attempt_id`, `job_id`, `fencing_token`) to -be recorded on the commit this action produces, for reconciliation. Merged with -iceberg-rust's own computed summary properties, which win on key collision -- this cannot -be used to override computed metrics such as `added-data-files`. - -Safe to call multiple times before the action is applied; later calls add to (and -override, on key collision) earlier ones. +Set custom snapshot summary properties for the commit this action produces. Merged with +iceberg-rust's computed properties, which win on key collision. """ function set_snapshot_properties(action::OverwriteAction, properties::Dict{String,String}) if action.ptr == C_NULL diff --git a/test/overwrite_tests.jl b/test/overwrite_tests.jl index dea7cbc..f032e6b 100644 --- a/test/overwrite_tests.jl +++ b/test/overwrite_tests.jl @@ -435,8 +435,6 @@ end data = read_table_data(updated) @test sort(data.id) == [1, 2] - # The property must actually be there, not just fail to throw -- and - # iceberg-rust's own computed metrics must still be present alongside it. summary = table_current_snapshot_summary(updated) @test !isnothing(summary) @test summary["attempt_id"] == "test-attempt-123" From 6fd375d305d3e8c0b8ca2f4a5e5b507f1e099d3c Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Mon, 14 Sep 2026 11:45:32 +0200 Subject: [PATCH 4/4] Address review comments: full summary JSON, null-table test, collision 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 --- iceberg_rust_ffi/src/table.rs | 7 ++++--- src/RustyIceberg.jl | 11 +++++++++-- test/overwrite_tests.jl | 18 +++++++++++++++++- 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/iceberg_rust_ffi/src/table.rs b/iceberg_rust_ffi/src/table.rs index 4499aee..1f0a507 100644 --- a/iceberg_rust_ffi/src/table.rs +++ b/iceberg_rust_ffi/src/table.rs @@ -453,8 +453,9 @@ pub extern "C" fn iceberg_table_schema(table: *mut IcebergTable) -> *mut c_char } } -/// Current snapshot's summary properties (caller-supplied + iceberg-rust's computed -/// metrics) as a JSON object string. Null if the table has no current snapshot. +/// Current snapshot's summary (`operation` + caller-supplied and computed properties, +/// all flattened into one object) as a JSON string. Null if the table has no current +/// snapshot. #[no_mangle] pub extern "C" fn iceberg_table_current_snapshot_summary(table: *mut IcebergTable) -> *mut c_char { if table.is_null() { @@ -464,7 +465,7 @@ pub extern "C" fn iceberg_table_current_snapshot_summary(table: *mut IcebergTabl let Some(snapshot) = table_ref.table.metadata().current_snapshot() else { return ptr::null_mut(); }; - match serde_json::to_string(&snapshot.summary().additional_properties) { + match serde_json::to_string(snapshot.summary()) { Ok(json) => match std::ffi::CString::new(json) { Ok(c_str) => c_str.into_raw(), Err(_) => ptr::null_mut(), diff --git a/src/RustyIceberg.jl b/src/RustyIceberg.jl index ad1d6ac..8748bac 100644 --- a/src/RustyIceberg.jl +++ b/src/RustyIceberg.jl @@ -671,10 +671,17 @@ end """ table_current_snapshot_summary(table::Table)::Union{Dict{String,String},Nothing} -Current snapshot's summary properties (caller-supplied + iceberg-rust's computed -metrics). `nothing` if the table has no current snapshot. +Current snapshot's summary (`operation` + caller-supplied and computed properties). +`nothing` if the table has no current snapshot; throws if `table` has been freed. """ function table_current_snapshot_summary(table::Table) + if table == C_NULL + throw(IcebergException( + STATE_RESOURCE_FREED, + "Resource has been freed", + "Table has been freed", + )) + end ptr = @ccall rust_lib.iceberg_table_current_snapshot_summary(table::Table)::Ptr{Cchar} if ptr == C_NULL return nothing diff --git a/test/overwrite_tests.jl b/test/overwrite_tests.jl index f032e6b..2d91506 100644 --- a/test/overwrite_tests.jl +++ b/test/overwrite_tests.jl @@ -427,7 +427,12 @@ end updated = RustyIceberg.with_transaction(table, cat) do tx with_overwrite(tx) do action add_data_files(action, files) - set_snapshot_properties(action, Dict("attempt_id" => "test-attempt-123")) + # "added-data-files" collides with a computed key; the real value + # (asserted below) must win, not this bogus one. + set_snapshot_properties(action, Dict( + "attempt_id" => "test-attempt-123", + "added-data-files" => "999", + )) end end @@ -437,6 +442,7 @@ end summary = table_current_snapshot_summary(updated) @test !isnothing(summary) + @test summary["operation"] == "overwrite" @test summary["attempt_id"] == "test-attempt-123" @test summary["added-data-files"] == "1" @test summary["total-records"] == "2" @@ -449,6 +455,16 @@ end println("✅ set_snapshot_properties round-trips through a real commit") end +@testset "table_current_snapshot_summary on a null table throws" begin + # `Table` is a raw `Ptr{Cvoid}`, not a wrapper struct -- `free_table` can't null it + # out on the caller's side, so a freed-then-reused Table is unchecked UB here, same + # as every other Table-consuming function in this file. What the null-check in + # table_current_snapshot_summary actually guards against is a genuinely-null Table + # value, matching table_schema's existing behavior -- that's what this tests. + @test_throws RustyIceberg.IcebergException table_current_snapshot_summary(C_NULL) + println("✅ table_current_snapshot_summary on a null table throws") +end + @testset "set_snapshot_properties on a freed action throws" begin action = RustyIceberg.OverwriteAction() free_overwrite_action!(action)