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:34:56Z

+    FieldStats<?> stats = new MapBackedContentStats(schema).wrap(file).statsFor(fieldId);
+
+    Comparator<Object> comparator = Comparators.forType(type.asPrimitiveType());
+    assertThat(comparator.compare(stats.lowerBound(), lower)).isZero();


Can this call usingComparator instead of asserting isZero()?

Done. boundDecodingPerType uses usingComparator(comparator).isEqualTo(...).

rdblue · 2026-10-08T19:37:12Z

+    // FILE_WITH_STATS has no avg-size map.
+    assertThat(id.avgValueSizeInBytes()).isNull();
+
+    FieldStats<?> score = stats.statsFor(2);


Why are there no assertions for the bound values here?

In Iceberg, we still assert equality for floating point values because as a storage format, the bits must match exactly. Checks that the bounds are what we expect are valid because we are checking that the values are correctly deserialized and are present.

I think all of the accessors should be checked here.

Bound checks stay in boundDecodingPerType. countsAndTightBounds stays on the count maps, so I didn't add lower/upper asserts for fields 2–4. Field 1 still checks tightBounds() is false because the content-file maps have no tight-bounds flag.

rdblue · 2026-10-08T19:39:50Z

+    assertThat(stats.statsFor(1).lowerBound()).isEqualTo(500);
+    assertThat(stats.statsFor(1).upperBound()).isEqualTo(5000);
+    assertThat(stats.statsFor(1).valueCount()).isEqualTo(50L);
+    assertThat(stats.statsFor(2)).isNull();


Should there be a corresponding isNotNull() assertion for the FILE_WITH_STATS case?

Done. reuseRebindsFields asserts statsFor(2) is not null after FILE_WITH_STATS.

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

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


Co-locate reuse tests?

Done. wrapInvalidatesType now sits next to reuseRebindsFields.

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

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


Should this be rolled into the last test by adding ID 5 that is missing from the FILE_WITH_STATS maps?

Done. Folded this into reuseRebindsFields and removed absentIdIsRereadWhenPresentAgain. Field 5 is absent on FILE_WITH_STATS and present on file2. Field 2 stays off file2.

rdblue · 2026-10-08T19:42:25Z

+          PartitionData.EMPTY,
+          1024L,
+          METRICS_WITH_BOUNDS,
+          null,


Why use different constants? Does this change matter to the tests?

Done. DATA_FILE_WITH_METRICS now uses KEY_METADATA, SORT_ORDER_ID, and FIRST_ROW_ID.

rdblue · 2026-10-08T19:45:08Z

+        .hasMessageContaining("null");
+  }
+
+  private static void assertWrappedDataFileMatchesFileFields(TrackedFile result, DataFile file) {


Co-locate assertion helpers?

Leaving this assert helper where it is (after all test methods).

assertSameDataFile and assertSameDeleteFile predate this PR and were placed before some test methods. Didn't relocate them to avoid code churning.

rdblue · 2026-10-08T19:46:50Z

+        minSequenceNumber,
+        SNAPSHOT_ID,
+        partitions,
+        null,


Nit: always nice to identify what these null args represent:

Suggested change

null,

null, // sort order id

Labeled both nulls.

The first is key metadata, not sort order id.

The last is first row id. This helper is shared by DATA and DELETES, and a delete manifest's first row id must be null. Set it to null for both manifest types.

rdblue · 2026-10-08T19:47:52Z

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


Should other cases be tested as well?

Looks like this uses the oddly-named writeManifestFile that returns GenericManifestFile. Why not use manifestWithCounts instead, like the other tests do?

Ah, I see that this is validating the row count, not the file count. I would expect those tests to be similar though. The one for added files count uses a mock. The ones above for modified/replaced count have a method to create a mock, and this uses a different method.

I think this could use a little clean-up for consistency.

writeManifestFile is now newManifestFile.

wrap() calls manifestRecordCount that reads file counts. Invalid file counts would cause wrap to fail. File-count failures are now covered by manifestTrackedFileAdapterRejectsInvalidFilesCount. The file-count cases now share one mock, newManifestFileWithInvalidCount. It stubs every count to the normal constant, then overrides only the injected file-count failure.

I dropped the row-count cases, which were testing NPE when accessing null row counts after wrap. that is not an interesting test scenario.

rdblue · 2026-10-08T19:50:27Z

+        content, MANIFEST_SEQUENCE_NUMBER, MANIFEST_MIN_SEQUENCE_NUMBER, ADDED_ROWS_COUNT);
+  }
+
+  private static ManifestFile writeManifestFile(


I think this should have a better name since it is not writing a file. How about newManifestFile?

Renamed to newManifestFile. Only newManifestFile(ManifestContent) remains, and the counts are constants inside the helper.

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.

rdblue · 2026-10-08T19:55:47Z

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


Can this be combined with the test below? Looks like the only difference is that it tests manifest content. That can be done by parameterizing the method.

Combined into manifestTrackedFileAdapter(ManifestContent). DATA maps to DATA_MANIFEST and DELETES maps to DELETE_MANIFEST.

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).

rdblue · 2026-10-08T19:58:24Z

+    TrackedFile source = trackedFile(FileContent.DATA);
+    DataFile dataFile = TrackedFileAdapters.asDataFile(source, UNPARTITIONED);
+    TrackedFile roundTripped = TrackedFileAdapters.forDataFile(TABLE_SCHEMA).wrap(dataFile);
+    assertThat(roundTripped).isSameAs(source);


Nit: names are inconsistent between test cases. This uses source / roundTripped and the manifest equivalent uses original / result.

Both round trips now use original -> adapted -> roundTripped.

rdblue · 2026-10-08T20:01:24Z

+    assertThat(result.partition())
+        .usingComparator(Comparators.forType(PARTITIONED_SPEC.partitionType()))
+        .isEqualTo(PARTITION);
+    assertThat(TrackedFileAdapters.asDataFile(result, specsById(PARTITIONED_SPEC)).partition())


What is this assertion checking? asDataFile should return the original DataFile, which I would expect to be validated by a test like the ones below for the tracked file adapter unwrapping.

I think this assertion should be dropped since it is either testing double-wrapping (which we don't want) or it's testing that the original is returned by a different method (asDataFile) and neither of those are directly related to DataTrackedFile behavior for partition().

Removed the asDataFile partition assert. The partition check remains on dataTrackedFileAdapterKeepsPartitionTuple, using Comparators.forType.

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>

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

stevenzwu and others added 14 commits October 9, 2026 11:55

Core: Reshape v4 write adapters as conversion wrappers.

bd715ae

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.

c2cbf3e

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

27d111c

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

56873b4

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

15db2a6

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

7ab9f99

Core: Align v4 wrapper tests with review

f06c00a

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>

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!

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!

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!

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 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!

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 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!

stevenzwu commented Jun 23, 2026 •

edited

Loading

rdblue Oct 8, 2026 •

edited

Loading

rdblue Oct 8, 2026 •

edited

Loading

rdblue Oct 8, 2026 •

edited

Loading