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!

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>

rdblue · 2026-10-06T20:15:50Z

+    return (FieldStats<T>) statsById.get(fieldId);
+  }
+
+  FieldStats<?> createFieldStats(int fieldId) {


Nit: private? Or is this exposed for testing?

rdblue · 2026-10-06T20:18:05Z

+    throw new UnsupportedOperationException("copy is not implemented");
+  }
+
+  boolean containsFieldInMaps(int fieldId) {


Nit: private?

rdblue · 2026-10-06T20:25:59Z

+      new PartitionData(PartitionSpec.unpartitioned().partitionType());
+
+  /**
+   * Stats on fields 1-4; field 5 is absent. Field 3 has no value_count entry so the absent-count


What is an "absent-count" getter?

Reworded the comment. Field 3 has no value-count entry, so valueCount() throws when it unboxes a null Long.

rdblue · 2026-10-06T20:28:24Z

+              4, buf(Types.StringType.get(), "zzz")));
+
+  @Test
+  void contentStatsTypeBuiltLazilyFromMapIds() {


This doesn't test that the type was built lazily and probably shouldn't. Correctness is what should be tested, not laziness.

Renamed this to typeMatchesIdsPresentInMaps. It checks the struct before wrap and the fields after wrap. It does not assert laziness.

rdblue · 2026-10-06T20:28:54Z

+
+    assertThat(stats.type().fields())
+        .extracting(field -> StatsUtil.toFieldId(field.fieldId()))
+        .containsExactlyInAnyOrder(1, 2, 3, 4);


I think that this should validate against statsReadSchema so that all fields are tested.

The type is now compared to statsReadSchema for the ids in the file. The comparison is order-independent because the id set is a HashSet. wrapInvalidatesType does the same for the single remaining id.

rdblue · 2026-10-06T20:31:56Z

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


I think that this should test all data types. We have similar tests in TestFieldStatsStruct that you can copy: https://github.com/apache/iceberg/blob/main/core/src/test/java/org/apache/iceberg/TestFieldStatsStruct.java#L203-L228

boundDecodingPerType is now a parameterized round trip over the same type and bound pairs as TestFieldStatsStruct.TYPES_AND_BOUNDS is private, so the tuples are copied here. Unknown still expects null bounds.

Geometry stays in geoBoundsDecode.

rdblue · 2026-10-06T20:36:55Z

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


This is alright, but I don't think that we need to test it. We may choose to implement this later.

Removed copyNotSupported and fieldStatsCopyNotSupported.

rdblue · 2026-10-06T20:38:54Z

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


The assertions here are mixed. Only the lower bound is tested for the example file, but lower bound, upper bound, value count, and another field entirely have assertions for file2. This could be "reuseRebindsFields` if you want to test them all and a negative case (id=2), but I would do either both bounds or all fields, not half-way between.

Renamed to reuseRebindsFields. Both files now assert lower bound, upper bound, and value count for field 1. The second file still expects statsFor(2) to be null.

rdblue · 2026-10-06T20:39:45Z

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


I would remove this. Correctness doesn't depend on reusing stats and I think the tests should validate correctness, not that a particular implementation choice was made.

Removed wrapReusesFieldStats. absentIdIsRereadWhenPresentAgain no longer checks instance identity. It still checks that the id is absent, then present again with the new file's values.

rdblue · 2026-10-06T20:41:42Z

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


I don't think that this is needed, especially if it requires extra code injected to validate caching.

Removed outOfRangeFieldStatsAreNullAndCached and CountingContentStats.

rdblue · 2026-10-06T20:42:34Z

+      Map<Integer, ByteBuffer> upperBounds) {
+    Metrics metrics =
+        new Metrics(
+            100L, null, valueCounts, nullValueCounts, nanValueCounts, lowerBounds, upperBounds);


I think it would be better to pass in 100 rather than assuming it matches value counts.

dataFile helper now takes the record count and passes it into Metrics. Each call site passes 100L.

rdblue · 2026-10-06T20:46:27Z


+  @Test
+  void manifestFileAdapterKeepsZeroFormatVersion() {
+    TrackedFile original = dummyTrackedFile(FileContent.DATA_MANIFEST, 0);


It is unclear what is being tested here because of the use of dummyTrackedFile. Is this a tracked file? Why would a tracked file have a format version of 0?

Removed manifestFileAdapterKeepsZeroFormatVersion. dataManifestTrackedFileAdapter already wraps a real manifest whose format version is 0.

rdblue · 2026-10-06T20:48:08Z

+          PartitionData.EMPTY,
+          1024L,
+          new Metrics(100L, null, null, null, null),
+          null,


What are these null values and why should they not be tested?

DATA_FILE now uses new Metrics(100L), KEY_METADATA, a sort order id, and FIRST_ROW_ID, and the field helper asserts those values. firstRowId is checked on the existing-entry path. wrapAppend still suppresses it.

rdblue · 2026-10-06T20:48:38Z

  }

+  @Test
+  void dataTrackedFileAdapterHasNoTracking() {


I think this is too specific should be combined with other cases. There should be a test that validates the behavior for DATA_FILE and this should be part of that test.

Kept this as the single wrap(DataFile) test and renamed it to dataTrackedFileAdapterFromDataFile. It still expects tracking to be null.

rdblue · 2026-10-06T20:49:28Z

+
+  @Test
+  void dataTrackedFileAdapterFromAddedManifestEntry() {
+    TrackedFile result = TrackedFileAdapters.forDataFile(TABLE_SCHEMA).wrap(addedEntry(DATA_FILE));


Entry is pretty small so it would be great to just embed the information here rather than needing to go look at what addedEntry sets for defaults.

Removed addedEntry, existingEntry, and deletedEntry. The remaining tests call wrapExisting directly.

rdblue · 2026-10-06T20:50:40Z

+
+    assertThat(result.tracking().status()).isEqualTo(EntryStatus.ADDED);
+    assertThat(result.tracking().snapshotId()).isEqualTo(SNAPSHOT_ID);
+    assertThat(result.tracking().dataSequenceNumber()).isNull();


Why are the sequence numbers null? Are they not mapped properly or are they set to null in Tracking? If it is that they are null in tracking, I think this should pass real values because they should be non-null when converting from a manifest entry.

Those nulls came from wrapAppend, which stores a null data sequence number. The existing-entry test now asserts DATA_SEQUENCE_NUMBER and FILE_SEQUENCE_NUMBER.

rdblue · 2026-10-06T20:50:51Z

+    assertThat(result.tracking().snapshotId()).isEqualTo(SNAPSHOT_ID);
+    assertThat(result.tracking().dataSequenceNumber()).isNull();
+    assertThat(result.tracking().fileSequenceNumber()).isNull();
+    assertThat(result.tracking().firstRowId()).isNull();


Should be tested

The existing-entry test builds the file with FIRST_ROW_ID and asserts tracking().firstRowId().

rdblue · 2026-10-06T20:51:53Z

+    TrackedFile result = TrackedFileAdapters.forDataFile(TABLE_SCHEMA).wrap(DATA_FILE);
+
+    assertThat(result.tracking()).isNull();
+    assertWrappedDataFileMatchesFileFields(result, DATA_FILE);


Good that this is validated. That means that this test is correct and it's just the name that is wrong.

Renamed to dataTrackedFileAdapterFromDataFile.

rdblue · 2026-10-06T20:52:31Z

+    assertWrappedDataFileMatchesFileFields(result, DATA_FILE);
+    assertThatThrownBy(result::formatVersion)
+        .isInstanceOf(IllegalStateException.class)
+        .hasMessage("Format version is assigned at write time");


Hm. Should this be 0 since it is converted from v3 or earlier?

I think this should keep throwing. A v3 DataFile has no format version to copy. On the manifest path, 0 is forwarded from ManifestFile.formatVersion when the manifest predates format tracking.

We also discussed on Monday's sync that format version should be moved to manifest_info struct that should be set at write time.

rdblue · 2026-10-06T20:54:17Z

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


I don't think this is needed. We know from other tests that the snapshot ID is plumbed through correctly. We don't need to validate that it is plumbed correctly when it takes a specific value (null).

Removed this. wrapAppend stores the snapshot id it is given, and the existing-entry test already checks a non-null id.

rdblue · 2026-10-06T20:55:33Z

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


I think this is a better test than the added version because it checks values. But we don't need two. The point here is to check that fields are mapped or defaulted correctly in the adapter, not to check that specific cases work.

Kept dataTrackedFileAdapterFromExistingManifestEntry and removed the added-entry test. The existing-entry test inlines wrapExisting and checks status, snapshot id, both sequence numbers, first row id, manifest position, and the file fields.

rdblue · 2026-10-06T20:55:42Z

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


Not needed.

Removed dataTrackedFileAdapterFromDeletedManifestEntry.

rdblue · 2026-10-06T20:56:15Z

+    TrackedFileAdapters.DataTrackedFile adapter = TrackedFileAdapters.forDataFile(TABLE_SCHEMA);
+
+    adapter.wrap(DATA_FILE);
+    assertThat(adapter.location()).isEqualTo(DATA_FILE_LOCATION);


There's a helper to validate all fields, so we should use it.

dataTrackedFileAdapterReuse now calls assertWrappedDataFileMatchesFileFields after each wrap. The test only asserts tracking itself: null, then EXISTING and the manifest position.

rdblue · 2026-10-06T20:58:38Z

+            TrackedFileAdapters.asDataFile(result, specsById(PARTITIONED_SPEC))
+                .partition()
+                .get(0, String.class))
+        .isEqualTo("books");


What is this assertion doing?

I would expect this to use a struct comparator to verify that PARTITION matches result.partition().

The partition is compared with Comparators.forType on the tracked file and again after asDataFile.

rdblue · 2026-10-06T21:00:10Z

+
+    DataFile roundTripped = TrackedFileAdapters.asDataFile(tracked, specs);
+
+    assertThat(roundTripped.content()).isEqualTo(FileContent.DATA);


This should validate that the returned file is the same as the original. That is, it should assert that the file was unwrapped, not equal.

The reason to validate this is to ensure that we can rely on the assumption that double wrapping does not occur so you are always wrapping a real DataFile.

Renamed this to trackedFileDoubleWrapRoundTrip. It builds a fresh TrackedFile, converts it with asDataFile, and asserts wrap returns that same TrackedFile.

asDataFile always allocates a new TrackedDataFile, so the DataFile to TrackedFile to DataFile path is not identity. The original-file check is only in wrap, when the argument is already a TrackedDataFile.

rdblue · 2026-10-06T21:03:26Z

+
+  private static ManifestFile writeManifestFile(
+      ManifestContent content, long sequenceNumber, long minSequenceNumber) {
+    return writeManifestFile(content, sequenceNumber, minSequenceNumber, 200L);


I'd prefer having constants defined at the top and to minimize these helper methods since you have to look at how they work in order to see if a test is valid or correct.

Removed the unused 3-arg writeManifestFile and hoisted the manifest counts and sequence numbers the tests assert. The 4-arg overload stays because one rejection test passes a null added-rows count. manifestWithCounts stays for those rejection cases.

rdblue · 2026-10-06T21:04:34Z

+    return dummyTrackedFile(contentType, FORMAT_VERSION_V4);
+  }
+
+  private static TrackedFileStruct dummyTrackedFile(FileContent contentType, int formatVersion) {


Minor: Why "dummy" here but almost nowhere else? I don't think this naming convention is a good one. Dummy is not telling the reader anything significant. And in fact, it is misleading because this creates a TrackedFileStruct rather than something else.

Renamed dummyTrackedFile to trackedFile.

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>

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

Core: Reuse field stats across file wrap

3008d15

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>

rdblue reviewed Oct 6, 2026

View reviewed changes

stevenzwu and others added 2 commits October 6, 2026 18:39

Core: Tighten content stats and tracked file tests

494c61e

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

0a19d5a

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>


		DataFile roundTripped = TrackedFileAdapters.asDataFile(tracked, specs);

		assertThat(roundTripped.content()).isEqualTo(FileContent.DATA);

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!

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

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

stevenzwu commented Jun 23, 2026 •

edited

Loading

rdblue Oct 6, 2026 •

edited

Loading

rdblue Oct 6, 2026 •

edited

Loading