Fix multi-schema replication intermediates dropped on snapshots-endpoint commits by dushyantk1509 · Pull Request #603 · linkedin/openhouse

dushyantk1509 · 2026-05-27T16:58:44Z

Summary

Replication commits arriving through PUT /iceberg/v2/snapshots with multi-schema-jump intermediate schemas were silently losing them due to two issues that combine to leave the dest replica with a snapshot whose schema-id points at a schema not present in schemas[]. Trino strictly resolves snapshot schema-id against schemas[] and fails with "Cannot find schema with schema id N"; Spark falls back to current-schema-id and reads succeed.

Two fixes:

TablesMapper#toTableDto(TableDto, IcebergSnapshotsRequestBody) was missing the @mapping for newIntermediateSchemas. The CreateUpdateTableRequestBody overload had it (added in Add multi schema update support for tables during replica table commits #407), but the IcebergSnapshotsRequestBody overload used for the snapshots endpoint did not — so intermediates packed correctly by the client got dropped at the server-side DTO mapping step.
OpenHouseTableOperations#doCommit (after [RTAS] Add client side support (part 3) #524 introduced putSnapshotsForReplace) was routing cross-cluster REPLICA_TABLE replication commits through the RTAS replace path because they update both metadata and snapshots in one commit. The server's REPLACE branch treats the commit as a fresh table creation and uses CLIENT_TABLE_SCHEMA (bootstrap-era schema) as the final schema, not the request's actual target. This produces a different but related corruption pattern. Detect REPLICA commits (base REPLICA_TABLE + metadata PRIMARY_TABLE + different cluster IDs) and route them through the regular putSnapshots path so the intermediate-schemas plumbing fires.

Both fixes are needed in tandem: without (1), even the regular update branch drops intermediates; without (2), commits hitting the post-#524 client get routed around (1) entirely.

Changes

For all the boxes checked, please include additional details of the changes made in this pull request.

Testing Done

Manually Tested on local docker setup. Please include commands ran, and their output.
Added new tests for the changes made.
Updated existing tests to reflect the changes made.
No tests added or updated. Please explain why. If unsure, please feel free to ask for help.
Some other form of testing like staging or soak time in production. Please explain.

For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request.

Additional Information

Breaking Changes
Deprecations
Large PR broken into smaller PRs, and PR plan linked in the description.

For all the boxes checked, include additional details of the changes made in this pull request.

mkuchenbecker · 2026-05-27T21:06:31Z

+      boolean isReplicaCommit =
+          getTableType(base, metadata) == CreateUpdateTableRequestBody.TableTypeEnum.REPLICA_TABLE;
+      if (metadataUpdated && snapshotsUpdated && base != null && !isReplicaCommit) {


What makes a replica commit different that we need to special case?

Does that mean that this excludes RTAS commit for replica table?

This block is supposed to be used for RTAS/CTAS where these is no concept of intermediate schemas but the issue is that this also sources replica commits from gaas into replace snapshots loosing intermediate schemas.

mkuchenbecker · 2026-05-27T21:10:00Z

+      boolean isReplicaCommit =
+          getTableType(base, metadata) == CreateUpdateTableRequestBody.TableTypeEnum.REPLICA_TABLE;
+      if (metadataUpdated && snapshotsUpdated && base != null && !isReplicaCommit) {
        // Only CTAS and RTAS can update both metadata and snapshots at the same time.


this comment is no longer accurate.

mkuchenbecker · 2026-05-27T21:12:21Z

-              .get(OPENHOUSE_CLUSTER_ID_KEY)
-              .equals(metadata.properties().get(OPENHOUSE_CLUSTER_ID_KEY))) {
-        return baseTableType;
+      String baseTypeStr = base.properties().get(OPENHOUSE_TABLE_TYPE_KEY);


Why might this not exist?

This is added as a safety check for legacy tables.

what are legacy tables?

mkuchenbecker · 2026-05-27T21:12:43Z

  @VisibleForTesting
  CreateUpdateTableRequestBody.TableTypeEnum getTableType(
      TableMetadata base, TableMetadata metadata) {
+    String metaTypeStr = metadata.properties().get(OPENHOUSE_TABLE_TYPE_KEY);


nit, I would move this to after the if so on the case where base is null its not evaluated.

This is needed for the end return statement.

mkuchenbecker · 2026-05-27T21:13:33Z

+        if (baseTableType == CreateUpdateTableRequestBody.TableTypeEnum.REPLICA_TABLE
+            && metadataTableType == CreateUpdateTableRequestBody.TableTypeEnum.PRIMARY_TABLE
+            && baseCluster != null
+            && !baseCluster.equals(metaCluster)) {
+          return baseTableType;
+        }


This logic does not makse sense to me. Why can we not trust the table type?

Trying to understand the change here. Seems like this change is applicable only for replica table to determine table type?

Just to be clear, this logic is pre-existing and this change only introduce null checks.

Metadata's type can be trusted by default; this branch is the one exception. In a cross-cluster replication commit, the destination replica's metadata is overwritten with the source primary's content, so metadata.tableType says PRIMARY even though this table is a REPLICA. base is the destination's truth, so for that fingerprint (base=REPLICA, metadata=PRIMARY, different cluster) we override to base.

I do agree that it can be collapsed to "base if non-null, else metadata". I would put it out of scope of this PR.

To me the concept of a Replica table as a first class citizen is adding the friction here. A replica there is nothign special about other than the permissions on who can modify it. Its unclear to me why this is a table type, and why that needs a special case.

…int commits Replication commits arriving through PUT /iceberg/v2/snapshots with multi-schema-jump intermediate schemas were silently losing them due to two issues that combine to leave the dest replica with a snapshot whose schema-id points at a schema not present in schemas[]. Trino strictly resolves snapshot schema-id against schemas[] and fails with "Cannot find schema with schema id N"; Spark falls back to current-schema-id and reads succeed. Two fixes: 1. TablesMapper#toTableDto(TableDto, IcebergSnapshotsRequestBody) was missing the @mapping for newIntermediateSchemas. The CreateUpdateTableRequestBody overload had it (added in linkedin#407), but the IcebergSnapshotsRequestBody overload used for the snapshots endpoint did not — so intermediates packed correctly by the client got dropped at the server-side DTO mapping step. 2. OpenHouseTableOperations#doCommit (after linkedin#524 introduced putSnapshotsForReplace) was routing cross-cluster REPLICA_TABLE replication commits through the RTAS replace path because they update both metadata and snapshots in one commit. The server's REPLACE branch treats the commit as a fresh table creation and uses CLIENT_TABLE_SCHEMA (bootstrap-era schema) as the final schema, not the request's actual target. This produces a different but related corruption pattern. Detect REPLICA commits (base REPLICA_TABLE + metadata PRIMARY_TABLE + different cluster IDs) and route them through the regular putSnapshots path so the intermediate-schemas plumbing fires. Both fixes are needed in tandem: without (1), even the regular update branch drops intermediates; without (2), commits hitting the post-linkedin#524 client get routed around (1) entirely. Unit tests: - TablesMapperTest#testToTableDtoWithPutSnapshotsPreservesNewIntermediateSchemas pins the mapper extraction. - OpenHouseTableOperationsTest#testDoCommitRoutesReplicaTableThroughPutSnapshotsNotReplace pins REPLICA dispatch goes to regular putSnapshots (replaceCommit=false). - OpenHouseTableOperationsTest#testDoCommitRoutesPrimaryRtasThroughPutSnapshotsForReplace pins genuine RTAS on PRIMARY tables still uses the replace path.

Documentation-only follow-up to the multi-schema replication fix. No logic change. - Rewrite the putSnapshotsForReplace branch comment: the old "only CTAS and RTAS update both metadata and snapshots" was inaccurate now that REPLICA commits also touch both and are excluded via isReplicaCommit. - Ground getTableType's null return in the real cause (legacy tables predating openhouse.tableType, back-filled server-side by OpenHouseInternalRepositoryImpl#checkIfTableTypeAdded) instead of the vague "replace transactions before properties populate". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

abhisheknath2011

Looks good to me. Lets get a stamp from Mike as well.

mkuchenbecker · 2026-06-23T17:30:05Z

-      if (metadataUpdated && snapshotsUpdated && base != null) {
-        // Only CTAS and RTAS can update both metadata and snapshots at the same time.
-        // When the table exists, it will be a replace commit operation.
+      // REPLICA_TABLE commits from cross-cluster replication can update both


I don't think detecting that this is a replica seems incorrect vs detecting the type of change and applying it correctly. Why can't we detect the condition and apply it vs detecting the table type?

The existing implementation already tries to detect a change-type: metadataUpdated && snapshotsUpdated && base != null is meant to mean "this is a replace (RTAS/CTAS)." The problem is a cross-cluster replication commit also updates both metadata and snapshots in one shot, so it matches that same condition — the two operations are indistinguishable by their metadata/snapshot deltas alone.

That's what's unclear to me is why this needs a special commit type at all. This is what's unclear by this logic:

1 - Why does Replication have special handling at-all in this case? What is the distinguishing feature of Replication that requires it to be handled specifically
2 - Why is replication table type not enough of a signal to determine what to do?

I understand we need to preserve history but "replica" type seems like the wrong thing to be detecting, and if it is the thing that we are detecting then it should be enough to have its own dedicated logic "when replica do x"

mkuchenbecker

If we are keeping replica tables as separately handled, a clear "if replica, do X" will make the logic easier to follow and later remove if the case is no longer needed, and protects replication from changes to other commit types.

cbb330 · 2026-07-02T00:04:21Z

+1 to merge. this stops the data loss.

But, this fix adds a fourth way to detect an intent the client already knows but never states. Server infers "replace vs. not" from comparing before/after states and then corrects that guess with three table properties (is-a-copy, is-an-original, cluster names differ) and now a further check to steer replication back to the update path. Each is a workaround for the same root which is that the caller knows what operation it's doing and doesn't send that operation to the server, so the server reconstructs it. Every new commit shape will need another clue and another branch.

The durable fix is for the client to declare the operation instead of the server deducing it. This is what the open-source Iceberg REST catalog does that we can draw inspiration from. A commit is an explicit list of changes to apply (add this schema, set current schema, add this snapshot, point this branch at it) plus preconditions, with no "replace mode" to detect. Replication stops being special, same change list as anything else, just more "add schema" entries.

With that in mind, I suggest a not-blocking follow-up: add one field where the caller declares the operation (replication / replace / append) and route the logic on that enum in server. then delete the metadata-comparison guess and the property-sniffing that was bandaided over time.

Longer term, we can align with the REST spec [0] and replace the two schema fields with one ordered list of schema additions. Today the final schema goes in one field and the in-between schema versions go in a separate list and it is handled as a branched logic on server. Reducing branching logic helps prevent bugs on edge cases. And then openhouse.tableType and openhouse.clusterId can stop being used to pick a code path, as sort of side-affect magic variables.

[0] - In the Iceberg REST catalog, a commit (UpdateTableRequest, sent to POST /v1/.../tables/{table}) has two arrays:

updates: an ordered list of change actions, each tagged with an action name. The relevant ones here: add-schema (adds one schema version to the table's schema list), set-current-schema (points the table at one of those schema versions by id), add-snapshot (adds a data snapshot), set-snapshot-ref (points a branch, e.g. main, at a snapshot).
requirements: preconditions the server checks before applying, e.g. assert-ref-snapshot-id ("the main branch must currently point at snapshot X") or assert-create ("the table must not already exist").

mkuchenbecker · 2026-07-03T13:59:33Z

+          putSnapshots(base, metadata);
+        } else if (metadataUpdated) {
+          createUpdateTable(base, metadata);
+        }


nit. else not needed, just return short circuit.

dushyantk1509 marked this pull request as ready for review May 27, 2026 17:02

mkuchenbecker reviewed May 27, 2026

View reviewed changes

Comment thread ...e-java-runtime/src/main/java/com/linkedin/openhouse/javaclient/OpenHouseTableOperations.java

mkuchenbecker reviewed May 27, 2026

View reviewed changes

Dushyant Kumar and others added 2 commits June 22, 2026 13:01

dushyantk1509 force-pushed the dushyantk1509/fix-snapshot-mapper-intermediate-schemas branch from 6db910e to 29a4ecd Compare June 22, 2026 07:32

abhisheknath2011 previously approved these changes Jun 23, 2026

View reviewed changes

mkuchenbecker reviewed Jun 23, 2026

View reviewed changes

mkuchenbecker reviewed Jun 30, 2026

View reviewed changes

Address review comments: Make replica commit path explicit

f9ef484

dushyantk1509 dismissed abhisheknath2011’s stale review via f9ef484 July 3, 2026 04:47

mkuchenbecker reviewed Jul 3, 2026

View reviewed changes

mkuchenbecker approved these changes Jul 3, 2026

View reviewed changes

Uh oh!

Conversation

dushyantk1509 commented May 27, 2026 • edited by kamanavishnu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing Done

Additional Information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dushyantk1509 Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhisheknath2011 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkuchenbecker left a comment

Choose a reason for hiding this comment

Uh oh!

cbb330 commented Jul 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dushyantk1509 commented May 27, 2026 •

edited by kamanavishnu

Loading

dushyantk1509 Jun 22, 2026 •

edited

Loading