Skip to content

fix(iceberg): publish merge deletes and append atomically - #1073

Merged
iamgp merged 1 commit into
mainfrom
fix/1064-atomic-iceberg-merge
Oct 8, 2026
Merged

iamgp merged 1 commit into
mainfrom
fix/1064-atomic-iceberg-merge

Conversation

@iamgp

@iamgp iamgp commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Closes #1064.

Changes

  • Validate and align incoming Parquet data before staging any deletes. Cast failures now propagate instead of continuing into the write.
  • Stage every 1,000-key delete batch and the replacement append in one Iceberg transaction. This guarantees one atomic catalog publication, not one snapshot. PyIceberg 0.11.1 can stage multiple snapshots in that publication.
  • Propagate commit conflicts explicitly. A retry reruns the complete merge against a newly loaded table. Existing catalog-ref routing, deduplication options, approximate delete metrics, and post-commit operation evidence remain intact.

Real SQLite catalog and filesystem acceptance tests cover 1,003 incoming keys, reader-visible publication boundaries, failures after staged deletes and append and before catalog commit, unchanged rows and data snapshot on failure, replay, real optimistic conflicts, resource evidence, and DLT ingestion with isolated main/dev routing.

The transaction boundary is one merge call. This does not make a multi-file ingestion run one transaction. Schema policies and history work in #1065 and #1066 remain out of scope.

Verification

  • make setup
  • uv run --locked pytest packages/phlo-iceberg/tests packages/phlo-dlt/tests -m "not integration" --tb=short -q: 294 passed, 6 deselected.
  • make typecheck-python, focused Ruff lint/format, file-header check, and git diff --check: passed.
  • make check: passed; 5,802 tests passed, 4 skipped, 228 deselected. The initial header-baseline test failure was caused by missing history in the shallow checkout; fetching full history resolved it without code changes.
  • uv run --locked python scripts/run_integration.py: all seven disposable Nessie/MinIO verification suites passed, 110 tests total with no skips. New atomicity acceptance tests run locally without those services.

No merge or deployment has been performed.

Validate and align incoming data before staging writes. Use one Iceberg transaction for every delete batch and the replacement append, and record post-commit evidence only after publication.

Add local catalog acceptance tests for multiple delete batches, rollback, replay, real commit conflicts, resource evidence, and DLT ingestion with ref isolation.

Closes #1064
@coldtea-pr-lens

coldtea-pr-lens Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Nothing flagged · reviewed b247334


Architecture

Architecture diagram for phlohouse/phlo at b247334

Play the walkthrough


Data flow

Data flow diagram for phlohouse/phlo at b247334

Follow each request


View

  • Architecture lens
  • Data flow lens
  • Expand every detail

Tip

Click the link under each diagram to open it on a canvas you can zoom, pan and step through

🪧 More tips
  • Run npx skills add coldteadotai/pr-lens, then tell your coding agent: "Diagram the change you just made with PR Lens and attach it to the pull request."
  • Run npx @coldtea/pr-lens-cli analyze --base origin/main on a branch, then npx @coldtea/pr-lens-cli render .pr-lens/graph.json. Same lenses, your own model key, before the pull request exists
  • Untick Architecture lens or Data flow lens under View to hide a diagram, or tick Expand every detail to open every section. The comment redraws in a few seconds
  • The diagrams are links. Click one to open it on the canvas, then press W or click play to walk through the change
  • Open a diagram on the canvas, then press W or click play to walk through the change one step at a time
  • The CLI's render reads .github/pr-lens.yml and applies your renames, exclusions and lane pins at draw time
  • Set github.comment.collapsed: true in .github/pr-lens.yml to fold the comment behind one View architecture and data flow row. Drawing still runs as before
  • Set github.draw: on-demand in .github/pr-lens.yml and PR Lens stops drawing pull requests on its own. Comment @pr-lens draw on a pull request when you want that one drawn
  • Add .github/workflows/pr-lens.yml with coldteadotai/pr-lens/packages/action@v0 and your model provider's key as its api-key to run PR Lens from your own CI. Any /chat/completions endpoint works
  • Push a commit and the drawing stays, with a note that it is out of date. Tick Redraw in the note to draw the new head
  • Switch GitHub to dark mode and the diagrams follow. The moving dots are this pull request's data in motion

Thanks for using PR Lens! It's built by Coldtea, free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

@phlo-agent

phlo-agent Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

No blocking findings. Reviewed the full diff at b247334, the linked issue #1064, the surrounding phlo_iceberg.tables / phlo_iceberg.resource code, and the installed PyIceberg 0.11.1 transaction internals.

What I verified

  • merge_to_table now stages every ≤1000-key In delete plus the append inside one table.transaction() (tables.py#L544-L565). In PyIceberg 0.11.1 Transaction.__exit__ commits once through Table._do_commit, and each staged producer captures its parent from the cumulative transaction.table_metadata, so the delete → delete → append snapshot chain is well-formed and only the first AssertRefSnapshotId (the pre-merge ref) is retained as the commit requirement. That makes the "one catalog publication, not one snapshot" claim hold.
  • _do_commit reassigns table.metadata = response.metadata, so _finish_iceberg_commit(commit_op, table) after the with block still reads the post-commit snapshot id; the evidence assertions in test_resource_ref_metrics_and_evidence_follow_publication are consistent with this.
  • A precommit exception skips the commit (__exit__ only commits when no exception is pending), so the old rows and snapshot survive; the staged deletes are never published. This matches the acceptance criteria.
  • The new raise after the Arrow cast failure (tables.py#L527-L532) moves schema validation ahead of the first delete. Previously the cast failure was logged and execution continued into the deletes, which is the divergence bug(phlo-iceberg): commit merge deletes and replacement rows atomically #1064/investigation(phlo-dlt/phlo-iceberg): repeated branch merge doubled physical rows despite unique_key dedup #777 describes; the new behaviour fails closed before any mutation.
  • test_merge_dedup.py's fake table gained transaction() -> nullcontext(self), so its class-level delete patches still intercept the call, and the new phlo-dlt test follows the existing phlo_dlt.executor.setup_dlt_pipeline patch and context=None pattern.

Remaining validation

  • CI was green at the time of review except ci / python / core tests (3.12, shard 1), which was still in progress.
  • make check does not exercise the Nessie/MinIO integration lane; the PR reports scripts/run_integration.py passing locally, which is the boundary that would confirm the atomic publication against a real catalog.

No changes requested.

@iamgp
iamgp added this pull request to the merge queue Oct 8, 2026
Merged via the queue into main with commit c0079ed Oct 8, 2026
29 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.

bug(phlo-iceberg): commit merge deletes and replacement rows atomically

1 participant