Core: Project tracked-file partitions onto the resolved spec by anoopj · Pull Request #18108 · apache/iceberg

anoopj · 2026-09-14T20:30:48Z

TrackedFileAdapters exposed the manifest's union-type partition tuple unchanged while reporting the file's own spec ID. Residual evaluation and partition-constant injection read the tuple by the spec's field ordinals, so on partition-evolved tables a later spec's field (at a different position in the union than in its own spec) was read from the wrong slot, silently dropping rows or injecting null partition constants.

Project each file's partition onto its resolved spec's partition type before exposing it, so partition() is always in the file's own spec order.

TrackedFileAdapters exposed the manifest's union-type partition tuple unchanged while reporting the file's own spec ID. Residual evaluation and partition-constant injection read the tuple by the spec's field ordinals, so on partition-evolved tables a later spec's field (at a different position in the union than in its own spec) was read from the wrong slot, silently dropping rows or injecting null partition constants. Project each file's partition onto its resolved spec's partition type before exposing it, so partition() is always in the file's own spec order.

Per review, the union->spec partition projection is TrackedFile's responsibility, not the adapter's or the reader's. TrackedFileStruct.partition() now projects the stored union tuple onto the file's own spec (lazily; copies materialize it into a spec-ordered PartitionData), and V4ManifestReader evaluates partition filters against partition() directly instead of projecting itself. Reverts the adapter-level projection.

… is read, not only on the filter path, so partition() stays spec-projected for select()/project() callers (not just filtered scans). - Cache the per-spec project-or-passthrough decision in TrackedFileStruct so the filter hot path is a map lookup instead of a per-row StructType comparison. - Add a round-trip test that a materialized spec-ordered partition survives Java and Kryo serialization.

Cover the no-filter path where the partition is projected: the reader must still project spec_id so partition() returns the file's own spec order, and a narrow nested partition selection must still read the whole union tuple so the projection onto the file's spec does not fail on an omitted field.

anoopj · 2026-09-15T17:38:20Z

+    // tuple (including data) so partition() can project it onto the file's data spec
+    try (V4ManifestReader reader =
+        V4ManifestReader.builder(manifest, io, specsById, TABLE_LOCATION)
+            .select("partition.id")


selecting id is intentional, btw. hopefully the comment on line 708 is clear.

yangshangqing95

Overall LGTM, just a few non-blocking questions.

yangshangqing95 · 2026-09-15T20:55:58Z

  }

  @Override
  public StructLike partition() {


Nit, non-blocking: could we make the TrackedFile.partition() contract explicit that the returned struct is shaped/ordered according to the file's resolved partition spec? This PR makes that invariant important for adapters and downstream evaluators, while the current method Javadoc only says it returns the partition.

I can clarify this, but to me it was a basic expectation from this field.

I agree that we should update the Javadoc to reflect this expectation.

Updated the javadocs

yangshangqing95 · 2026-09-15T21:00:43Z

+      assertThat(actual.partition().get(0, CharSequence.class)).hasToString("x");
+    }
+  }
+


Nit, non-blocking: would it be worth adding a regression test that goes through TrackedFileAdapters as well?

The current tests cover the projection behavior in TrackedFileStruct and V4ManifestReader well, but the original failure shows up downstream when the partition tuple is interpreted using the file's spec, e.g. during residual evaluation or partition-constant injection.

A test along the lines of V4ManifestReader -> TrackedFileAdapters -> partition/residual evaluation would help lock in the user-visible behavior and guard against the adapter boundary regressing in the future.

We do have coverage in the scan planner here: #17541 You can see that there is a test failure (which will be red until this PR is merged)

rdblue · 2026-09-15T23:50:31Z

    return fileSizeInBytes;
  }

+  void setSpecsById(Map<Integer, PartitionSpec> newSpecsById) {


I'm not a fan of needing to set specs on the class like this. I think maybe it would be cleaner to just do the work to remove partition().

Agree. Leaving it as is for now, and we can remove it as part of the change to recover partition tuple from stats.

rdblue · 2026-09-15T23:51:31Z

+    return projection != null ? projection.wrap(partitionData) : partitionData;
+  }
+
+  private StructProjection partitionProjection(int id) {


Nit: which ID? Generally better to be specific and use specId.

Actually there is a Checkstyle's HiddenField rule: the param specId collides with the specId field. I renamed it to partitionSpecId to avoid collision.

rdblue · 2026-09-15T23:57:09Z

+          id,
+          specType == null || partitionData.getPartitionType().equals(specType)
+              ? null
+              : StructProjection.create(partitionData.getPartitionType(), specType));


This put is too complicated and it forces the caller to handle null. The projection should be created and stored in a variable so put can be separate from the control flow for creating the projection.

I'd rather use a Function<PartitionData, StructLike> to project. That way this can return projection::wrap or Functions.identity and the caller is simpler.

That looks a lot nicer. Making that change.

FYI, I did this change, but this implementation had a thread safety issue, so I changed this.

rdblue · 2026-09-15T23:59:25Z

+    return partitionProjections.get(id);
+  }
+
+  private PartitionData materializedPartition() {


I think this class should use StructUtil.copy(partition()). There's no need to ensure that the result is a PartitionData. Won't any StructLike work?

Yes, I didn't know about StructlikeUtil.copy(). Thanks.

Actually that didn't work because StructLikeUtil.copy(...) returns a StructCopy which isn't Serializable. So it has to stay a serializable PartitionData and we need to keep PartitionData.copy()/copyFor(...).

Or should we make StructLikeUtil.copy serializable? (felt out of scope for this PR)

Reconcile the partition projection with main's V4ManifestReader rework (apache#18109 content stats, apache#18147 stats filtering, apache#18138 ManifestBitmap MDV)

github-actions Bot added the core label Sep 14, 2026

anoopj commented Sep 14, 2026

View reviewed changes

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

anoopj added 6 commits September 14, 2026 16:20

Trim comments

928b5f0

Cleanup

2211994

add comment

d16f726

anoopj commented Sep 15, 2026

View reviewed changes

yangshangqing95 approved these changes Sep 15, 2026

View reviewed changes

yangshangqing95 reviewed Sep 15, 2026

View reviewed changes

anoopj mentioned this pull request Sep 15, 2026

Core: Add V4 scan task planner #17541

Open

rdblue reviewed Sep 15, 2026

View reviewed changes

anoopj added this to V4: metadata tree Sep 16, 2026

anoopj moved this to In review in V4: metadata tree Sep 16, 2026

rdblue mentioned this pull request Sep 16, 2026

Core: Implement stats filtering in V4ManifestReader #18147

Merged

anoopj added 2 commits September 17, 2026 12:45

incorporate feedback

eba1fe6

Merge main

3603904

Reconcile the partition projection with main's V4ManifestReader rework (apache#18109 content stats, apache#18147 stats filtering, apache#18138 ManifestBitmap MDV)

anoopj requested a review from rdblue September 17, 2026 22:59

anoopj added 2 commits September 17, 2026 21:49

Fix bug and add regression test

09a4684

Make partition() thread-safe by projecting on read

bda24da

anoopj commented Sep 18, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TrackedFileStruct.java

Make partition() a thread-safe immutable read

b3cebd0

Conversation

anoopj commented Sep 14, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants