Skip to content

Core: copy ByteBuffers when copying GenericRecord (cherry-pick apache/iceberg#12855) - #18

Merged
abrarsher23 merged 8 commits into
1.8.1-affirm-patchfrom
abrar/1.8.1-patch4-copy-bytebuffers
Oct 6, 2026
Merged

abrarsher23 merged 8 commits into
1.8.1-affirm-patchfrom
abrar/1.8.1-patch4-copy-bytebuffers

Conversation

@abrarsher23

@abrarsher23 abrarsher23 commented Sep 29, 2026 •

Copy link
Copy Markdown

What this does

Fixes a bug in Iceberg 1.8.1 where some equality deletes on binary columns are silently ignored when reading a table.
This is a straight cherry-pick of the upstream fix, apache/iceberg#12855
(for issue #11239), with no changes. It also sets this fork's version
to 1.8.1-PATCH.4.

The bug

When Iceberg reads an equality-delete file, it reuses a single buffer for binary values and relies on copying each
record to keep them separate. In 1.8.1 that copy is shallow, so every delete record loaded from the file ends up
pointing at the same buffer, which by the end holds the last record's value. For a binary key, only the last delete
in each delete file actually takes effect.

What that looks like:

  • Rows that should be deleted still show up, so you see duplicate keys. A compaction running on the same reader then
    writes those duplicates out permanently.
  • If the key combines a binary column with other columns, the mixed-up values can also match rows that were never
    deleted, so live rows can disappear from reads.

The fix

GenericRecord.copy() now makes a real copy of binary (ByteBuffer) and nested-record values instead of sharing them.
One production file, about 16 lines. The upstream regression tests come with it.

Testing

  • Unit tests: the iceberg-data suite passes: 602 tests, 0 failures (JDK 17).
  • The new tests catch the bug: reverting only the fix, while keeping the new tests, makes them fail: 8 delete-read
    tests (binary and struct keys, across Avro, ORC and Parquet) plus testDeepCopyValues.
  • Real tables, before and after: we re-read tables that were affected by this bug, pinned to the same snapshots,
    first with 1.8.1-PATCH.3 and then with this patch. Before: duplicate keys. After: the duplicates are gone, and the
    results match an independent check computed from the same files.

What's in this PR

File Why
core/.../data/GenericRecord.java The fix
data/.../DeleteReadTests.java, TestGenericReaderDeletes.java, TestGenericRecord.java Upstream regression tests
version.txt Sets the version to 1.8.1-PATCH.4. The build reads this file; without it the jar would be versioned 1.9.0-SNAPSHOT.
AFFIRM_VERSIONS.md A short changelog of what each 1.8.1 fork release contains (same idea as on 1.11.0-affirm-patch)
dev/.rat-excludes Excludes the changelog from the license-header check

After merge I'll tag 1.8.1-PATCH.4 at the merge commit.

Worth knowing

  • Failing CI checks are pre-existing and unrelated to this change. They fail the same way on other PRs against
    1.8.1-affirm-patch (e.g. [LAKE-6732] Spark 3.5: refuse to rewrite a table with duplicate data file registrations #6), and this PR doesn't touch any of the code involved:

    Check (JDK 11 / 17 / 21) What fails Why
    build-checks :iceberg-core:spotlessJavaCheck Formatting in ExponentialHttpRequestRetryStrategy.java, TestExponentialHttpRequestRetryStrategy.java and TestRESTCatalog.java, which came in with Prevent Retries 5xx Errors for Non-Idempotent Requests #1. Not touched here, and deliberately not reformatted in this PR.
    core-tests :iceberg-aws:test — 6 S3 test classes fail at startup The tests start a MinIO container from Docker Hub (minio/minio), and that pull is now denied.
    kafka-connect-tests integration tests fail at startup Same MinIO image from Docker Hub.

    The tests for this fix live in iceberg-data; :iceberg-data:test runs inside core-tests and passes. The Spark,
    Flink and Hive checks pass too. The MinIO failures were fixed on the 1.11 line by Build: Pull MinIO images from Quay (cherry-pick apache/iceberg#18071) #11 (pull the image from Quay instead);
    porting that and the formatting fix to this line are separate, small PRs.

  • 1.10.0 and later already contain this fix, so this patch goes away when we move to the 1.11 line.

Change management

CMT approvals happen on this PR. The downstream version-bump PR points here instead of carrying its own.

  • Risk: low to medium. It's one upstream commit with its own tests, checked before and after on real tables. The
    bigger risk is the manual release process (build the jar, then the Spark package), and a dev build of that went through
    a test deployment first.
  • Rollback: go back to the 1.8.1-PATCH.3 build. No data or metadata formats change, so the old reader just picks up
    where it left off.
  • Release order: merge → tag → build and publish the package → bump the version downstream (separate PR) → deploy.

hsingh574 and others added 7 commits September 25, 2026 12:32
Adds version.txt (1.8.1-PATCH.4) and an AFFIRM_VERSIONS.md changelog for the
1.8.1 line, following the convention now used on 1.11.0-affirm-patch. PATCH.1
and PATCH.3 were built from an uncommitted version.txt; their contents are
recorded from branch history.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@rayhondo rayhondo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM. Reviewed the diff and PR description only. I did not check out the branch or run the tests.

  • Cherry-pick of apache#12855: GenericRecord.copy() now deep-copies ByteBuffer and nested GenericRecord values. This fixes equality deletes on binary keys collapsing to the last record of each delete file.
  • The new and updated tests (DeleteReadTests, TestGenericReaderDeletes, TestGenericRecord) cover the fix. I found no correctness problems.
  • The PATCH.4 version bump, AFFIRM_VERSIONS.md entry and dev/.rat-excludes change are consistent.
  • Non-blocking: List and Map values are still shared by copy(). Equality-delete fields can't be lists or maps, and upstream has the same limit. copy() also allocates per binary or struct field, which matches upstream.

Review authored by Claude Code (Claude Sonnet 5.5) at Ray Kim's request.

@abrarsher23
abrarsher23 merged commit e7bbee7 into 1.8.1-affirm-patch Oct 6, 2026
37 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants