Core, Parquet: Allow for Writing Parquet/Avro Manifests in V4 by RussellSpitzer · Pull Request #15634 · apache/iceberg

RussellSpitzer · 2026-03-15T02:21:36Z

Extends V4 Manifest writer to allow it to write manifests in either Parquet or Avro based on the file extension. A default is also added to do Parquet Manifests in the SDK when the Version is 4. This could be parameterized later but that will require parameterizing the test suites so I decided on a single format (parquet) for now.

There are a few other required changes here outside of testing

Handling of splitOffsets in Parquet needs to be changed since BaseFile returns an immutable view which Parquet was attempting to re-use by clearing.
Unpartitioned Tables need special care since parquet cannot store empty structs in the schema. This means reading from parquet manifests means skipping the parquet field and then changing read offsets if the partition is not defined. The read code is shared between all versions at this time so this change effects older avro readers as well.
Some of the tests code for TestReplacePartitions assumed that you could validate against a slightly different vesrion of the table. This is a problem if the table you make is partitioned and the validation table is unpartitioned. It use to work ... accidently I think because we would make unpartitioned operations committed to a partitioned table.

--- Some Benchmarks
Note this is all done with Full reads, while we expect writes to be slower, reads should be faster when we actually do column specific projection. Since in this code the avro and parquet read paths are both doing full scans we don't expect them to be materially different.

I also deleted the old Manifest benchmarks which were specific to V1, and V1/V2 respectively and replaced them with a new benchmark which can be used on any version

RussellSpitzer · 2026-03-15T02:22:52Z

@anoopj I hear your PR #15049 will make this a lot cleaner, so I will review that on Monday :)

RussellSpitzer · 2026-03-16T14:32:12Z

Updated with the new benchmark code

RussellSpitzer · 2026-03-16T16:00:13Z

I have missed some deprecations and test rewrites, going to split those off to another branch to resolve first to not confuse this pr.

anoopj

Code looks great to me. A couple of minor comments.

anoopj · 2026-03-18T00:29:38Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java

      if (reuse != null) {
-        this.lastList = reuse;
+        // reuse containers may come from a different reader (e.g. Avro) with incompatible types
+        this.lastList = reuse instanceof ArrayList ? reuse : null;


Could you add a brief comment to explain why this looks for ArrayList? (was not super obvious to me). Same for the LinkedHashMap on line 980

anoopj · 2026-03-18T00:31:59Z

core/src/main/java/org/apache/iceberg/ManifestReader.java

+    FileFormat manifestFormat = FileFormat.fromFileName(inputFile.location());
+    Preconditions.checkArgument(
+        manifestFormat == FileFormat.AVRO,
+        "Reading manifest metadata is only supported for Avro manifests: %s",


I assume read support will come in a separate PR? Not sure if it would be useful, but #14577 is a quick and dirty prototype i did last year.

I'll look for the old discussions but we basically came to consensus that we just weren't going to read metadata out of manifests in this code path, all the deprecation work I've been doing is to remove that dependency so we won't ever have a read metadata pathway for internal data readers that aren't Avro

Now that I fixed all the tests, there is no code inside the Iceberg project that relies on this code path.

sfc-gh-rspitzer · 2026-03-18T02:13:15Z

Could you please check out #15656? It has about 1k more net changes to handle deprecations I missed. After that I'll come back to this one and finish it up. Thanks for checking on this PR as well!

RussellSpitzer · 2026-03-26T16:09:26Z

@anoopj Ok! Everything is good for review now. Please if you have a moment take a look

RussellSpitzer · 2026-03-26T16:09:59Z

core/src/jmh/java/org/apache/iceberg/ManifestBenchmark.java

@@ -18,23 +18,15 @@
 */
 package org.apache.iceberg;


I split off the Compression Benchmark into it's own file

RussellSpitzer · 2026-03-26T16:11:19Z

core/src/main/java/org/apache/iceberg/ManifestReader.java

+    // For older versions where the empty struct is present, making it optional is harmless.
    List<Types.NestedField> fields = Lists.newArrayList();
-    fields.addAll(projection.asStruct().fields());
+    for (Types.NestedField field : projection.asStruct().fields()) {


As the large comment above says, this is where we are handling the fact that Parquet doesn't like empty structs. We just inject an "empty - optional" parquet field so we can just get nulls in that slot.

As mentioned above, we can clean this up after v4 data structures get added, since we are folding partition into content_stats.

Yes i'm very excited for that. Not sure if you want to get that in first then this, or vice versa.

RussellSpitzer · 2026-03-26T16:13:31Z

core/src/main/java/org/apache/iceberg/ManifestReader.java

    }
    fields.add(MetadataColumns.ROW_POSITION);

-    CloseableIterable<ManifestEntry<F>> reader =


Looking back we don't need to change this at all now, i'll clean that up

RussellSpitzer · 2026-03-26T16:20:59Z

core/src/main/java/org/apache/iceberg/V4Metadata.java

+    fields.add(DataFile.CONTENT.asRequired());
+    fields.add(DataFile.FILE_PATH);
+    fields.add(DataFile.FILE_FORMAT);
+    if (!partitionType.fields().isEmpty()) {


We need to omit this field for unpartitioned tables if we are using parquet because parquet doesn't support the empty struct

In v4, we are folding partition into content_stats, so this problem goes away when we merge the v4 TrackedFile code in.

RussellSpitzer · 2026-03-26T16:38:07Z

core/src/test/java/org/apache/iceberg/TestBase.java

+  }
+
+  @SuppressWarnings("checkstyle:HiddenField")
+  void validateSnapshot(


We had to do some changes here but it wasn't actually related to V4 but was exposed by the "missing partition spec" field.

Basically we had some tests like
TestReplacePartitions

That would validate against the wrong table but it happened to work because AVRO didn't care if the "SPEC" information was incorrect, it would just go ahead and read the manifests. With Parquet there is now a missing field if the table is unpartitioned so you get an actual read error when you validate with the wrong table.

RussellSpitzer · 2026-03-26T16:39:12Z

core/src/test/java/org/apache/iceberg/TestReplacePartitions.java

    assertThat(TestTables.metadataVersion("unpartitioned")).isEqualTo(0);

-    commit(table, unpartitioned.newAppend().appendFile(FILE_A), branch);
+    commit(unpartitioned, unpartitioned.newAppend().appendFile(FILE_A), branch);


Here are tests that were being tested against the wrong table previously.

anoopj · 2026-03-28T00:00:27Z

core/src/main/java/org/apache/iceberg/V4Metadata.java

+    fields.add(DataFile.CONTENT.asRequired());
+    fields.add(DataFile.FILE_PATH);
+    fields.add(DataFile.FILE_FORMAT);
+    if (!partitionType.fields().isEmpty()) {


In v4, we are folding partition into content_stats, so this problem goes away when we merge the v4 TrackedFile code in.

anoopj · 2026-03-28T00:04:08Z

core/src/main/java/org/apache/iceberg/ManifestWriter.java

      Long firstRowId,
      Map<String, String> writerProperties) {
-    this.file = file.encryptingOutputFile();
+    this.format = FileFormat.fromFileName(file.encryptingOutputFile().location());


V4 will always have Parquet right? so the decision to use the right extension is made somewhere upstream?

@rdblue and i were debating whether or not we want to let engines still choose whether they want to use Avro or not. So for the moment I'm leaving it open. We can always close it down later when we actually change the spec.

anoopj · 2026-03-28T00:05:35Z

core/src/main/java/org/apache/iceberg/ManifestReader.java

+    // For older versions where the empty struct is present, making it optional is harmless.
    List<Types.NestedField> fields = Lists.newArrayList();
-    fields.addAll(projection.asStruct().fields());
+    for (Types.NestedField field : projection.asStruct().fields()) {


As mentioned above, we can clean this up after v4 data structures get added, since we are folding partition into content_stats.

anoopj

A big thank you to doing this, BTW. The benchmarking etc is so thorough.

…et by Default Extends V4 Manifest writer to allow it to write manfiests in either Parquet or Avro based on the file extension. A default is also added to do Parquet Manifests in the SDK when the Version is 4. This could be parameterized later but that will requrie parameterizing the test suites so I decied on a single format (parquet) for now. There are a few other requried changes here outside of testing 1. Handling of splitOffsets in Parquet needs to be changed since BaseFile returns an immutable view which Parquet was attempting to re-use by clearing. 2. Unpartitioned Tables need special care since parquet cannot store empty structs in the schema. This means reading from parquet manfiests means skipping the parquet field and then changing read offsets if the partition is not defined. The read code is shared between all versions at this time so this change effects older avro readers as well. 3. Some of the tests code for TestReplacePartitions assumed that you could validate against a slightly different vesrion of the table. This is a problem if the table you make is partitioned and the validation table is unpartitioned. It use to work ... accidently I think because we would make unpartitioned operations committed to a partitioned table.

- ManifestReader: Mark partition field optional for unpartitioned tables instead of removing it from the projection, preserving positional access and avoiding ClassCastException from shifted ordinals - BaseFile: Deep copy ByteBuffer values in copyByteBufferMap to prevent Parquet container reuse from corrupting bounds in copied files, which caused equality deletes to fail stats-based overlap checks - BaseFile: Guard against null partition value in internalSet - TestRewriteTablePathsAction: Simplify manifest file predicate to use name patterns instead of file extensions

- Collapse broken builder chain in ManifestReader.open() into a single fluent expression - Extract manifest format determination in SnapshotProducer into a private field computed once in the constructor - Replace magic format version 4 with TableMetadata.MIN_FORMAT_VERSION_PARQUET_MANIFESTS in tests - Parameterize TestManifestFileUtil across all format versions - Fix TestJdbcCatalog.manifestFiles to use exclusion filter instead of allowlisting file extensions - Improve ParquetValueReaders container reuse comments to reference specific BaseFile fields

Replace instanceof-then-cast with Java 16+ pattern matching to eliminate redundant casts in outputFile() and keyMetadataBuffer().

RussellSpitzer · 2026-03-30T15:59:24Z

Force pushed after rebasing on the test replace partitions fix #15798

github-actions bot added parquet core labels Mar 15, 2026

RussellSpitzer mentioned this pull request Mar 16, 2026

Core: Propagate Avro compression settings to manifest writers #15652

Merged

RussellSpitzer mentioned this pull request Mar 16, 2026

Core, Data, Delta, Flink, Kafka, Spark : Migrate callers off deprecated Snapshot file-access methods to Snapshot Changes #15656

Merged

anoopj reviewed Mar 18, 2026

View reviewed changes

RussellSpitzer force-pushed the ParquetManifests branch from a5cb9ac to 41d2c09 Compare March 24, 2026 22:02

github-actions bot added the spark label Mar 25, 2026

RussellSpitzer force-pushed the ParquetManifests branch from e42de44 to 5877a84 Compare March 25, 2026 21:54

RussellSpitzer commented Mar 26, 2026

View reviewed changes

RussellSpitzer mentioned this pull request Mar 27, 2026

Core: Fix TestReplacePartitions using wrong table for validation #15798

Merged

anoopj reviewed Mar 28, 2026

View reviewed changes

RussellSpitzer added 5 commits March 30, 2026 10:54

Core: Modulerize Benchmarks Better

f6927fa

Core: Use instanceof pattern matching in ManifestWriter

49fe6e2

Replace instanceof-then-cast with Java 16+ pattern matching to eliminate redundant casts in outputFile() and keyMetadataBuffer().

RussellSpitzer force-pushed the ParquetManifests branch from 1d1a626 to 49fe6e2 Compare March 30, 2026 16:07

Conversation

RussellSpitzer commented Mar 15, 2026

Uh oh!

RussellSpitzer commented Mar 15, 2026

Uh oh!

RussellSpitzer commented Mar 16, 2026

Uh oh!

RussellSpitzer commented Mar 16, 2026

Uh oh!

anoopj left a comment

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!

sfc-gh-rspitzer commented Mar 18, 2026

Uh oh!

RussellSpitzer commented Mar 26, 2026

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!

RussellSpitzer Mar 26, 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!

anoopj left a comment

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer commented Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RussellSpitzer Mar 26, 2026 •

edited

Loading