Skip to content

Core: Skip concurrent DV validation for added data files - #18626

Merged
pvary merged 2 commits into
apache:mainfrom
kinolaev:fix-added-dvs-validation
Nov 3, 2026
Merged

pvary merged 2 commits into
apache:mainfrom
kinolaev:fix-added-dvs-validation

Conversation

@kinolaev

@kinolaev kinolaev commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

On v3+ tables, RowDelta validates that no concurrent commit added a DV for any data file it adds a DV for. When no starting snapshot is set, this walks the whole branch history and fails with Cannot determine history between starting snapshot null and the last known ancestor once the oldest snapshot is expired. DVs for data files added in the same commit can't conflict with concurrent commits, so the history walk is now skipped when every added DV references a data file added in that commit.

Note that the old check also incidentally rejected replays of commits with DVs, as long as history back to the original commit was available. With this change such a replay succeeds and leaves two DVs for the same data file, which makes reads fail. Iceberg doesn't reject duplicate data files in general, so preventing replays remains the committer's responsibility.

On v3 tables, this also changes the retry of an operation whose earlier attempt was applied but reported as failed: if all of its DVs reference data files added by the same operation, the retry no longer fails this check and, unless another validation rejects it, takes the existing no-op path for an already-committed snapshot. The general fix for this retry case, covering failures from any validation, is being developed in #18407.

Test plan: a new test expires the oldest snapshot, then commits a data file with a DV for it and asserts the commit succeeds; it fails without the fix. testConcurrentDVsForSameDataFile now also adds a data file with its own DV, verifying that DVs for existing files are still validated.


AI Disclosure

  • Model: Claude Opus 5.5
  • Platform/Tool: Claude Code
  • Human Oversight: fully reviewed
  • Prompt Summary: Skip DV validation for DVs that reference data files added in the same commit.

Generated-by: Claude Code
Signed-off-by: Sergei Nikolaev <kinolaev@gmail.com>

@JeonDaehong JeonDaehong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this with a small probe against core (in-memory catalog, v3). I expired the oldest snapshot, then ran a RowDelta without validateFromSnapshot:

commit main this PR
new data file + its DV fails (Cannot determine history ...) OK
DV for an existing data file fails fails
both in one commit fails fails
DV for an existing file, with validateFromSnapshot OK OK

So it does what it says, and DVs for existing files are still validated. The second row is the same expired-history failure that came up for RewriteFiles in #18224, in case someone wants to look at the two together. LGTM.

@pvary pvary left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Just small nits

Signed-off-by: Sergei Nikolaev <kinolaev@gmail.com>
@kinolaev

kinolaev commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @pvary! Both suggestions are applied (3dde1b3) and CI is green again.

@pvary
pvary merged commit ae86975 into apache:main Nov 3, 2026
40 checks passed
@pvary

pvary commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Merged to main.
Thanks @kinolaev for the fix!

@tomerr90

tomerr90 commented Nov 3, 2026 •

Copy link
Copy Markdown

Can this be backported to 1.12.x? Apologies if thats not the right forum, couldnt really find a process for this documented anywhere

kinolaev added a commit to kinolaev/iceberg that referenced this pull request Nov 3, 2026
@kinolaev

kinolaev commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor Author

Hi @tomerr90, I've just published iceberg-flink-runtime-2.2-1.12.0.5.jar from my fork (full change list from 1.12.0). Feel free to use it!

Since there's no 1.12.x branch, I'm assuming the fix will be in 1.12.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants