Repository navigation
Conversation
|
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
2 tasks done
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
force-pushed
the
gb/assert-requirements-stage-commit
branch
from
October 15, 2026 07:43
03e3418 to
5791a6e
Compare
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
force-pushed
the
gb/assert-requirements-stage-commit
branch
from
October 15, 2026 08:48
5791a6e to
4a68816
Compare
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wraps RelationalAI/iceberg-rust#83's
OverwriteAction::assert_requirementsfor Julia:assert_requirements(action::OverwriteAction, requirements)lets a caller supply arbitraryTableRequirements (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'sdo_commitalways refreshes the table from the catalog and silently rebases onto whatever is current before computing its auto-derivedRefSnapshotIdMatchrequirement, 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_requirementslets a caller pin that assertion to an externally-resolved base snapshot instead, so the catalog itself enforces it.Bumps
iceberg_rust_ffi'siceberg-rustgit rev to RelationalAI/iceberg-rust#83's branch head to pick up the new API; move this to amainrev 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 genuineCatalogCommitConflictsby pinning a stale/wrong requirement and asserting the catalog itself rejects the commitoverwrite_tests.jl-- unaffected, still greencargo build/cargo fmt --check/cargo clippyoniceberg_rust_ffi-- clean🤖 Generated with Claude Code