Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions core/src/main/java/org/apache/iceberg/Partitioning.java
Original file line number Diff line number Diff line change
Expand Up @@ -238,9 +238,18 @@ public static StructType groupingKeyType(Schema schema, Collection<PartitionSpec
* @return the constructed unified partition type
*/
public static StructType partitionType(Table table) {
Collection<PartitionSpec> specs = table.specs().values();
return buildPartitionProjectionType(
"table partition", specs, allActiveFieldIds(table.schema(), specs));
return partitionType(table.schema(), table.specs().values());
}

/**
* Builds a unified partition type from a schema and its specs, unioning every partition field
* whose source column is present in the schema.
*
* @param schema the schema used to determine which partition fields are active
* @param specs the partition specs to unify
Comment thread
anoopj marked this conversation as resolved.
*/
static StructType partitionType(Schema schema, Collection<PartitionSpec> specs) {
return buildPartitionProjectionType("table partition", specs, allActiveFieldIds(schema, specs));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think using allActiveFieldIds is correct here.

That method will filter out partition fields when the source is no longer part of the table schema. In some cases (bucket joins), that is fine because the data is still partitioned by a subset of its partition fields.

However, for a partition type that can hold any partition tuple, it doesn't work. Equality deletes are matched using partition tuple equality. For example, say I have file_a.parquet in partition (id_bucket=7, is_test=false), and I also have a delete file deletes.parquet in partition (id_bucket=7, is_test=true). The delete file does not apply to the data file because they are in different partitions.

With the use of allActiveFieldIds here, dropping the is_test identity partition and then the is_test column from the table would result in a unified partition type that only has id_bucket. As a result, checking whether the partition matches (and the deletes should be applied) will pass for file_a.parquet and deletes.parquet.

This needs to union all partition fields so that we don't drop tuple values that define equality.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out - I didn't think about this scenario. We can fix it by dropping the filter on active fields, but there might be another problem: partition specs don't store the type, and we derive it based on the type of the source column. So if the source column doesn't exist in the schema anymore, we can't derive the partition type. I think this might be working in v3 because of the way the reader is implemented: the reader gets the schema from the Avro schema, and uses it instead of the schema from the metadata.

Maybe the Parquet reader can also work that way (can verify), but is this by design? Does the other Iceberg implementations actually work in this scenario in v3 if this detail is not in the spec?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if the source column doesn't exist in the schema anymore, we can't derive the partition type.

This is a good point to think about, but I think it will not affect what we do in practice. It's possible to remove old partition specs, but in practice this hardly ever happens. In addition, the only cases where we can't determine the output type of the transform are identity and truncate transforms. Truncation isn't very common and identity columns are unlikely to be removed from the table.

Because there are good reasons to think this case is unlikely, I think we should move forward assuming that the partition specs will be there. If we want to be more careful, we can require a metadata check to remove old specs to avoid breaking anything. Also, we can always go back and get the type from existing metadata files at read time if we can't determine the type.

I've also thought about storing the output type of transforms when we add column ranges for partition output values, like we do for UDF result types. We may just need to solve this by storing the output type.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. partitionType now unions all partition fields across specs and the reader's builder no longer takes a table schema. I left the existing partitionType(Table) unchanged since fixing it has wider implications; we can revisit separately. Added a test that a partition field survives dropping its source column.

I agree the output type is only underivable for identity and truncate, but PartitionSpec.partitionType() currently returns unknown whenever the source column is missing so today even a bucket field degrades. If it makes sense, I can do a follow-up that only falls back to unknown for source-dependent transforms, which shrinks this to the rare case you described. I also did some code reading in Iceberg rust, which seems to have a hard failure, possibly because unknown type is not supported yet in Iceberg Rust.

BTW +1 to storing the output type in metadata. v4 seems like the right time to add it since we're already revising the spec. It seems like a good overall improvement. Happy to write that up.

}

/**
Expand Down
282 changes: 282 additions & 0 deletions core/src/main/java/org/apache/iceberg/V4ManifestReader.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,282 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.iceberg;

import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import org.apache.iceberg.expressions.Evaluator;
import org.apache.iceberg.expressions.Expression;
import org.apache.iceberg.expressions.Expressions;
import org.apache.iceberg.expressions.Projections;
import org.apache.iceberg.io.CloseableGroup;
import org.apache.iceberg.io.CloseableIterable;
import org.apache.iceberg.io.CloseableIterator;
import org.apache.iceberg.io.InputFile;
import org.apache.iceberg.metrics.ScanMetrics;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.StructProjection;

/** Reader that reads a v4+ manifest file as {@link TrackedFile}s. */
class V4ManifestReader extends CloseableGroup implements CloseableIterable<TrackedFile> {

@stevenzwu stevenzwu Jun 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this name V4ManifestReader would become stale when V5 rolls in. maybe TrackedFileManifestReader or just TrackedFileReader.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TrackedFile is an abstraction that we want to keep internal for now. V4ManifestReader is the best name I could come up with. Open to other suggestions here. cc @rdblue for his thoughts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class implements and returns a CloseableIterable<TrackedFile> so we don't really keep TrackedFile internal in my opinion. Following this design we can name this TrackedFileReader.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is actually a good point. Leaving it open to hear from others.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TrackedFile is internal in the sense that it is not a public interface (at least not yet). This class TrackedFileReader would probably only be package private.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point, but I don't have a name suggestion that is better right now. Let's keep it in mind.

// Tracking fields read on the scan path. Omits the change-tracking fields (dv_snapshot_id,
// deleted_positions, replaced_positions) that a scan does not need. row_position backs
// Tracking.manifestPos.
private static final Types.StructType SCAN_TRACKING =
Types.StructType.of(
Tracking.STATUS,
Tracking.SNAPSHOT_ID,
Tracking.SEQUENCE_NUMBER,
Tracking.FILE_SEQUENCE_NUMBER,
Tracking.FIRST_ROW_ID,
MetadataColumns.ROW_POSITION);

private final InputFile file;
private final Types.StructType partitionType;
private final Schema fileProjection;
private final ScanMetrics scanMetrics;

// partition pruning state, keyed by spec ID; empty when no filtering is required
Comment thread
anoopj marked this conversation as resolved.
private final Map<Integer, Evaluator> partitionEvaluators;
private final Map<Integer, StructProjection> partitionProjections;

private V4ManifestReader(
InputFile file,
Types.StructType partitionType,
Map<Integer, Evaluator> partitionEvaluators,
Map<Integer, StructProjection> partitionProjections,
Schema fileProjection,
ScanMetrics scanMetrics) {
this.file = file;
this.partitionType = partitionType;
this.partitionEvaluators = partitionEvaluators;
this.partitionProjections = partitionProjections;
this.fileProjection = fileProjection;
this.scanMetrics = scanMetrics;
}

static Builder builder(
InputFile file, Schema tableSchema, Map<Integer, PartitionSpec> specsById) {
return new Builder(file, tableSchema, specsById);
}

/** Returns all tracked files in this manifest, regardless of status. */
CloseableIterable<TrackedFile> allFiles() {
return files(false /* all files */);
}

/** Returns tracked files whose tracking {@link Tracking#isLive() is live}. */
CloseableIterable<TrackedFile> liveFiles() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the original ManifestReader, the idea was to configure the reader itself and then be able to inspect its configuration. In that model, we had similar methods to select just the live entries for all entries, but I think this no longer makes sense with the builder model. Shouldn't these be builder config that gets passed in? Then the the reader becomes basically a ClosableIterable that carries out filtering.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I was following the original ManifestReader as the model. The reader is now just a CloseableIterable that applies the configured partition/live filters. I also added a reuseContainers option for scan planning.

return files(true /* only live files */);
}

/** Returns live tracked files. Makes defensive copies before returning. */
@Override
public CloseableIterator<TrackedFile> iterator() {
return CloseableIterable.transform(liveFiles(), TrackedFile::copy).iterator();
}

private CloseableIterable<TrackedFile> files(boolean onlyLive) {
CloseableIterable<TrackedFile> entries = CloseableIterable.transform(open(), this::prepare);
if (!partitionEvaluators.isEmpty()) {
entries = CloseableIterable.filter(entries, this::matchesPartition);
}

if (onlyLive) {
entries = CloseableIterable.filter(entries, entry -> entry.tracking().isLive());
}

return entries;
}

private boolean matchesPartition(TrackedFile trackedFile) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we want to drop tuple-based pruning entirely and push predicates against partition-transform-expression columns in content_stats. Under that model the partition tuple is only needed to match data files to equality-delete files.

I am planning to bring up the discussion in Monday's sync.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a followup of today's discussion, we will keep the partition tuple for planning/pruning for now and we can move to content stats based pruning later. This implementation in this PR is purely a pruning optimization, so swapping/removing it has no correctness implications.

FileContent content = trackedFile.contentType();
if (content == FileContent.DATA_MANIFEST || content == FileContent.DELETE_MANIFEST) {
// manifest references are expanded later and are not pruned by the partition filter
return true;
}

Integer specId = trackedFile.specId();
Evaluator evaluator = specId != null ? partitionEvaluators.get(specId) : null;
StructProjection projection = specId != null ? partitionProjections.get(specId) : null;
Preconditions.checkState(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to wrap my head around this partition filtering: specId is null if the file was written when the table was unpartitioned, if Im not mistaken. Do we want to throw an exception here if there are such files and the user provided a rowFilter?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a table goes from unpartitioned, to partitioned, new files also can be written in the unpartitioned space - this is a supported use case. So an unpartitioned file should not throw. Instead, it should not get filtered out by partition filtering, so that filters should apply to rows instead. e.g. table is partitioned by a color string column, and an unparitioned file could mix rows from different colors.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was that specId = null is a valid state, but we throw an exception here because of that because both the evaluator and the projection will be null.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specID is a required field, so it should not be null. Even unpartitioned data has a spec (the unpartitioned spec).

Also, any spec that exists within the table metadata is valid and can be written to directly (it isn't limited to older data written and then spec being evolved).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specId must be null for leaf manifest entry in the root manifest file, as a leaf manifest file can contain data file entries with mixed spec. It doesn't make sense to populate a single specId. Leaf manifest file entry should only have partition_summary field populated.

Hence the evaluation and preconditions check seem to only apply to data file entries in a leaf manifest file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specId is an optional field in the V4 schema. I double checked the proposal doc and the spec PR and none of them gives extra info on the behavior difference between leaf manifest and data/delete file entries.

I get the point, we return early for leaf manifests so here we only have data/delete entries where we expect specId not to be null. I think in the spec we should articulate better that it is mandatory for those entries. Will leave a comment.

Since the field is optional, shouldn't we have an extra Preconditions check to verify that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the spec needs to be a bit clear on this. I have a slight preference to make it optional for data/delete files as well, for truly unpartitioned cases. I will leave this thread open till we resolve this.

evaluator != null && projection != null,
"Cannot apply partition filter: spec ID %s is not one of the known specs %s in manifest %s",
specId,
partitionEvaluators.keySet(),
file.location());

boolean matches = evaluator.eval(projection.wrap(trackedFile.partition()));
if (!matches) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the CloseableIterable.filter(skipCounter, iterable, pred) method to take care of the counters and the filtering? Like how we do it in ManifestReader:

CloseableIterable.filter(
          content == FileType.DATA_FILES
              ? scanMetrics.skippedDataFiles()
              : scanMetrics.skippedDeleteFiles(),
          onlyLive ? filterLiveEntries(entries) : entries,
          entry ->
              entry != null
                  && evaluator.eval(entry.file().partition())
                  && metricsEvaluator.eval(entry.file())
                  && inPartitionSet(entry.file()));

Maybe constructing a predicate for partition filter and use CloseableIterable.filter?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking a second look, for this probably we should know the contentType beforehand, but it depends on the TrackedFile entry.
Just an idea: would it make sense to introduce a CloseableIterable.filter(counter, iterable, pred) version where counter is not a Counter type but "entry -> Counter" function?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, your second comment is correct. A v4 manifest can mix data files, delete files, and manifest references in one file. So the counter depends on each entry's contentType and can only be selected inside the predicate. The current code is quite simple to undersand.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope #17118 can help here where filter can now provide onKeep/onSkip callback and counters can be configured directly in the callback with more flexibility for the heterogeneous entry of TrackedFile

if (content == FileContent.DATA) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about manifests?

scanMetrics.skippedDataFiles().increment();
} else {
scanMetrics.skippedDeleteFiles().increment();
}
}

return matches;
}

private CloseableIterable<TrackedFile> open() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

firstRowId is not supported in this version?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, row lineage isn't wired up in this reader yet.

FileFormat format = FileFormat.fromFileName(file.location());
Preconditions.checkArgument(
format != null, "Cannot determine format of manifest: %s", file.location());

CloseableIterable<TrackedFile> reader =
InternalData.read(format, file)
.project(readSchema())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're missing CONTENT_TYPE, which always needs to be projected due to the matchesPartition relying on it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Added content_type as always projected.

.setRootType(TrackedFileStruct.class)
.setCustomType(TrackedFile.TRACKING.fieldId(), TrackingStruct.class)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe a TODO that ContentStats is not covered now?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally we don't add many TODOs in the code (I got past feedback on this).

@rdblue rdblue Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not for things that are currently being worked on. The TODO should not catch anything. Tests should catch when something is missing.

.setCustomType(TrackedFile.DELETION_VECTOR.fieldId(), DeletionVectorStruct.class)
.setCustomType(TrackedFile.MANIFEST_INFO.fieldId(), ManifestInfoStruct.class)
.setCustomType(TrackedFile.PARTITION_ID, PartitionData.class)
.reuseContainers()
.build();
addCloseable(reader);
return reader;
}

private TrackedFile prepare(TrackedFile trackedFile) {
Tracking tracking = trackedFile.tracking();
// manifestLocation is not stored in the manifest; the reader fills it from the file location.
// manifestPos is filled from ROW_POSITION while reading the tracking struct.
if (tracking instanceof TrackingStruct) {
((TrackingStruct) tracking).setManifestLocation(file.location());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We populate Tracking.manifestPos using a metadata column ROW_POSITION, while we populate Tracking.manifestLocation manually here. For the latter, is there a reason we can't use MetadataColumns.FILE_PATH to be consistent?
We could get rid of the custom setter TrackingStruct.setManifestlocation method too and let it flow through internalSet getByPos methods.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They look symmetric but the underlying mechanism differs. ROW_POSITION is synthesized by the reader itself. FILE_PATH us only populated when the caller injects it. I'd lean toward keeping the manual set for now

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the difficulty here is that for InternalData API we don't have a way to pass the constants map to the underlying reader, right?
Would it worth considering as an improvement? For me both manifestLocation and manifestPos seems regular metadata columns, we just don't have the plumbing currently to make the former act like a metadata column.

I wonder what others say. Probably is an overkill at this point.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manifestLocation might be able to be populated via constant map. But we didn't populate the manifestPos field, which may have to be populated via a setter method in this reader.

}

return trackedFile;
}

private Schema readSchema() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the builder to handle schema projection. Why do this here instead of in the builder?

Types.StructType fullType =
TrackedFile.schemaWithContentStats(partitionType, Types.StructType.of());
boolean unpartitioned = partitionType.fields().isEmpty();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't an empty partition type already be handled by the schema generation in the previous statement?


Set<Integer> projectedIds = null;
if (fileProjection != null) {
projectedIds =
fileProjection.asStruct().fields().stream()
.map(Types.NestedField::fieldId)
.collect(Collectors.toCollection(Sets::newHashSet));

// Always project tracking and content type. status drives live-file filtering, and content
// type distinguishes data, delete, and manifest-reference entries.
projectedIds.add(TrackedFile.TRACKING.fieldId());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything else needed from tracking? Not exactly sure how much projection gives us, but can't we project TRACKING.STATUS.fieldId() if nothing else is needed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we only rely on status now, and projecting only that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are wrong here with adding only the status. See my above comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please see my comment above.

projectedIds.add(TrackedFile.CONTENT_TYPE.fieldId());

// Force-project the remaining fields the partition filter reads, regardless of caller
// projection.
if (!partitionEvaluators.isEmpty()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PARTITION_ID should be force-added here too when partitionEvaluators is non-empty. matchesPartition calls evaluator.eval(projection.wrap(trackedFile.partition())), so a caller that passes a fileProjection excluding partition + a row filter ends up evaluating against an empty tuple and prunes every row.

Same reasoning as spec_id on the line above: fields the reader itself needs for filtering must be projected regardless of what the caller asked for. A test covering the caller-projected-subset + filter combination would surface this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed such that we will be force-projecting partition (alongside spec_id) when a filter is active. Also added testPartitionFilterForceProjectsFilterFields() as a test

projectedIds.add(TrackedFile.SPEC_ID.fieldId());
projectedIds.add(TrackedFile.PARTITION_ID);
}
}

List<Types.NestedField> fields = Lists.newArrayList();
for (Types.NestedField field : fullType.fields()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this performing its own projection rather than using TypeUtil methods?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know about TypeUtil. Fixed.

if (projectedIds != null && !projectedIds.contains(field.fieldId())) {
continue;
}

if (field.fieldId() == TrackedFile.TRACKING.fieldId()) {
fields.add(
Types.NestedField.required(
TrackedFile.TRACKING.fieldId(),
TrackedFile.TRACKING.name(),
SCAN_TRACKING,
TrackedFile.TRACKING.doc()));
} else if (field.fieldId() == TrackedFile.CONTENT_STATS_ID) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is because temporarily we don't read stats. I'm wondering what it would take to avoid this branch and read the stats even if we don't use them.

@anoopj anoopj Jun 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent here is actually to avoid reading stats we don't use. content_stats is the heaviest part of a manifest entry, so reading it when nothing consumes it is wasted I/O. That's the same reason v3's ManifestReader drops stats unless a filter needs them. So I'd lean away from reading them unconditionally; the end state is to read content_stats only when projected/needed, not always. This branch is the interim stand-in for that until content-stats reading is implemented.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! Projecting stats makes total sense.

I think most of my comment on this area are for a single reason: for me this readSchema() function seems a bit more complicated than what it is for. What I have in mind is like this:

if (fileProjection == null) {
  return TrackedFileStruct.READ_SCHEMA;
  // this could be prepared with the desired Tracking schema, omitting stats, including partition unconditionally. Preferable a static field in TrackedFileStruct.
}

// construct the projected schema including the mandatory fields similarly as now

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced this is how we'll be projecting stats from the read path. Ideally, we wouldn't project the whole stats field (and that's just the containing field id).

What we should have is the specific field ids for the individual status required to evaluate the filter.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree: we will be projecting only the field IDs we need. This is just a placeholder for now, as the interfaces for content stats are being revamped.

// content_stats are not projected yet
} else if (field.fieldId() == TrackedFile.PARTITION_ID && unpartitioned) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still read the partition even if unpartitioned. As we discussed lately, it will be null. I don't think this branch of the if is needed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The branch is intentional: when the table is fully unpartitioned there's no partition tuple, so we just don't project the field. Happy to switch to that if we want null per #17000, but the explicit omit is clearer here.

// unpartitioned manifests omit the partition field
} else {
fields.add(field);
}
}

return new Schema(fields);
}

static class Builder {
private final InputFile file;
private final Types.StructType partitionType;
private final Map<Integer, PartitionSpec> specsById;
private Expression rowFilter = Expressions.alwaysTrue();
private boolean caseSensitive = true;
private Schema fileProjection = null;
private ScanMetrics scanMetrics = ScanMetrics.noop();

private Builder(InputFile file, Schema tableSchema, Map<Integer, PartitionSpec> specsById) {
this.file = file;
this.partitionType = Partitioning.partitionType(tableSchema, specsById.values());
this.specsById = specsById;
}

/** Sets a row filter; files that cannot match the expression are skipped. */
Builder filterRows(Expression expr) {
Preconditions.checkArgument(expr != null, "Invalid row filter: null");
this.rowFilter = expr;
return this;
}

Builder caseSensitive(boolean isCaseSensitive) {
this.caseSensitive = isCaseSensitive;
return this;
}

Builder project(Schema newFileProjection) {
this.fileProjection = newFileProjection;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: precondition for != null?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional. Null projection means read all columns, and it's a supported case. This is the current semantics of the existing manifest reader as well.

return this;
}

Builder scanMetrics(ScanMetrics newScanMetrics) {
Preconditions.checkArgument(newScanMetrics != null, "Invalid scan metrics: null");
this.scanMetrics = newScanMetrics;
return this;
}

V4ManifestReader build() {
Map<Integer, Evaluator> partitionEvaluators = Maps.newHashMap();
Map<Integer, StructProjection> partitionProjections = Maps.newHashMap();
if (rowFilter != Expressions.alwaysTrue() && !partitionType.fields().isEmpty()) {
for (PartitionSpec spec : specsById.values()) {
Expression partFilter = Projections.inclusive(spec, caseSensitive).project(rowFilter);
partitionEvaluators.put(
spec.specId(), new Evaluator(spec.partitionType(), partFilter, caseSensitive));
partitionProjections.put(
spec.specId(), StructProjection.create(partitionType, spec.partitionType()));
}
}

return new V4ManifestReader(
file,
partitionType,
partitionEvaluators,
partitionProjections,
fileProjection,
scanMetrics);
}
}
}
Loading