Skip to content

Carry Iceberg REST TableUpdate deltas on the snapshots commit request, and use them for audit branchRefName - #669

Draft
cbb330 wants to merge 2 commits into
linkedin:mainfrom
cbb330:chbush/audit-branch-ref-metadata-updates
Draft

Carry Iceberg REST TableUpdate deltas on the snapshots commit request, and use them for audit branchRefName#669
cbb330 wants to merge 2 commits into
linkedin:mainfrom
cbb330:chbush/audit-branch-ref-metadata-updates

Conversation

@cbb330

@cbb330 cbb330 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds an optional jsonMetadataUpdates field to IcebergSnapshotsRequestBody carrying a commit's deltas as Iceberg REST spec TableUpdate actions, and uses it to populate a new branchRefName on TableAuditEvent so operations against named Iceberg branches become observable.

Today the commit request carries only replacement state (jsonSnapshots + snapshotRefs) — what the table now looks like, never what the commit changed. That makes "which branch was written" unanswerable at the audit layer for the operation that matters most: ALTER TABLE t CREATE BRANCH b adds a ref at the current head and commits no snapshot, so main and b point at the same snapshot and are indistinguishable in the resulting state.

Relationship to #616: this supersedes #616, which added the same branchRefName field using a snapshot-ordering heuristic. #616 is left open and untouched for now; the two are alternatives and only one should merge. See "Why not infer it" below for why the heuristic was abandoned.

Changes

  • Client-facing API Changes — new optional jsonMetadataUpdates field on IcebergSnapshotsRequestBody. Purely additive: the server still builds table metadata from jsonSnapshots/snapshotRefs, older clients omit the field, and consumers must tolerate null/empty. No existing field changes meaning.
  • New FeaturesTableAuditEvent.branchRefName, populated from the commit's set-snapshot-ref action.
  • Tests — see below.

Why the Iceberg REST spec shape

The spec already models this exactly. CommitTableRequest is {requirements[], updates[]}, where updates[] is a discriminated union on action. CREATE BRANCH b is a single set-snapshot-ref with ref-name: b, type: branch, and no add-snapshot.

The client already has this list for free — TableMetadata.changes(), the same one every Iceberg REST catalog sends (RESTTableOperations line 136) — and MetadataUpdateParser serializes it in the spec wire format verbatim. So this ships the spec shape rather than a lookalike: when OpenHouse adopts the REST commit endpoint, the field is promoted to updates at the top level and the full-state fields retire, with no re-modeling.

This is phase 1 of 3. Phase 2 has the server prefer these deltas over the snapshot-set diff it currently reconstructs in OpenHouseInternalTableOperations (lines ~325-351, which already computes new-vs-existing snapshot IDs and diffs refs). Phase 3 replaces baseTableVersion with spec requirements[]. Each is separately reviewable; this PR changes no commit behavior.

Why not infer it from the resulting state

The obvious approach — match the last snapshot in jsonSnapshots to the ref pointing at it — is wrong in four ways, all of which CREATE BRANCH triggers:

  1. Nondeterministic on ties. After CREATE BRANCH b, main and b both point at the last snapshot. snapshotRefs is a HashMap, so the answer depended on iteration order.
  2. The invariant does not hold. "The last snapshot is the newly-committed one" is false for any commit that writes no snapshot.
  3. Tags counted as branches. SnapshotRefParser parses both and ref type was absent from the comparison.
  4. Order dependence. Correctness rested on client serialization order of TableMetadata.snapshots(), which no API contract guarantees.

Reading the commit's declared set-snapshot-ref removes all four: no tie to break, no ordering assumption, and tags excluded by their type.

Safety

  • Client-side serialization failures are swallowed and individual unserializable actions skipped — the field can never fail a commit.
  • Server-side, unparseable actions are skipped individually so one bad entry cannot hide the rest; extraction stays best-effort and cannot fail the request.
  • Clients predating the field omit it, and branchRefName is then left unset rather than guessed. An absent audit field is preferable to one that is wrong roughly half the time on ties.
  • One source file serves both the iceberg-1.2 and iceberg-1.5 runtimes (1.5 pulls 1.2's srcDirs), so both are covered.

Testing Done

  • Added new tests for the changes made.

Server-side (IcebergSnapshotsApiHandlerAuditTest), covering the cases the heuristic could not get right:

  • CreateBranchAtHeadReportsNewBranchNotMain — the tie; reports b, not main
  • CreateBranchIsDeterministicRegardlessOfRefOrderLinkedHashMap with main first, still correct
  • TagCommitLeavesBranchRefNameNulltype: tag ignored
  • DropBranchLeavesBranchRefNameNullremove-snapshot-ref is not a write
  • WithoutMetadataUpdatesLeavesBranchRefNameNull — clients omitting the field
  • SkipsUnparseableMetadataUpdate — malformed action does not hide valid ones

Client-side (OpenHouseTableOperationsMetadataUpdatesTest) asserts changes() produces the expected spec actions per operation: CREATE BRANCH emits only a branch-typed set-snapshot-ref with no add-snapshot, CREATE TAG is tag-typed, DROP BRANCH is a remove-snapshot-ref, and an append reports both add-snapshot and the ref that moved.

  • Updated existing tests to reflect the changes made. Existing audit fixtures now carry spec-shaped updates for a plain append to main.

Note for reviewers: TableMetadata.changes() accumulates across builds within a session, so the client fixture discards construction history to match production, where the base always comes from a refresh parsed off disk. This is the assumption the approach rests on.

Additional Information

  • Large PR broken into smaller PRs, and PR plan linked in the description. Two commits, independently reviewable and independently green: the API contract (jsonMetadataUpdates, no consumer) and the audit consumer. Phases 2-3 described above are separate PRs.

Not covered, and worth flagging: no end-to-end assertion through BranchTestSpark3_5.java (which already has ~30 CREATE BRANCH invocations, including tie cases), and putSnapshotsForReplace (CTAS/RTAS) is not covered by the changes() tests. Both are worth adding before phase 2 relies on this field for commit behavior rather than audit only.

cbb330 added 2 commits August 2, 2026 21:15
Adds an optional `jsonMetadataUpdates` field to IcebergSnapshotsRequestBody
holding a commit's deltas as Iceberg REST spec `TableUpdate` actions, and
populates it client-side from `TableMetadata.changes()`.

Why: `jsonSnapshots` and `snapshotRefs` carry complete replacement state, so
the request describes what the table now looks like but never what the commit
actually changed. The server recovers the deltas by diffing the incoming
snapshot set against prior state (OpenHouseInternalTableOperations), and some
changes cannot be recovered at all — `CREATE BRANCH b` adds a ref at the
current head and commits no snapshot, so the resulting state is
indistinguishable from a no-op on main.

The Iceberg REST spec already models this: `CommitTableRequest.updates[]` is a
list of `TableUpdate` actions, where that operation is a single
`set-snapshot-ref` with `ref-name: b`, `type: branch`, and no `add-snapshot`.
The client has the list for free via `TableMetadata.changes()` — the same one
every REST catalog sends — and `MetadataUpdateParser` emits the spec wire
format verbatim. This ships the spec shape rather than a lookalike, so when
OpenHouse adopts the REST commit endpoint the field is promoted to `updates`
and the full-state fields retire, with no re-modeling.

Additive and advisory: the server still builds table metadata from
jsonSnapshots/snapshotRefs and is not changed here, clients predating the field
omit it, and consumers must tolerate null/empty. Serialization failures are
swallowed and individual unserializable actions skipped, so the field can never
fail a commit. One source file serves both the iceberg-1.2 and iceberg-1.5
runtimes.

No consumer yet — the audit path and the server-side delta handling land
separately.

Tests assert `changes()` yields the expected spec actions per operation:
CREATE BRANCH emits only a branch-typed `set-snapshot-ref`, CREATE TAG is
tag-typed, DROP BRANCH is a `remove-snapshot-ref`, and an append reports both
`add-snapshot` and the ref that moved. Note `changes()` accumulates across
builds within a session, so the fixture discards construction history to match
production, where the base always comes from a refresh parsed off disk.
Table operations against named Iceberg branches are not currently observable:
TableAuditEvent records currentSnapshotId and currentSnapshotTimestampMs but
carries no signal for which branch ref a commit wrote.

Add branchRefName, populated from the `set-snapshot-ref` action in the request's
jsonMetadataUpdates. The commit states which ref it moved and whether that ref
is a branch or a tag, so the field reports what happened rather than inferring
it.

Inferring it from the resulting table state does not work. The obvious approach
— match the last snapshot in jsonSnapshots to the ref pointing at it — fails on
the operation this field exists to observe: `CREATE BRANCH b` creates a ref at
the current head and commits no snapshot, so main and b both point at the last
snapshot and the answer depends on HashMap iteration order. It would also treat
tags as branches, since SnapshotRefParser parses both and ref type is absent
from that comparison, and it assumes the client always serializes snapshots
chronologically. None of those hold for ref-only commits.

Clients predating jsonMetadataUpdates omit it; branchRefName is then left unset
rather than guessed, since an absent audit field is preferable to one that is
wrong roughly half the time. Unparseable actions are skipped individually so a
single bad entry cannot hide the rest, and extraction stays best-effort — it
cannot fail the request.

currentSnapshotId and currentSnapshotTimestampMs continue to track the main ref
unchanged, for backwards compatibility.

This reports what the client declared, not what the server committed. Making it
authoritative means having doCommit surface the refs it actually changed, which
it already computes internally; that is a separate change.

Tests cover CREATE BRANCH at head, determinism across ref iteration order,
CREATE TAG, DROP BRANCH, clients omitting the field, and malformed actions.
@cbb330
cbb330 marked this pull request as draft August 3, 2026 05:25
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.

1 participant