Skip to content

feat: remove manual REST update_schema fallback and upgrade to iceber… - #7

Merged
sionsmith merged 7 commits into
osodevops:mainfrom
manudiv16:feat/remove-rest-update-schema-fallback-iceberg-010
Jul 24, 2026
Merged

feat: remove manual REST update_schema fallback and upgrade to iceber…#7
sionsmith merged 7 commits into
osodevops:mainfrom
manudiv16:feat/remove-rest-update-schema-fallback-iceberg-010

Conversation

@manudiv16

@manudiv16 manudiv16 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

This change upgrades the official Apache Iceberg Rust client from 0.7 to 0.10.0 and the Arrow/Parquet ecosystem from 54 to 58, then removes the temporary standalone REST update_schema fallback that was introduced in PR #6.

Key improvements

  • Official schema evolution: OfficialRestCommitter::update_schema now delegates to Transaction::update_schema() via the official iceberg-rust 0.10.0 API instead of a hand-rolled REST path.
  • Removes ~400 lines of temporary scaffolding: OAuth2 refresh, runtime route resolution, multipart namespace encoding, and manifest-list cache workarounds are no longer needed.
  • Cleaner CAS semantics: snapshot commits rely on the official Transaction CAS inside commit_append_with_catalog, removing the duplicate caller-side snapshot precondition check.
  • Dependency refresh: Arrow/Parquet/Iceberg versions are aligned with the latest stable iceberg-rust release.

Implementation

  • crates/k2i-core/src/iceberg/official.rs — delegates update_schema to the official Transaction API, deletes the standalone REST path, simplifies schema/TableInfo conversion, and maps errors by message (ErrorKind variants changed in 0.10).
  • crates/k2i-core/src/iceberg/writer.rs — switches Parquet writer to set_max_row_group_row_count(Some(...)) for the parquet 58 API.
  • Cargo.toml / Cargo.lock — bump iceberg and iceberg-catalog-rest to 0.10, Arrow/Parquet to 58, and raise rust-version to 1.94.
  • CHANGELOG.md, README.md, claude.md, docs/quickstart.md, docs/deployment.md, Dockerfile, and scripts/security-audit.sh — update documented minimum Rust version and dependency versions.

Requirements

  • Minimum supported Rust version raised from 1.88 to 1.94, matching iceberg 0.10.0 and the updated AWS SDK dependency graph.

Test plan

  • cargo fmt --all --check
  • cargo check --workspace --all-targets
  • cargo test --workspace --no-fail-fast
  • cargo clippy --workspace --all-targets -- -D warnings
  • scripts/e2e-docker-iceberg.sh
  • K2I_E2E_LOAD_MESSAGES=100000 scripts/e2e-docker-iceberg-load.sh

Compatibility

  • REST catalogs: schema evolution now uses the official iceberg-rust transaction path; all auth, route, and warehouse-prefix negotiation is handled by the official client.
  • Glue/Hive/Nessie/SQL catalogs: unaffected by this change.
  • Hot buffer, read registry, txlog, and Kafka consumer: unaffected.
  • Downstream users building from source must use Rust 1.94+.

@manudiv16
manudiv16 force-pushed the feat/remove-rest-update-schema-fallback-iceberg-010 branch from 495c229 to 0439b1e Compare July 21, 2026 22:36
@manudiv16 manudiv16 closed this Jul 21, 2026
@manudiv16
manudiv16 force-pushed the feat/remove-rest-update-schema-fallback-iceberg-010 branch from d16da1f to 4ac9c64 Compare July 21, 2026 23:22
@manudiv16 manudiv16 reopened this Jul 21, 2026
@manudiv16
manudiv16 marked this pull request as draft July 21, 2026 23:45
@sionsmith

Copy link
Copy Markdown
Contributor

@manudiv16 do you want us to look at this?

The upgrade from iceberg-rust 0.7 to 0.10 changed how nested types
are represented when loaded from table metadata. The Display impl for
Type::Struct produces 'struct<field1: type1, ...>' while the protobuf
decoder uses full JSON objects like '{"type":"struct","fields":[...]}'.

This mismatch caused the schema compatibility check to detect breaking
changes on every startup, even when the schemas were identical.

Add iceberg_type_to_json_string() to convert iceberg::spec::Type back
to the same JSON format used by the protobuf decoder's iceberg_type_value(),
ensuring consistent round-trip through TableSchema and Iceberg Schema.
@manudiv16

Copy link
Copy Markdown
Contributor Author

I pushed this before running the full test suite. The library upgrade (iceberg 0.7 → 0.10, Arrow/Parquet 54 → 58) turned out to require significantly more changes than initially expected, so I have been working through the fixes. The latest commit addresses a schema round-trip mismatch caused by the new iceberg Display impl for nested types.

@manudiv16
manudiv16 marked this pull request as ready for review July 22, 2026 19:08
@manudiv16

Copy link
Copy Markdown
Contributor Author

Test verification

All tests in the plan were executed locally from the repository root before opening this PR:

# Static checks
cargo fmt --all --check
cargo check --workspace --all-targets
cargo clippy --workspace --all-targets -- -D warnings

# Unit + integration tests
cargo test --workspace --no-fail-fast

# Iceberg E2E (correctness + load profiles)
scripts/e2e-docker-iceberg.sh
K2I_E2E_LOAD_MESSAGES=100000 scripts/e2e-docker-iceberg-load.sh

Context

This PR introduces three new unit tests in crates/k2i-core/src/iceberg/official.rs to cover the simplified TableSchema → iceberg Schema conversion path:

  • iceberg_type_to_json_all_primitives — verifies all primitive JSON type mappings
  • iceberg_schema_to_table_schema_with_nested_types — covers struct and list field projection
  • schema_roundtrip_with_nested_types — full TableSchema → iceberg Schema → JSON round-trip with nested types

The Rust 1.94 MSRV bump and the Arrow 54→58 / Parquet 54→58 / Iceberg 0.7→0.10 upgrade also touches the write path (writer.rs row-group API) and the Docker E2E image build, so the full cargo suite plus both Iceberg E2E profiles were re-run rather than relying on cargo check alone.

Verified

  • cargo fmt --all --check
  • cargo check --workspace --all-targets
  • cargo test --workspace --no-fail-fast
  • cargo clippy --workspace --all-targets -- -D warnings
  • scripts/e2e-docker-iceberg.sh (v1 → v2 additive evolution, v3 breaking rejection, schema readiness blocking) ✅
  • K2I_E2E_LOAD_MESSAGES=100000 scripts/e2e-docker-iceberg-load.sh (100k rows, full cold visibility, k2i_errors_total = 0) ✅

Fixes lint failure — recent changes to iceberg_type_to_json_string and
its tests weren't formatted, breaking cargo fmt --check in CI.
@sionsmith

Copy link
Copy Markdown
Contributor

Full review of this PR: read the whole diff, checked out the branch locally, ran the unit suite, clippy, the Docker e2e suites (including two not in the test plan), and wrote targeted integration probes against a live apache/iceberg-rest-fixture to exercise paths the e2e configs don't cover. Overall this is a solid cleanup and the core direction is right — but there are two blocking issues and a few smaller ones.

Blocking

1. CI is red: test.yml is missing libcurl4-openssl-dev

The lockfile refresh bumps rdkafka-sys 4.9.0+2.10.0 → 4.10.0+2.12.1. librdkafka 2.12 requires libcurl headers at build time, so Check & Lint fails with:

rdkafka-sys-4.10.0+2.12.1/librdkafka/src/rdkafka_conf.c:60:10: fatal error: curl/curl.h: No such file or directory

semver-check.yml already installs libcurl4-openssl-dev (which is why that job passes), but the three install steps in test.yml only install cmake libssl-dev pkg-config. Because Check & Lint gates everything, no unit or integration tests actually ran in CI for this PR. Please add libcurl4-openssl-dev (and check whether libsasl2-dev is needed to match semver-check) to all three apt-get install steps in test.yml.

2. commit_snapshot now silently drops files_to_remove in release builds

The old code returned a hard SnapshotCommit error when files_to_remove was non-empty. This PR replaces that with a debug_assert!, which compiles out in release builds (the Dockerfile builds --release), and then reports files_removed: commit.files_to_remove.len() in the result as if the removals happened.

This path is reachable: maintenance/compaction.rs commits with non-empty files_to_remove (compaction.rs:252-258). The e2e configs all set compaction_enabled = false, so no e2e run covers it. I verified against a live REST fixture:

  • Release build: the commit succeeds via fast_append, claims files_removed=1, and the old file stays referenced by the table → after a compaction cycle, every compacted row exists twice (original file + merged file). Silent data duplication.
  • Debug build: the debug_assert panics inside the maintenance task instead.

On main, the same scenario returns an error, compaction logs a warning, and the merged file is left as an orphan — no correctness impact. Please restore the hard error (the comment about the invariant can stay, but the check must be a real one). The result should also not report removals that didn't happen.

3. map_iceberg_error string-matching is unnecessary — and provably misclassifies

The PR description says the rewrite was needed because "ErrorKind variants changed in 0.10", but that's not the case: iceberg 0.10.0 still has ErrorKind::TableNotFound and ErrorKind::CatalogCommitConflicts, err.kind() is still public, and the REST client still returns CatalogCommitConflicts on HTTP 409 (iceberg-catalog-rest-0.10.0/src/catalog.rs:1031). The old kind-based match compiles fine against 0.10.

The string version is also already wrong: message.contains("does not exist") matches the 0.10 REST client's NamespaceNotFound message. Verified live:

create_table under missing namespace mapped to:
Iceberg(TableNotFound("NamespaceNotFound => Tried to create a table under a namespace that does not exist"))

More importantly, CAS-conflict detection (which drives the transaction coordinator's retry behavior) now depends on upstream message text that can change in any patch release. Please revert to matching on err.kind().

Non-blocking

  • manifest_list_path_for_snapshot regression vs PR Unify REST catalog onto official client with standalone update_schema fallback #6: the bounded manifest-list cache was added in Unify REST catalog onto official client with standalone update_schema fallback #6 specifically so the WAL records the real manifest-list path. With the default trait impl returning None, writer.rs:430 now falls back to a fabricated {warehouse}/metadata/snap-{id}.avro path in the IcebergSnapshot txlog entry. Recovery only keys on batch_id, so this is informational-only today — but worth either documenting the fallback as synthetic or dropping the field.
  • update_schema silently ignores _expected_schema_id: safe with today's callers (both call sites run diff_table_schema first and reject breaking changes), but the trait contract still advertises the parameter. A doc comment on the trait noting the REST impl delegates conflict detection to the transaction would prevent surprises. Same for commit_snapshot's ignored expected_snapshot_id — the reasoning in the code comment is sound (I verified fast_append emits a RefSnapshotIdMatch requirement in 0.10), but the deleted stale-snapshot CAS test wasn't replaced by any test asserting conflict behavior on the new path.
  • iceberg_type_to_json_string maps Fixed(n)"binary" (lossy), while parse_iceberg_type supports fixed[n]. For tables created by external tools with fixed-width columns, the reported schema will diff as binary. Edge case, but an easy fix for symmetry.

Verified good

  • 284 k2i-core unit tests pass locally; cargo clippy --workspace --all-targets -- -D warnings clean; cargo fmt --check clean (Rust 1.96).
  • MSRV claim is correct: iceberg / iceberg-catalog-rest 0.10.0 both declare rust-version = "1.94".
  • iceberg-storage-opendal with opendal-fs+opendal-s3 is feature-parity with 0.7's defaults (storage-fs, storage-s3) — no storage regression.
  • scripts/e2e-docker-iceberg.sh passes: v1 ingest, additive v2 evolution through the official Transaction::update_schema() path, v3 breaking-change pause, DuckDB iceberg_scan validation.
  • scripts/e2e-docker-backfill.sh and scripts/e2e-docker.sh (neither was in the PR test plan) both pass: backfilled Parquet visible through RPC and DuckDB; hot/cold visibility and breaking-schema handling intact.
  • Targeted probe against a live REST fixture: update_schema adds an optional column correctly and is idempotent on re-apply (no duplicate columns, no error).
  • The new schema-evolution restriction (add-optional-column only, skip existing names) matches K2I's additive-only design — diff_table_schema rejects everything else before this code runs.

Verdict

Not mergeable yet: CI has to be green (fix #1), and #2 is a real data-correctness regression relative to main with #3 a cheap robustness fix alongside it. All three are small, contained changes — happy to re-review once they're pushed.

sionsmith and others added 2 commits July 24, 2026 08:40
rdkafka-sys 4.10.0 bundles librdkafka 2.12.1, which requires curl/curl.h
at build time. semver-check.yml already installs libcurl4-openssl-dev;
test.yml did not, so every job behind Check & Lint failed before running.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Restore the hard error for non-empty files_to_remove in commit_snapshot.
  The debug_assert compiled out in release builds, so a compaction commit
  against a REST catalog would fast_append the merged file while leaving
  the source files live (duplicate rows), and report files_removed as if
  the removals had happened.
- Map catalog errors by ErrorKind again instead of message text. The 0.10
  ErrorKind enum still exposes TableNotFound and CatalogCommitConflicts;
  the string match misclassified namespace-not-found responses (their
  message contains "does not exist") and tied CAS-conflict detection to
  upstream message wording.
- Restore the bounded manifest-list cache so the transaction log records
  the real manifest-list path for committed snapshots instead of the
  synthesized fallback.
- Emit fixed[n] from iceberg_type_to_json_string instead of collapsing
  fixed-width types to binary, matching parse_iceberg_type.
- Document why update_schema ignores expected_schema_id and re-add the
  removals/manifest-cache regression test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sionsmith

Copy link
Copy Markdown
Contributor

Pushed two commits on top of this branch addressing the review findings (maintainer edits):

ci: install libcurl and SASL dev headers for librdkafka 2.12 — adds libcurl4-openssl-dev libsasl2-dev to the three install steps in test.yml, matching semver-check.yml. This unblocks Check & Lint (and therefore the unit/integration jobs) after the rdkafka-sys 4.10 bump.

fix: harden official REST committer after iceberg 0.10 upgrade

  • commit_snapshot rejects non-empty files_to_remove with a hard error again (the debug_assert compiled out in release builds and would have silently duplicated data on compaction commits).
  • map_iceberg_error matches on err.kind() again — ErrorKind::TableNotFound / CatalogCommitConflicts still exist in 0.10.0, and the string match misclassified namespace-not-found as table-not-found.
  • Restored the bounded manifest-list cache so the txlog records the real manifest-list path (PR Unify REST catalog onto official client with standalone update_schema fallback #6 behavior).
  • iceberg_type_to_json_string emits fixed[n] instead of collapsing to binary.
  • Doc note on why update_schema ignores expected_schema_id, plus the removals/manifest-cache regression test is back.

Verification on the combined branch: cargo fmt --check, clippy --workspace --all-targets -D warnings, 276 k2i-core unit tests, scripts/e2e-docker-iceberg.sh, and live probes against apache/iceberg-rest-fixture in both debug and release profiles (removals rejected, real manifest-list path cached, correct error classification, additive schema evolution idempotent).

None of the upgrade work itself changed — this only restores the guardrails around it.

@sionsmith
sionsmith merged commit f51faac into osodevops:main Jul 24, 2026
13 checks passed
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