Core: Add v4 TrackedFileAdapters to bridge Data/Delete Files by anoopj · Pull Request #16100 · apache/iceberg

anoopj · 2026-04-24T14:22:47Z

The adapter bridges TrackedFile to existing DataFile/DeleteFile APIs and would allow to minimize the v4 related code changes during scan planning and commits.

Closes#16222

anoopj · 2026-04-24T15:13:12Z

+ return new TrackedDeleteFile(file, spec);
+ }
+
+ // TODO: TrackedFile will likely get an explicit partition tuple field (using a union partition


This will change after the approach to store partition tuple is settled.

Steven and Russell replied on the dev list that they're good with a tracked partition tuple. I also confirmed with Dan and Amogh, so I think we are probably good to go. Maybe we should start getting the changes to the interfaces and implementations done? This would then only need to run the projection.

anoopj · 2026-04-28T17:11:48Z

+ return result.isEmpty() ? null : result;
+ }
+
+ static Map<Integer, Long> nullValueCounts(ContentStats stats) {


An open question is whether it's worth caching the stats (lazy/eager). I don't see a lot of repeated reads, so may not be worth it.

It's probably fine. I have a comment about this below.

rdblue · 2026-05-04T20:41:18Z

+
+ private TrackedFileAdapters() {}
+
+ static DataFile asDataFile(TrackedFile file, PartitionSpec spec) {


What was your reason not to make this a method of TrackedFile?
Also, PartitionSpec is tracked by the file itself, so is very strange to pass it in here. At a minimum, I would expect this to have a validation that the spec's ID matches the file's spec ID. But it would also be better to have some way to look up spec by ID instead of forcing the caller to do it and then validate that the caller did it correctly.

I intentionally kept the adapters outside of TrackedFile to avoid coupling it with Data/DeleteFile. It seemed like an adapter concern.
The tracked file keeps track of the spec ID, but not the spec itself. I saw some v3 code paths that followed a similar pattern of passing around specsById. Pretty much open to changing it if you have suggestions.

I changed the methods to take in specsById map instead.

rdblue · 2026-05-04T23:16:15Z

+ // each reported the full file size).
+ @Override
+ public long fileSizeInBytes() {
+ return dv.sizeInBytes();


I appreciate the comment. The decision here seems reasonable to me since we don't know the total Puffin file size.
We should also consider whether we want to have a field in the tracked_file struct for this. We originally wanted to use file_size_in_bytes for the Puffin file size so that we could determine when DVs should be compacted. But variance in the footer was a problem that prevented it from being used as intended.
@aokolnychyi should we revisit this?

even if we know the total Puffin file size, dv.sizeInBytes still seems the correct value here. A puffin file may contain thousands of DVs. A logical DeleteFile should only contain one DV. The DeleteFile size should be the DV size.

rdblue · 2026-05-04T23:39:15Z

+
+ @Override
+ public Map<Integer, ByteBuffer> lowerBounds() {
+ return TrackedFileAdapters.lowerBounds(file.contentStats());


This is okay, but a little concerning because the helper method here creates a new map every time it is called. Creating the map is an expensive operation because it allocates buffers to hold each column bound and serializes into that buffer. Then the map is thrown away rather than reused.
The evaluators themselves (for example, InclusiveMetricsEvaluator) only call these methods once to evaluate for the data file, but if multiple evaluators are used then we should expect a performance degredation.
On the other hand, we want to have evaluators that work directly with ContentStats instead of going through these methods. I think I'm fine with leaving this as-is for now, but we need to make sure that we use the right evaluators everywhere.

Agree. Will build a new evaluator that operates on content stats, as a followup. cc @nastra

Filed #16218 to track this.

Let's hold off on doing it right now. I think the ContentStats API is going to change.

rdblue · 2026-05-04T23:44:14Z

+ return new TrackedDataFile(file, spec);
+ }
+
+ static DeleteFile asDVDeleteFile(TrackedFile file, PartitionSpec spec) {


We will also need a way to wrap TrackedFile as a v2 DeleteFile for position deletes.

Ack. I will create a followup PR for this so that the size is manageable.

Added TrackedPositionDeleteFile

rdblue · 2026-05-05T23:07:52Z

+ private final Tracking tracking;
+ private final PartitionSpec spec;
+
+ private AbstractTrackedContentFile(TrackedFile file, PartitionSpec spec) {


Rather than Abstract, we typically use Base for the prefix. For example, BaseAction or BaseContentStats.

I think we can even drop the Base here with just TrackedContentFile, because ContentFile is a base class. This is also consistent with other classes like TrackedDataFile, TrackedDVDeleteFile.

I dropped the Base. @rdblue let me know if thats OK.

rdblue · 2026-05-05T23:13:02Z

+
+ @Override
+ public int specId() {
+ return spec.specId();


If the spec ID is set on file then it should be returned.
The last version was this:
returnfile.specId() != null ? file.specId() : 0;
The problem wasn't that it was returning file.specId(). When that spec ID is set, it is canonical and is the ID that was used to look up spec. The problem was that it was guessing ID 0 for the unpartitioned spec, which is not correct. The updated version should be this:
returnfile.specId() != null ? file.specId() : spec.specId();

Is it? It looks the same to me.

I had incorporated it in May. But recently reverted it yesterday due to a conflicting review feedback. My bad - I forgot our original discussion on why we added it. But with the new validation we added, just using spec.specId is provably correct. Let me know if that is reasonable.

Thanks for the reference. I agree with @stevenzwu that it should be the same. And now that we are checking it in the constructor, this should be fine.

rdblue · 2026-05-05T23:17:21Z

+ * <p>Subclasses provide {@code content()}, {@code firstRowId()}, {@code equalityFieldIds()}, and
+ * the copy methods.
+ */
+ private abstract static class AbstractTrackedContentFile<F extends ContentFile<F>>


It would be helpful to implement the version for v2 position deletes here as well. That would make it easier to evaluate the implementations here, although I suspect it will be fine.

Added it. It's actually fairly light.

stevenzwu · 2026-05-05T22:57:46Z

+ tracking.set(3, 11L);
+ tracking.set(5, 1000L);
+ tracking.setManifestLocation("s3://bucket/manifest.avro");
+ tracking.set(8, 7L);


switch to the builder added recently?

+1 as I feel setting all stuff by position is quite brittle

stevenzwu · 2026-05-05T23:51:07Z

+ private final Tracking tracking;
+ private final PartitionSpec spec;
+
+ private AbstractTrackedContentFile(TrackedFile file, PartitionSpec spec) {


I think we can even drop the Base here with just TrackedContentFile, because ContentFile is a base class. This is also consistent with other classes like TrackedDataFile, TrackedDVDeleteFile.

stevenzwu · 2026-05-06T00:07:43Z

+ // each reported the full file size).
+ @Override
+ public long fileSizeInBytes() {
+ return dv.sizeInBytes();


even if we know the total Puffin file size, dv.sizeInBytes still seems the correct value here. A puffin file may contain thousands of DVs. A logical DeleteFile should only contain one DV. The DeleteFile size should be the DV size.

stevenzwu · 2026-06-09T22:15:08Z

+ }
+ }
+
+ private static PartitionSpec resolveSpec(


a note for future. if this util method is useful in other settings, we can consider move it to a util class like TrackedFileUtil. but not now.

stevenzwu · 2026-06-10T03:14:43Z

The CVE scan failure are noises. They are not caused by this PR.

The fix has been merged. #16749

…e_count MetricsUtil.nullValueCounts(ContentStats) was added in apache#16100, when the FieldStats count accessors returned boxed Long, so a column without a null_value_count simply produced a null map entry. apache#17159 changed those accessors to primitive long. In v4 content stats, null_value_count is only stored for optional (nullable) columns: StatsUtil.fieldStatsStruct omits the field for required columns, so a deserialized FieldStatsStruct for a required column leaves it null. nullValueCounts now throws a NullPointerException unboxing that null. Add StatsUtil.tracksStat(fieldStatsType, fieldId, statOffset) and guard nullValueCounts with it, skipping columns whose stats struct does not track null_value_count (reported as unknown) instead of throwing, as nanValueCounts already filters by whether the metric applies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

github-actionsBot added the core label Apr 24, 2026

anoopj moved this to In review in V4: metadata tree Apr 24, 2026

anoopj added this to V4: metadata tree Apr 24, 2026

anoopj commented Apr 24, 2026
View reviewed changes

anoopj changed the title ~~[core] v4: Add TrackedFileAdapters to bridge Data/Delete Files~~Core: Add v4 TrackedFileAdapters to bridge Data/Delete FilesApr 24, 2026

anoopj commented Apr 28, 2026
View reviewed changes

stevenzwu reviewed Apr 28, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java

anoopj requested a review from stevenzwu April 30, 2026 15:10

rdblue reviewed May 4, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

rdblue reviewed May 4, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java

rdblue reviewed May 4, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

rdblue reviewed May 4, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java

rdblue reviewed May 4, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

rdblue reviewed May 4, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java

rdblue reviewed May 4, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

rdblue reviewed May 4, 2026
View reviewed changes

anoopj mentioned this pull request May 5, 2026
Implement metrics evaluators that work directly with ContentStats #16218
Open
3 tasks

anoopj requested a review from rdblue May 5, 2026 18:23

anoopj mentioned this pull request May 5, 2026
Add TrackedFile adapter for v2 position delete files #16221
Closed
3 tasks

rdblue reviewed May 5, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

rdblue reviewed May 5, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

rdblue reviewed May 5, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

rdblue reviewed May 5, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

stevenzwu reviewed May 6, 2026
View reviewed changes

anoopj added 7 commits June 8, 2026 16:02

Fix comment
6e4ea18

Fix failure
c12e004

Rebase
7a528f8

Fix spotless
5d758b2

Fix ordinals after rebasing
033c69c

Remove position delete from adapter
bd9c6bf

Incorporate feedback from Ryan
9fa6e79

anoopj force-pushed the v4-tracked-file-struct-adapters branch from 5ed9aeb to 9fa6e79Compare June 8, 2026 10:34

anoopj requested a review from rdblue June 8, 2026 10:35

stevenzwu reviewed Jun 9, 2026
View reviewed changes

PR feedback
ad06118

anoopj requested a review from stevenzwu June 10, 2026 00:06

stevenzwu approved these changes Jun 10, 2026
View reviewed changes

rdblue reviewed Jun 10, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

rdblue reviewed Jun 10, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

rdblue reviewed Jun 10, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java

rdblue reviewed Jun 10, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

rdblue reviewed Jun 10, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java Outdated

rdblue reviewed Jun 10, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/TrackedFileAdapters.java

Feedback
31c5726

anoopj requested a review from rdblue June 11, 2026 05:03

rdblue approved these changes Jun 11, 2026
View reviewed changes

rdblue merged commit 9c57bb5 into apache:mainJun 11, 2026
53 checks passed

github-project-automationBot moved this from In review to Done in V4: metadata tree Jun 11, 2026

anoopj deleted the v4-tracked-file-struct-adapters branch June 13, 2026 02:27

wombatu-kun mentioned this pull request Jun 19, 2026
Core: Add ManifestFile adapter for v4 tracked files #16867
Closed

nssalian added this to the Iceberg 1.12.0 milestone Jun 23, 2026

stevenzwu mentioned this pull request Jul 20, 2026
Core: guard MetricsUtil.nullValueCounts for columns without null_value_count #17306
Closed


		private TrackedFileAdapters() {}

		static DataFile asDataFile(TrackedFile file, PartitionSpec spec) {

Uh oh!

Conversation

anoopj commented Apr 24, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

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!

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!

anoopjJun 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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

anoopj commented Apr 24, 2026•
edited
Loading

anoopjJun 11, 2026•
edited
Loading