Skip to content

Add assert_requirements bindings for pinned-base commit assertions - #116

Open
gbrgr wants to merge 2 commits into
mainfrom
gb/assert-requirements-stage-commit
Open

gbrgr wants to merge 2 commits into
mainfrom
gb/assert-requirements-stage-commit

Conversation

@gbrgr

@gbrgr gbrgr commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Wraps RelationalAI/iceberg-rust#83's OverwriteAction::assert_requirements for Julia:

  • assert_requirements(action::OverwriteAction, requirements) lets a caller supply arbitrary TableRequirements (as typed subtypes: AssertTableUuid, AssertRefSnapshotId, etc.) alongside the ones an action derives automatically. A supplied requirement of an auto-derived kind replaces the derived one; any other kind is added; repeated calls accumulate.

Why

Transaction::commit's do_commit always refreshes the table from the catalog and silently rebases onto whatever is current before computing its auto-derived RefSnapshotIdMatch requirement, on every attempt, not just retries. That means a retried (or racing) commit would otherwise keep asserting whatever is current rather than the base it actually started from, and a genuine conflict could never surface. assert_requirements lets a caller pin that assertion to an externally-resolved base snapshot instead, so the catalog itself enforces it.

Bumps iceberg_rust_ffi's iceberg-rust git rev to RelationalAI/iceberg-rust#83's branch head to pick up the new API; move this to a main rev once that PR merges.

Test plan

  • assert_requirements_tests.jl -- covers override/accumulate/reject semantics against a real in-memory catalog, including two tests that provoke a genuine CatalogCommitConflicts by pinning a stale/wrong requirement and asserting the catalog itself rejects the commit
  • overwrite_tests.jl -- unaffected, still green
  • cargo build/cargo fmt --check/cargo clippy on iceberg_rust_ffi -- clean

🤖 Generated with Claude Code

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 64.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.72%. Comparing base (208160a) to head (03e3418).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/transaction.jl 64.00% 9 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #116      +/-   ##
==========================================
- Coverage   86.17%   85.72%   -0.46%     
==========================================
  Files           9        9              
  Lines        1013     1058      +45     
==========================================
+ Hits          873      907      +34     
- Misses        140      151      +11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gbrgr added a commit that referenced this pull request Oct 15, 2026
…ker images (#117)

## Summary

Three unrelated CI/correctness fixes, split out of #116 so that PR stays
focused on `assert_requirements`:

- **UB fix**: `nested_pipeline.rs`'s
`oversized_batch_does_not_deadlock_byte_sem` test freed
`batch.batch.rust_ptr` by casting it to `*mut Vec<u8>`, but
`record_batch_to_c_ffi` allocates it as `Box<ArrowBatchInner>` (two
Arrow FFI structs) -- see `iceberg_arrow_batch_free` in `table.rs`,
which does the correct cast. Dropping it as the wrong type is undefined
behavior. This was latent: CI's floating `stable` toolchain moved from
rustc 1.98.1 (last known-good run, 2026-09-16) to 1.99.0 (2026-09-28),
which added a stricter allocator precondition check that now catches it
as a `SIGABRT` ("NonNull::new_unchecked requires that the pointer is
non-null"). Not triggered by any functional change -- any PR running CI
today would hit it.
- **Deprecated API**: `pipeline_stats.rs` used
`AtomicU64::fetch_update`, deprecated in favor of `try_update`; under
`-D warnings` this now fails the build.
- **Unpullable docker images**: `quay.io/minio/minio`/`quay.io/minio/mc`
are no longer pullable (MinIO discontinued prebuilt community images),
failing `setup_bucket` with "unauthorized" on every PR's `test-julia-*`
jobs. Builds them locally instead, from MinIO's last published binary
release (mirrors the same fix already applied in raicode's own iceberg
docker-compose stack).

## Test plan

- [x] `cargo test` in `iceberg_rust_ffi` -- 40 passed, 0 failed,
including `oversized_batch_does_not_deadlock_byte_sem`
- [x] `docker compose up -d minio setup_bucket polaris init
table-creation` brings up the full stack locally; `table-creation`
completes successfully

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
@gbrgr
gbrgr force-pushed the gb/assert-requirements-stage-commit branch from 03e3418 to 5791a6e Compare October 15, 2026 07:43
Wraps RelationalAI/iceberg-rust#83's OverwriteAction.assert_requirements
for Julia: supply arbitrary TableRequirements (as typed TableRequirement
subtypes: AssertTableUuid, AssertRefSnapshotId, etc.), passed to the Rust
side as a JSON array via a new iceberg_overwrite_action_assert_requirements
FFI export. A supplied requirement of an auto-derived kind replaces the
derived one; any other kind is added; repeated calls accumulate.

This is needed because Transaction::commit's do_commit always refreshes
the table from the catalog and silently rebases onto whatever is current
before computing its auto-derived RefSnapshotIdMatch requirement, so a
retried (or racing) commit would otherwise keep asserting whatever is
current rather than the base it actually started from, and a genuine
conflict could never surface. assert_requirements lets a caller pin that
assertion to an externally-resolved base snapshot instead.

Bumps iceberg_rust_ffi's iceberg-rust git rev to f02c8db2f890d4447a2995
738668573769aa98a3 (that PR's branch head, rebased onto main after #84's
upstream sync + pipeline.rs architecture convergence merged) to pick up
the new API; move this to a `main` rev once the PR merges.

Tests: assert_requirements_tests.jl covers the override/accumulate/reject
semantics, including two tests that provoke a genuine CatalogCommitConflicts
by pinning a stale/wrong requirement and asserting the catalog itself
rejects the commit.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@gbrgr
gbrgr force-pushed the gb/assert-requirements-stage-commit branch from 5791a6e to 4a68816 Compare October 15, 2026 08:48
AssertTableUuid, AssertLastAssignedPartitionId, AssertDefaultSpecId, and
AssertDefaultSortOrderId are now exercised through a real commit, round-tripping
their JSON through the real Rust TableRequirement deserializer and catalog
check. AssertTableDoesNotExist can only ever fail against an existing table, so
its round-trip is covered by asserting that the catalog correctly rejects it.

Previously only 3 of 8 variants reached the real FFI boundary in a test; the
other 5 were verified correct only by manual inspection against iceberg-rust's
enum definition during self-review, which a future rename or type change there
could silently break without any test catching it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants