Core: Rebase replace transaction onto refreshed metadata on concurrent commit#16975
Open
Kurtiscwright wants to merge 46 commits into
Open
Core: Rebase replace transaction onto refreshed metadata on concurrent commit#16975Kurtiscwright wants to merge 46 commits into
Kurtiscwright wants to merge 46 commits into
Conversation
…t commit When a concurrent commit lands after a replace transaction is staged, commitReplaceTransaction advanced base to the refreshed metadata to satisfy the optimistic lock but committed the stale staged metadata. This dropped the concurrent commit's snapshots from history and reused its sequence number, violating sequence-number monotonicity. A replace is last-writer-wins on the current schema, spec, and data, but it is not a drop-and-recreate: the table must keep its history. Rebuild the replacement on the refreshed base and replay the staged updates so the concurrent commit's history is preserved and the replacement snapshot's sequence number is re-derived from the refreshed base. Add TableMetadata.buildReplacementPreservingIds, which keeps the staged schema's field IDs instead of re-deriving them by name, so a concurrent schema change cannot shift the IDs the staged data files were written against. Add tests covering the rebase, history preservation, and field-id stability. Update the two server-side conflict tests in CatalogTests, which previously asserted a spurious last-assigned-id requirement failure, to assert the replace succeeds as last-writer-wins.
Co-authored-by: Sreesh Maheshwar <maheshwarsreesh@gmail.com>
Kurtiscwright
force-pushed
the
replace-seq-reuse-repro
branch
from
44eb85c to
f19a18c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness bug in Core replace-table transactions where a concurrent commit could be silently overwritten. The new behavior rebases a staged replace onto refreshed table metadata when the base has advanced, preserving concurrent history (snapshots/schema/spec history) while still applying the replacement’s intended schema/spec/sort/location/properties and adding the replacement’s new data after the concurrent commit.
Changes:
- Rebase
BaseTransactionreplace commits onto refreshedTableMetadatawhen concurrent commits advance the base, and replay staged updates on the rebuilt replacement. - Add
TableMetadata.buildReplacementPreservingIdsto rebuild replacements without reassigning schema field IDs by name. - Add/adjust tests to validate preserved history, stable field IDs, dropped-table handling, and updated REST/HMS expectations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCreateReplaceTable.java | Updates HMS tests to expect concurrent property preservation and relaxes spec equality to content equivalence. |
| core/src/test/java/org/apache/iceberg/TestReplaceTransaction.java | Adds new replace-transaction concurrency tests (rebase behavior, schema history preservation, field ID stability, dropped-table case). |
| core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java | Updates REST/catalog concurrency tests to expect rebased replace to succeed instead of failing on spurious ID requirements. |
| core/src/main/java/org/apache/iceberg/TableMetadata.java | Adds buildReplacementPreservingIds to rebuild replacements while keeping pre-assigned schema field IDs. |
| core/src/main/java/org/apache/iceberg/BaseTransaction.java | Introduces staged replacement tracking and the rebase-and-replay path for replace commits when the base metadata changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
332
to
336
| if (base != null) { | ||
| rebaseReplaceOnto(base); | ||
| } | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
To keep in line with the simple-transaction path this has been added in the latest commit.
Member
|
@Kurtiscwright What do you mean by rebased on PR 16975 which is the current PR? |
…ge (apache#16725) * Docker: Add configuration to toggle info logs flooding in iceberg-rest-fixture * Docker: Support REST fixture logging config Generated-by: GPT-5 --------- Co-authored-by: Kevin Liu <kevin.jq.liu@gmail.com>
…#16979) The monitor source rate was computed as perSecond(1.0 / rateLimit.getSeconds()). Duration.getSeconds() rounds any sub-second rate limit down to 0, so the rate became perSecond(Infinity), which turns off rate limiting. This change computes the rate from millis, so sub-second intervals are limited correctly. The 60s default is unchanged. This made the maintenance E2E tests flaky, since they use sub-second rate limits. With no rate limit the source calls table.refresh() in a loop and uses a full CPU core per job. CI runs the tests with -DtestParallelism=auto, which starts one MiniCluster per fork, so these busy sources use up all the cores. The converter runs on a timer, and under that load the timer fires too slowly, letting the test fail after the timeout hits.
…e#16988) Bumps [org.codehaus.jettison:jettison](https://github.com/jettison-json/jettison) from 1.5.5 to 1.5.6. - [Release notes](https://github.com/jettison-json/jettison/releases) - [Commits](jettison-json/jettison@jettison-1.5.5...jettison-1.5.6) --- updated-dependencies: - dependency-name: org.codehaus.jettison:jettison dependency-version: 1.5.6 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/checkout](https://github.com/actions/checkout) from 6.0.3 to 7.0.0. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@df4cb1c...9c091bb) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: 7.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/setup-java](https://github.com/actions/setup-java) from 5.2.0 to 5.3.0. - [Release notes](https://github.com/actions/setup-java/releases) - [Commits](actions/setup-java@be666c2...ad2b381) --- updated-dependencies: - dependency-name: actions/setup-java dependency-version: 5.3.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [com.diffplug.spotless:spotless-plugin-gradle](https://github.com/diffplug/spotless) from 8.6.0 to 8.7.0. - [Release notes](https://github.com/diffplug/spotless/releases) - [Changelog](https://github.com/diffplug/spotless/blob/main/CHANGES.md) - [Commits](diffplug/spotless@gradle/8.6.0...gradle/8.7.0) --- updated-dependencies: - dependency-name: com.diffplug.spotless:spotless-plugin-gradle dependency-version: 8.7.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
) * Build: Bump datamodel-code-generator from 0.63.0 to 0.64.1 Regenerate open-api/rest-catalog-open-api.py with datamodel-code-generator 0.64.1. As of 0.64.0, optional primitive const fields no longer emit the const value as an injected default, so the generated discriminator fields (action/type) are now `Literal[...] | None = None`. Committing the regenerated output keeps `make generate` + `git diff --exit-code` green. Supersedes apache#16985. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Mark REST spec discriminators as required so they are not generated as nullable --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…e#16989) Bumps software.amazon.awssdk:bom from 2.46.10 to 2.46.15. --- updated-dependencies: - dependency-name: software.amazon.awssdk:bom dependency-version: 2.46.15 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
The current spec defines this object as a single enum constant, not a comma-separated list of enum constants.
* CVE scan: improve PR failure reporting Keep the Trivy scan step green so GitHub opens the reporting step that prints the actionable CVE findings, then fail PR runs from that reporting step. Also remove the Spark 3.5 Jackson CVE ignore entries on this test branch so the PR run exercises the failure UI. Generated-by: GPT-5 Codex * CVE scan: format Trivy findings as a table Parse Trivy SARIF messages into a compact table and keep the PR annotation concise so the failed reporting step is easier to read. Generated-by: GPT-5 Codex * CVE scan: simplify Trivy report step Extract SARIF parsing and report rendering into small shell helpers so the report step is easier to read without changing behavior. Generated-by: GPT-5 Codex * CVE scan: restore Spark 3.5 Trivy ignore Restore the Spark 3.5 Jackson CVE ignore entries that were removed only to exercise the PR failure UI. Generated-by: GPT-5 Codex * CVE scan: document PR and push behavior Clarify that Trivy always writes SARIF, PRs fail from the reporting step on findings, push runs keep findings informational, and missing or unparseable SARIF is still an error. Generated-by: GPT-5 Codex
…e#17015) Include snapshot row-lineage fields and manifest list key IDs in BaseSnapshot equals/hashCode and toString. This keeps comparisons and hashes aligned with metadata that changes row ID assignment or manifest-list encryption key selection, and makes those values visible in debug output. Co-authored-by: Codex <codex@openai.com>
github-actions
Bot
added
Specification
Kurtiscwright
force-pushed
the
replace-seq-reuse-repro
branch
from
a7b204f to
9bc05ff
Compare
Contributor
Author
I meant that I pulled this PR locally and started my PR based on #16975 but I didn't use the REST catalog changes and I added tests that simulate the history overwrite scenario. |
Member
Then, which is your PR? |
Contributor
Author
Member
|
Do you mean #16289? |
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.
Fixes #16232
Rebased on PR #16975
Replacing a table replaces its schema, partitioning, and data, but it keeps the
table's history instead of deleting the table and making a new one. The old code
didn't do this when another commit landed first. While a replace was being
prepared, a second commit could update the table. The replace then reloaded the
latest table state to pass the catalog's conflict check, but still wrote out the
changes it had prepared at the start. The second commit's snapshots disappeared
from history, and if that commit had changed the schema, the replace could end
up pointing columns at IDs that no longer matched the data files.
Now, when the table has changed, the replace is re-applied on top of the latest
table state: the new schema, partitioning, sort order, location, and properties
are kept, but they sit on top of the other commit's snapshots, and the replace's
own new data is added after it. If the table no longer exists when the replace
commits (for example, it was dropped in the meantime), there is nothing to build
on, so the replace just creates the table fresh.
Add TableMetadata.buildReplacementPreservingIds. It keeps the field IDs the
replace schema already has, instead of matching columns up by name and handing
out new IDs. Matching by name can move a column to a different ID than its data
files were written with, which makes that data unreadable.
Two design notes:
The replace gets its own re-apply path rather than reusing
applyUpdates, because a replace's schema and partitioning live in the prepared
metadata, not in the list of pending updates, so replaying only those updates
would drop the replacement and leave the old schema in place.
It uses buildReplacementPreservingIds rather than buildReplacement
so a concurrent schema change can't shift the replace's field IDs away
from its data files.
Add tests for the rebuilt replace, kept history, stable field IDs, and the
dropped-table case. Two REST-only tests used to expect a replace with an
unchanged schema or partitioning to fail on a spurious ID check; update them to
expect it to succeed.