Core: V4 write direction wrappers by stevenzwu · Pull Request #16936 · apache/iceberg

stevenzwu · 2026-06-23T06:05:07Z

What

Adds v4 write-direction TrackedFile adapters: reusable forwarding wrappers that present a legacy v2/v3 ContentFile (DataFile/DeleteFile) as a v4 manifest-entry row on the write path, without materializing a fresh struct per row. A single adapter is allocated per writer and re-pointed at each file; content stats are served by a map-backed view (MapBackedContentStats) over the file's stat maps, with copy() producing stable snapshots.

Benchmark summary

JMH microbenchmarks compared two strategies for presenting a legacy file as a v4 row: convert (materialize a fresh struct per row) vs wrap (a reusable wrapper allocated once per writer and re-pointed per row, with zero per-row allocation). Throughput in ops/ms (higher is better); allocation via the GC profiler in B/op (lower is better).

Per-column content stats: wrap runs ~1.6–1.9x faster than convert across all column counts and allocates far less (39,280 vs 98,984 B/op at 200 columns). Convert pays per-column allocation and eager bound decode that the map-backed view avoids.
Fixed envelope (null stats): convert and wrap are within measurement error (9130 vs 8680 ops/ms); wrap still allocates less (256 vs 688 B/op). The envelope's small, fixed field count means the wrapper's positional dispatch neither meaningfully helps nor hurts.
Combined (envelope + stats): wrap-both is fastest or statistically tied at every column count with the lowest per-row allocation. The column-stats cost dominates and scales with table width, so wrapping stats is what matters; the fixed envelope edge is swamped once real column stats are present.

Net: the reusable wrap model is adopted because it wins or ties everywhere and minimizes per-row allocation — the cost that grows with table width.

See the benchmark doc for full methodology and per-column numbers.

gaborkaszab

Hey @stevenzwu ,
I went through mostly for my own understanding. Left some comments, mostly I'm a bit hesitant to expose setting status and sequence numbers directly through the builders. Let me know what you think!

rdblue · 2026-10-08T19:53:06Z

+  }
+
+  @Test
+  void manifestTrackedFileAdapterFailsWhenAddedFilesCountMissing() {


Use manifestWithCounts? What about existing and deleted cases?

Existing and deleted file counts are in InvalidCount, along with null and non-zero replaced and modified file counts. Row counts are not in that enum.

rdblue · 2026-10-08T19:54:40Z

+    assertThat(result.contentType()).isEqualTo(FileContent.DELETE_MANIFEST);
+    assertThat(result.formatVersion()).isZero();
+    assertThat(result.recordCount())
+        .isEqualTo(ADDED_FILES_COUNT + EXISTING_FILES_COUNT + DELETED_FILES_COUNT);


This uses a helper method to create manifest and then assumes what that helper passed for these counts. I'd rather see manifestWithCounts used with DELETES passed in so that this test can be read without skipping to see how manifests are written by writeManifestFile.

The success test calls newManifestFile(content). The count constants live in the helper. I removed the multi-arg overload because the argument list was hard to read.

This is still a problem. The test is not readable on its own and there's no need for this because the newManifestFile(FileContent) method is only called here. It should be embedded.

Embedded the construction in manifestTrackedFileAdapter and removed newManifestFile, so the counts sit next to the assertions.

rdblue · 2026-10-08T19:56:43Z

+    ManifestFile adapted = TrackedFileAdapters.asManifestFile(original);
+    TrackedFile result = TrackedFileAdapters.forManifestFile().wrap(adapted);
+    assertThat(result).isSameAs(original);
+    assertThat(result.formatVersion()).isZero();


Why does this pass and validate that the format version of the TrackedFile is 0? That seems odd to me. I think that a TrackedFile should typically use 4 when directly constructed.

I also think it is reasonable to drop this assertion entirely. It returns the original object.

Dropped the format-version 0 check on the unwrap test, and that test now constructs the tracked file without passing version 0.

isZero() remains on manifestTrackedFileAdapter because GenericManifestFile does not override formatVersion() (the interface default is 0).

Add reusable write-direction wrappers that present a legacy DataFile, DeleteFile, or ManifestFile as a v4 TrackedFile row for manifest serialization. Content stats are served by a reusable map-backed view over the file's stat maps. Generated-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>

Leave replaced counts as boxed zeros for consistency with other ManifestFile counters, unwrap already-adapted content files and manifest references, drop the formatVersion factory arg, and construct write wrappers from the TrackedFile writer schema instead of schema fragments. Report an absent partition or content stats as null rather than an empty struct, matching TrackedFileStruct. Newly written manifest-reference rows report format version 4; a rewrite of an already-adapted ManifestFile returns the original TrackedFile so a persisted 0 survives. Generated-by: Cursor Grok 4.6 Generated-by: Cursor Claude Opus 5 Co-authored-by: Cursor <cursoragent@cursor.com>

…e wrapped TrackedFile. The asManifestFile adapter inherited ManifestFile defaults, so a persisted format version and non-zero replaced counts were dropped on the ManifestFile view. Generated-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>

Drop StructLike and write-schema from TrackedFileAdapters, add ManifestFile modified counts, and align MapBackedContentStats with review feedback. Generated-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>

Reuse tracking wrappers when wrapping data and manifest files, implement StructLike on GeospatialBound, and treat pre-v4 manifests as Avro with EXISTING status. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

Geometry and geography bounds are already decoded by Conversions when given the column type, so decodeBound no longer needs a single-point special case. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

Look columns up by field id so field stats can be read from the maps without constructing the content-stats type first. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

Keep cached field stats when rebinding a file, and return null from a factory when the field has no stats struct. Look up the bound type by name. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

Assert mapped values instead of implementation details, matching review feedback on the v4 write-direction wrappers. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

The helpers are only used inside MapBackedContentStats. The manifest count errors stay valid for a null count. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

Cover manifest file-count failures in one parameterized test and fold the absent field-id case into field reuse, without repeating bound decoding. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

apache/main renamed Tracking.dvSnapshotId() to modifiedSnapshotId(). The write wrappers return null because manifest entries and manifest files have no such id. TrackedFileStruct also follows the partition field rename. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

apache/main moved formatVersion off TrackedFile onto manifest info. The write wrappers no longer implement it on TrackedFile; WrappedManifestInfo forwards the manifest's format version, and the manifest success test checks that forward. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

TrackedFile no longer carries equality_ids, so the write wrappers drop the equalityIds() overrides and size checks use totalBytes(). Generated-by: Cursor Grok 4.7

The row-count-only fixtures do not need the four null map arguments. Generated-by: Cursor Grok 4.7

rdblue · 2026-10-19T22:10:49Z

+    TrackedFile result =
+        TrackedFileAdapters.forDataFile(TABLE_SCHEMA)
+            .wrap(
+                newEntry()


newEntry returns an entry that uses a partitioned schema, but DATA_FILE uses the unpartitioned spec.

This is why in other test suites, I moved to using factory methods and constants that state whether they have content stats and/or partitioning. We should avoid mismatches here because we want test data that looks as close to real data as possible.

Also, does it make sense to have a newEntry() factory method? It looks like it's used here and in the next method. This suite is creating mismatched values because it is difficult for a reader to understand -- you have to go find newEntry to see what it's doing -- and that saves just a few lines. I would opt to have slightly longer tests that can be read and understood in one place.

Inlined the construction at both call sites and removed the helper.

new GenericManifestEntry<DataFile>( ManifestEntry.getSchema(UNPARTITIONED_SPEC.partitionType()).asStruct()) .wrapExisting(SNAPSHOT_ID, DATA_SEQUENCE_NUMBER, FILE_SEQUENCE_NUMBER, DATA_FILE)

The reuse test uses the same construction with file2 (instead of DATA_FILE).

rdblue · 2026-10-19T22:17:05Z

+  }
+
+  @Test
+  void dataTrackedFileAdapterUnwrapsToOriginalTrackedFile() {


Would be better to co-locate these with the other tests for wrap, like dataTrackedFileAdapterRejectsNullFile.

Moved only dataTrackedFileAdapterUnwrapsToOriginalTrackedFile next to dataTrackedFileAdapterRejectsNullFile.

manifestTrackedFileAdapterUnwrapsToOriginalTrackedFile stays with the other ManifestTrackedFile tests.

rdblue · 2026-10-19T22:18:41Z

+    assertThat(result.fileFormat()).isEqualTo(FileFormat.AVRO);
+    assertThat(result.tracking().status()).isEqualTo(EntryStatus.EXISTING);
+    assertThat(result.tracking().snapshotId()).isEqualTo(SNAPSHOT_ID);
+    assertThat(result.tracking().firstRowId()).isNull();


Why is this null? Manifests should always have first row ID set after they have been written. If the status is EXISTING then this should have a non-null first row ID. It's also the more common case.

A data manifest now uses FIRST_ROW_ID. A delete manifest stays null because a delete manifest's first_row_id is always null. The assertion follows the content.

I think that this is correct

rdblue · 2026-10-19T22:21:30Z

+    assertThat(result.tracking().snapshotId()).isEqualTo(SNAPSHOT_ID);
+    assertThat(result.tracking().firstRowId()).isNull();
+    assertThat(result.recordCount())
+        .isEqualTo(ADDED_FILES_COUNT + EXISTING_FILES_COUNT + DELETED_FILES_COUNT);


It isn't obvious from reading this test that this is correct. Looks like this is also the only test where newManifestFile(FileContent) is called. That should be embedded. (I also replied on the previous thread.)

Embedded the GenericManifestFile construction in manifestTrackedFileAdapter and removed newManifestFile helper.

rdblue

I think the tests look okay. I had a few comments but they are mostly about readability, not big issues.

The entry and manifest constructions sit next to the assertions they feed, and written data manifests keep a first row id. Generated-by: Cursor Grok 4.7

amogh-jahagirdar

Thanks @stevenzwu, I've been following the work offline and did another quick pass over it, seems right to me as well. I will go ahead and merge this; thanks @rdblue @gaborkaszab @anoopj @CTTY @anuragmantri for reviewing.

github-actions Bot added API core labels Jun 23, 2026

stevenzwu commented Jun 23, 2026

View reviewed changes

Comment thread api/src/main/java/org/apache/iceberg/ManifestFile.java

stevenzwu added this to V4: metadata tree Jun 23, 2026

gaborkaszab reviewed Jun 23, 2026

View reviewed changes

stevenzwu force-pushed the v4_write_direction_wrappers branch 5 times, most recently from c78d6b1 to 828e0c7 Compare June 25, 2026 21:35

github-actions Bot added docs build and removed API labels Jun 25, 2026

stevenzwu force-pushed the v4_write_direction_wrappers branch from 828e0c7 to fa86dc1 Compare June 25, 2026 22:18

github-actions Bot added the API label Jun 25, 2026

stevenzwu force-pushed the v4_write_direction_wrappers branch 5 times, most recently from fa46d3d to 55f5573 Compare June 26, 2026 05:57

stevenzwu mentioned this pull request Jun 26, 2026

Core: Adjustments for TrackedFileBuilder #16964

Closed

stevenzwu commented Jun 26, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TrackedFileBuilder.java Outdated

stevenzwu force-pushed the v4_write_direction_wrappers branch 8 times, most recently from b5c92a0 to d5c696b Compare June 28, 2026 06:09

rdblue reviewed Oct 8, 2026

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java Outdated

rdblue reviewed Oct 8, 2026

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java Outdated

rdblue reviewed Oct 8, 2026

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java Outdated

stevenzwu and others added 15 commits October 13, 2026 11:41

Core: Reshape v4 write adapters as conversion wrappers.

7b272a3

Drop StructLike and write-schema from TrackedFileAdapters, add ManifestFile modified counts, and align MapBackedContentStats with review feedback. Generated-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>

Core: Return map-backed stats without building the stats type.

b05210b

Look columns up by field id so field stats can be read from the maps without constructing the content-stats type first. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

Core: Reuse field stats across file wrap

cbc2f06

Keep cached field stats when rebinding a file, and return null from a factory when the field has no stats struct. Look up the bound type by name. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

Core: Tighten content stats and tracked file tests

6c7d039

Assert mapped values instead of implementation details, matching review feedback on the v4 write-direction wrappers. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

Core: Make field-stats helpers private and shorten manifest errors

4b07bfa

The helpers are only used inside MapBackedContentStats. The manifest count errors stay valid for a null count. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

minor test cleanup

1c54c40

Core: Align v4 wrapper tests with review

0f4e623

Cover manifest file-count failures in one parameterized test and fold the absent field-id case into field reuse, without repeating bound decoding. Generated-by: Cursor Grok 4.7 Co-authored-by: Cursor <cursoragent@cursor.com>

Core: Drop equality ids from v4 write wrappers

b1e4e30

TrackedFile no longer carries equality_ids, so the write wrappers drop the equalityIds() overrides and size checks use totalBytes(). Generated-by: Cursor Grok 4.7

rdblue reviewed Oct 19, 2026

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java Outdated

Core: Use the single-arg Metrics constructor in adapter tests

d1b9ebe

The row-count-only fixtures do not need the four null map arguments. Generated-by: Cursor Grok 4.7

rdblue reviewed Oct 19, 2026

View reviewed changes

rdblue approved these changes Oct 19, 2026

View reviewed changes

Core: Inline adapter test fixtures for review

2d84856

The entry and manifest constructions sit next to the assertions they feed, and written data manifests keep a first row id. Generated-by: Cursor Grok 4.7

amogh-jahagirdar approved these changes Oct 20, 2026

View reviewed changes

Conversation

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

Uh oh!

What

Benchmark summary

Uh oh!

Uh oh!

gaborkaszab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

rdblue Oct 8, 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!

Uh oh!

Uh oh!

Uh oh!

rdblue Oct 19, 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!

Choose a reason for hiding this comment

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

stevenzwu commented Jun 23, 2026 •

edited

Loading

rdblue Oct 8, 2026 •

edited

Loading

rdblue Oct 19, 2026 •

edited

Loading