From ffeddf283e51a4eb52400bcd0170a710eec63c1e Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Fri, 11 Sep 2026 09:33:50 -0700 Subject: [PATCH 01/21] Core: Add V4 file planner Plan FileScanTasks from a V4 root manifest, routing DATA entries to tasks and expanding DATA_MANIFEST entries into their leaf manifests through V4ManifestReader. Attach each data file's colocated deletion vector as a DeleteFile. Add RootManifestFile so the root reads through the same reader as the leaves, and TrackedFileAdapters.asDVDeleteFile(DataFile). --- .../java/org/apache/iceberg/FilePlanner.java | 254 ++++++++ .../org/apache/iceberg/RootManifestFile.java | 138 ++++ .../apache/iceberg/TrackedFileAdapters.java | 11 + .../org/apache/iceberg/TestFilePlanner.java | 600 ++++++++++++++++++ .../apache/iceberg/TestRootManifestFile.java | 108 ++++ .../iceberg/TestTrackedFileAdapters.java | 39 ++ 6 files changed, 1150 insertions(+) create mode 100644 core/src/main/java/org/apache/iceberg/FilePlanner.java create mode 100644 core/src/main/java/org/apache/iceberg/RootManifestFile.java create mode 100644 core/src/test/java/org/apache/iceberg/TestFilePlanner.java create mode 100644 core/src/test/java/org/apache/iceberg/TestRootManifestFile.java diff --git a/core/src/main/java/org/apache/iceberg/FilePlanner.java b/core/src/main/java/org/apache/iceberg/FilePlanner.java new file mode 100644 index 000000000000..fbee78772e5e --- /dev/null +++ b/core/src/main/java/org/apache/iceberg/FilePlanner.java @@ -0,0 +1,254 @@ +/* + * 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.io.IOException; +import java.io.UncheckedIOException; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ExecutorService; +import org.apache.iceberg.expressions.Expression; +import org.apache.iceberg.expressions.Expressions; +import org.apache.iceberg.expressions.ResidualEvaluator; +import org.apache.iceberg.io.CloseableIterable; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.metrics.ScanMetrics; +import org.apache.iceberg.metrics.ScanMetricsUtil; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.relocated.com.google.common.collect.Lists; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.util.ParallelIterable; + +/** + * Plans {@link FileScanTask}s from a V4 root manifest. + * + *

Emits a task for each live {@code DATA} entry and expands {@code DATA_MANIFEST} entries into + * their leaf manifests. A data file's colocated deletion vector is attached to its task as a {@link + * DeleteFile}. + */ +class FilePlanner { + private static final DeleteFile[] NO_DELETES = new DeleteFile[0]; + + private final FileIO io; + private final ManifestFile root; + private final Map specsById; + private final String tableLocation; + private final Expression dataFilter; + private final boolean ignoreResiduals; + private final boolean caseSensitive; + private final ScanMetrics scanMetrics; + private final ExecutorService executorService; + private final Map taskContextsBySpec = Maps.newConcurrentMap(); + + private FilePlanner( + FileIO io, + ManifestFile root, + Map specsById, + String tableLocation, + Expression dataFilter, + boolean ignoreResiduals, + boolean caseSensitive, + ScanMetrics scanMetrics, + ExecutorService executorService) { + this.io = io; + this.root = root; + this.specsById = specsById; + this.tableLocation = tableLocation; + this.dataFilter = dataFilter; + this.ignoreResiduals = ignoreResiduals; + this.caseSensitive = caseSensitive; + this.scanMetrics = scanMetrics; + this.executorService = executorService; + } + + static Builder builder( + FileIO io, ManifestFile root, Map specsById, String tableLocation) { + return new Builder(io, root, specsById, tableLocation); + } + + CloseableIterable planFiles() { + List rootDataFiles = Lists.newArrayList(); + List leafManifests = Lists.newArrayList(); + + try (CloseableIterable rootEntries = reader(root)) { + for (TrackedFile entry : rootEntries) { + switch (entry.contentType()) { + case DATA: + rootDataFiles.add(entry); + break; + case DATA_MANIFEST: + leafManifests.add(TrackedFileAdapters.asManifestFile(entry)); + break; + case DELETE_MANIFEST: + throw new UnsupportedOperationException("v3 and earlier deletes are not yet supported"); + default: + throw new UnsupportedOperationException( + "Unsupported file type in root manifest: " + entry.contentType()); + } + } + } catch (IOException e) { + throw new UncheckedIOException("Failed to close root manifest: " + root.path(), e); + } + + List> leafFiles = Lists.newArrayList(); + for (ManifestFile leaf : leafManifests) { + leafFiles.add(expandLeaf(leaf)); + } + + CloseableIterable expandedLeafFiles = + executorService != null && leafManifests.size() > 1 + ? new ParallelIterable<>(leafFiles, executorService) + : CloseableIterable.concat(leafFiles); + + CloseableIterable files = + CloseableIterable.concat( + ImmutableList.of(CloseableIterable.withNoopClose(rootDataFiles), expandedLeafFiles)); + + CloseableIterable dataFiles = + CloseableIterable.transform(files, file -> TrackedFileAdapters.asDataFile(file, specsById)); + + return CloseableIterable.transform(dataFiles, this::createTask); + } + + private CloseableIterable expandLeaf(ManifestFile leaf) { + return CloseableIterable.transform( + reader(leaf), + entry -> { + Preconditions.checkArgument( + entry.contentType() == FileContent.DATA, + "Unsupported file type in leaf manifest: %s", + entry.contentType()); + return entry; + }); + } + + private CloseableIterable reader(ManifestFile manifest) { + return V4ManifestReader.builder(manifest, io, specsById, tableLocation) + .forScanPlanning() + .filter(dataFilter) + .caseSensitive(caseSensitive) + .scanMetrics(scanMetrics) + .build(); + } + + private FileScanTask createTask(DataFile dataFile) { + TaskContext context = + taskContextsBySpec.computeIfAbsent(dataFile.specId(), this::newTaskContext); + + DeleteFile[] deletes = + dataFile.deletionVector() != null + ? new DeleteFile[] {TrackedFileAdapters.asDVDeleteFile(dataFile)} + : NO_DELETES; + + ScanMetricsUtil.fileTask(scanMetrics, dataFile, deletes); + + return new BaseFileScanTask( + dataFile, deletes, context.schemaAsString, context.specAsString, context.residuals); + } + + private TaskContext newTaskContext(int specId) { + PartitionSpec spec = specsById.get(specId); + Expression filter = ignoreResiduals ? Expressions.alwaysTrue() : dataFilter; + return new TaskContext( + SchemaParser.toJson(spec.schema()), + PartitionSpecParser.toJson(spec), + ResidualEvaluator.of(spec, filter, caseSensitive)); + } + + /** Per-spec task inputs computed once and shared across all files of a spec. */ + private static class TaskContext { + private final String schemaAsString; + private final String specAsString; + private final ResidualEvaluator residuals; + + private TaskContext(String schemaAsString, String specAsString, ResidualEvaluator residuals) { + this.schemaAsString = schemaAsString; + this.specAsString = specAsString; + this.residuals = residuals; + } + } + + static class Builder { + private final FileIO io; + private final ManifestFile root; + private final Map specsById; + private final String tableLocation; + private Expression dataFilter = Expressions.alwaysTrue(); + private boolean ignoreResiduals = false; + private boolean caseSensitive = true; + private ScanMetrics scanMetrics = ScanMetrics.noop(); + private ExecutorService executorService = null; + + private Builder( + FileIO io, ManifestFile root, Map specsById, String tableLocation) { + Preconditions.checkArgument(io != null, "Invalid file IO: null"); + Preconditions.checkArgument(root != null, "Invalid root manifest: null"); + Preconditions.checkArgument(specsById != null, "Invalid specs by ID: null"); + Preconditions.checkArgument(tableLocation != null, "Invalid table location: null"); + this.io = io; + this.root = root; + this.specsById = ImmutableMap.copyOf(specsById); + this.tableLocation = tableLocation; + } + + /** Sets the filter used for partition pruning and residual evaluation. */ + Builder filterData(Expression expr) { + Preconditions.checkArgument(expr != null, "Invalid filter: null"); + this.dataFilter = expr; + return this; + } + + Builder ignoreResiduals() { + this.ignoreResiduals = true; + return this; + } + + Builder caseSensitive(boolean newCaseSensitive) { + this.caseSensitive = newCaseSensitive; + return this; + } + + Builder scanMetrics(ScanMetrics newScanMetrics) { + Preconditions.checkArgument(newScanMetrics != null, "Invalid scan metrics: null"); + this.scanMetrics = newScanMetrics; + return this; + } + + Builder planWith(ExecutorService newExecutorService) { + Preconditions.checkArgument(newExecutorService != null, "Invalid executor service: null"); + this.executorService = newExecutorService; + return this; + } + + FilePlanner build() { + return new FilePlanner( + io, + root, + specsById, + tableLocation, + dataFilter, + ignoreResiduals, + caseSensitive, + scanMetrics, + executorService); + } + } +} diff --git a/core/src/main/java/org/apache/iceberg/RootManifestFile.java b/core/src/main/java/org/apache/iceberg/RootManifestFile.java new file mode 100644 index 000000000000..196cab48dfe9 --- /dev/null +++ b/core/src/main/java/org/apache/iceberg/RootManifestFile.java @@ -0,0 +1,138 @@ +/* + * 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.nio.ByteBuffer; +import java.util.List; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.util.ByteBuffers; + +/** + * A {@link ManifestFile} for the root manifest of a v4 adaptive metadata tree. + * + *

The root is referenced by location (like a manifest list), spans partition specs, and does not + * store per-manifest aggregates. Accessors for values it does not carry throw {@link + * UnsupportedOperationException}. + */ +class RootManifestFile implements ManifestFile { + private static final int FORMAT_VERSION = 4; + + private final InputFile file; + private final long snapshotId; + private final byte[] keyMetadata; + private Long length; + + RootManifestFile(InputFile file, long snapshotId, ByteBuffer keyMetadata) { + Preconditions.checkArgument(file != null, "Invalid file: null"); + this.file = file; + this.snapshotId = snapshotId; + this.keyMetadata = ByteBuffers.toByteArray(keyMetadata); + this.length = null; + } + + @Override + public String path() { + return file.location(); + } + + @Override + public long length() { + if (length == null) { + this.length = file.getLength(); + } + + return length; + } + + @Override + public int partitionSpecId() { + throw new UnsupportedOperationException("Root manifest has no partition spec"); + } + + @Override + public ManifestContent content() { + return ManifestContent.DATA; + } + + @Override + public long sequenceNumber() { + throw new UnsupportedOperationException("Root manifest has no sequence number"); + } + + @Override + public long minSequenceNumber() { + throw new UnsupportedOperationException("Root manifest has no minimum sequence number"); + } + + @Override + public Long snapshotId() { + return snapshotId; + } + + @Override + public Integer addedFilesCount() { + throw new UnsupportedOperationException("Root manifest has no added files count"); + } + + @Override + public Long addedRowsCount() { + throw new UnsupportedOperationException("Root manifest has no added rows count"); + } + + @Override + public Integer existingFilesCount() { + throw new UnsupportedOperationException("Root manifest has no existing files count"); + } + + @Override + public Long existingRowsCount() { + throw new UnsupportedOperationException("Root manifest has no existing rows count"); + } + + @Override + public Integer deletedFilesCount() { + throw new UnsupportedOperationException("Root manifest has no deleted files count"); + } + + @Override + public Long deletedRowsCount() { + throw new UnsupportedOperationException("Root manifest has no deleted rows count"); + } + + @Override + public List partitions() { + throw new UnsupportedOperationException("Root manifest has no partition summaries"); + } + + @Override + public ByteBuffer keyMetadata() { + return keyMetadata == null ? null : ByteBuffer.wrap(keyMetadata); + } + + @Override + public int formatVersion() { + return FORMAT_VERSION; + } + + @Override + public ManifestFile copy() { + return new RootManifestFile(file, snapshotId, keyMetadata()); + } +} diff --git a/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java b/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java index 5acb03b9089c..1657c499345a 100644 --- a/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java +++ b/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java @@ -46,6 +46,17 @@ static DeleteFile asDVDeleteFile(TrackedFile file, Map s return new TrackedDVDeleteFile(file, resolveSpecId(file, specsById)); } + static DeleteFile asDVDeleteFile(DataFile dataFile) { + // a DataFile produced by asDataFile is a wrapped TrackedFile; recover it rather than + // introducing a separate DataFile-backed deletion vector wrapper + Preconditions.checkArgument( + dataFile instanceof TrackedDataFile, + "Cannot create DV delete file from DataFile: %s", + dataFile.getClass().getName()); + TrackedDataFile tracked = (TrackedDataFile) dataFile; + return new TrackedDVDeleteFile(tracked.file(), tracked.specId()); + } + static DeleteFile asEqualityDeleteFile(TrackedFile file, Map specsById) { Preconditions.checkArgument( file.contentType() == FileContent.EQUALITY_DELETES, diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java new file mode 100644 index 000000000000..13099f080c25 --- /dev/null +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -0,0 +1,600 @@ +/* + * 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 static org.apache.iceberg.types.Types.NestedField.optional; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.tuple; + +import java.io.IOException; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.concurrent.ExecutorService; +import java.util.function.UnaryOperator; +import org.apache.iceberg.expressions.Expressions; +import org.apache.iceberg.inmemory.InMemoryFileIO; +import org.apache.iceberg.io.CloseableIterable; +import org.apache.iceberg.io.FileAppender; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.metrics.DefaultMetricsContext; +import org.apache.iceberg.metrics.ScanMetrics; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.relocated.com.google.common.collect.Lists; +import org.apache.iceberg.transforms.Transforms; +import org.apache.iceberg.types.Types; +import org.apache.iceberg.util.LocationUtil; +import org.apache.iceberg.util.ThreadPools; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.FieldSource; + +class TestFilePlanner { + private static final long SNAPSHOT_ID = 42L; + private static final int WRITER_FORMAT_VERSION = 4; + private static final long RECORD_COUNT = 100L; + private static final long FILE_SIZE_IN_BYTES = 1024L; + private static final String TABLE_LOCATION = "s3://bucket/db/table"; + private static final String DV_LOCATION = "s3://bucket/db/table/dv.puffin"; + private static final long DV_OFFSET = 100L; + private static final long DV_SIZE_IN_BYTES = 50L; + private static final long DV_CARDINALITY = 5L; + + private static final Schema TABLE_SCHEMA = + new Schema( + optional(1, "id", Types.IntegerType.get()), optional(2, "data", Types.StringType.get())); + private static final PartitionSpec SPEC = + PartitionSpec.builderFor(TABLE_SCHEMA).identity("id").build(); + private static final Types.StructType PARTITION_TYPE = SPEC.partitionType(); + private static final Types.StructType EMPTY_PARTITION = Types.StructType.of(); + private static final PartitionData EMPTY_PARTITION_DATA = new PartitionData(EMPTY_PARTITION); + private static final Map PARTITIONED_SPECS = + ImmutableMap.of(SPEC.specId(), SPEC); + private static final Map UNPARTITIONED_SPECS = + ImmutableMap.of(PartitionSpec.unpartitioned().specId(), PartitionSpec.unpartitioned()); + + private static final List MANIFEST_FORMATS = + ImmutableList.of(FileFormat.AVRO, FileFormat.PARQUET); + + private final InMemoryFileIO fileIO = new InMemoryFileIO(); + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void rootWithDirectDataEntries(FileFormat format) throws IOException { + InputFile root = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFile("a.parquet", EMPTY_PARTITION_DATA), + dataFile("b.parquet", EMPTY_PARTITION_DATA))); + + List tasks = plan(root, UNPARTITIONED_SPECS); + + assertThat(tasks) + .hasSize(2) + .extracting(task -> task.file().location()) + .containsExactlyInAnyOrder(resolved("a.parquet"), resolved("b.parquet")); + assertThat(tasks).allSatisfy(task -> assertThat(task.deletes()).isEmpty()); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void rootWithLeafManifestEntriesOnly(FileFormat format) throws IOException { + InputFile leaf = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFile("leaf-a.parquet", EMPTY_PARTITION_DATA), + dataFile("leaf-b.parquet", EMPTY_PARTITION_DATA))); + InputFile root = + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); + + ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); + List tasks = + plan(root, UNPARTITIONED_SPECS, planner -> planner.scanMetrics(metrics)); + + assertThat(tasks) + .hasSize(2) + .extracting(task -> task.file().location()) + .containsExactlyInAnyOrder(resolved("leaf-a.parquet"), resolved("leaf-b.parquet")); + assertThat(metrics.scannedDataManifests().value()) + .as("the root and the expanded leaf are each counted as a scanned manifest") + .isEqualTo(2L); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void mixedRootDataAndLeafManifest(FileFormat format) throws IOException { + InputFile leaf = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of(dataFile("leaf-data-file.parquet", EMPTY_PARTITION_DATA))); + InputFile root = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFile("root-data-file.parquet", EMPTY_PARTITION_DATA), + dataManifest(leaf.location()))); + + List tasks = plan(root, UNPARTITIONED_SPECS); + + assertThat(tasks) + .hasSize(2) + .extracting(task -> task.file().location()) + .containsExactlyInAnyOrder( + resolved("root-data-file.parquet"), resolved("leaf-data-file.parquet")); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void deletionVectorAttachedToTask(FileFormat format) throws IOException { + TrackedFile fileWithDv = + dataFile( + "with-dv.parquet", + EMPTY_PARTITION_DATA, + deletionVector(DV_LOCATION, DV_OFFSET, DV_SIZE_IN_BYTES, DV_CARDINALITY)); + InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(fileWithDv)); + + List tasks = plan(root, UNPARTITIONED_SPECS); + + assertThat(tasks).hasSize(1); + assertThat(tasks.get(0).deletes()) + .hasSize(1) + .allSatisfy( + delete -> { + assertThat(delete.content()).isEqualTo(FileContent.POSITION_DELETES); + assertThat(delete.referencedDataFile()).isEqualTo(resolved("with-dv.parquet")); + assertThat(delete.recordCount()).isEqualTo(DV_CARDINALITY); + assertThat(delete.contentOffset()).isEqualTo(DV_OFFSET); + assertThat(delete.contentSizeInBytes()).isEqualTo(DV_SIZE_IN_BYTES); + }); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void partitionFilterPrunesFiles(FileFormat format) throws IOException { + InputFile root = + writeManifest( + format, + PARTITION_TYPE, + ImmutableList.of( + dataFile("keep.parquet", partition(1)), dataFile("prune.parquet", partition(2)))); + + List tasks = + plan(root, PARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("id", 1))); + + assertThat(tasks) + .hasSize(1) + .extracting(task -> task.file().location()) + .containsExactly(resolved("keep.parquet")); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void residualAttachedFromFilter(FileFormat format) throws IOException { + InputFile root = + writeManifest( + format, PARTITION_TYPE, ImmutableList.of(dataFile("keep.parquet", partition(1)))); + + List withResidual = + plan( + root, PARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("data", "x"))); + // the identity partition on id leaves the data predicate as a residual + assertThat(withResidual.get(0).residual()) + .hasToString(Expressions.equal("data", "x").toString()); + + List ignored = + plan( + root, + PARTITIONED_SPECS, + planner -> planner.filterData(Expressions.equal("data", "x")).ignoreResiduals()); + assertThat(ignored.get(0).residual()).isEqualTo(Expressions.alwaysTrue()); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void sameSpecFilesShareContext(FileFormat format) throws IOException { + InputFile root = + writeManifest( + format, + PARTITION_TYPE, + ImmutableList.of( + dataFile("a.parquet", partition(1)), dataFile("b.parquet", partition(1)))); + + List tasks = + plan( + root, PARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("data", "x"))); + + // both files share spec 0, so both tasks resolve from the same shared context + assertThat(tasks).hasSize(2); + FileScanTask first = tasks.get(0); + FileScanTask second = tasks.get(1); + assertThat(second.schema()).isSameAs(first.schema()); + assertThat(second.spec()).isSameAs(first.spec()); + assertThat(second.residual()).isSameAs(first.residual()); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void buildingAndClosingWithoutIteratingDoesNotScanLeaves(FileFormat format) throws IOException { + InputFile leaf = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of(dataFile("leaf.parquet", EMPTY_PARTITION_DATA))); + InputFile root = + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); + + ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); + FilePlanner planner = + FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS, TABLE_LOCATION) + .scanMetrics(metrics) + .build(); + // the root is read eagerly to route its entries; leaf readers open lazily, so closing the plan + // without iterating scans only the root, not the leaf + planner.planFiles().close(); + + assertThat(metrics.scannedDataManifests().value()).isEqualTo(1L); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void parallelPlanningMatchesSequential(FileFormat format) throws IOException { + // a DV and a residual-bearing filter in the tree so parity actually exercises createTask's DV + // branch and per-spec residual keying on worker threads, not just locations + TrackedFile withDv = + dataFile( + "leaf1.parquet", + partition(1), + deletionVector(DV_LOCATION, DV_OFFSET, DV_SIZE_IN_BYTES, DV_CARDINALITY)); + InputFile leaf1 = writeManifest(format, PARTITION_TYPE, ImmutableList.of(withDv)); + InputFile leaf2 = + writeManifest( + format, PARTITION_TYPE, ImmutableList.of(dataFile("leaf2.parquet", partition(1)))); + InputFile root = + writeManifest( + format, + PARTITION_TYPE, + ImmutableList.of(dataManifest(leaf1.location()), dataManifest(leaf2.location()))); + + List sequential = + plan( + root, PARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("data", "x"))); + + ExecutorService pool = ThreadPools.newFixedThreadPool("test-scan-task-planner", 2); + try { + List parallel = + plan( + root, + PARTITIONED_SPECS, + planner -> planner.filterData(Expressions.equal("data", "x")).planWith(pool)); + assertThat(parallel) + .extracting( + task -> task.file().location(), + task -> task.residual().toString(), + task -> task.deletes().size()) + .containsExactlyInAnyOrderElementsOf( + Lists.transform( + sequential, + task -> + tuple( + task.file().location(), + task.residual().toString(), + task.deletes().size()))); + } finally { + pool.shutdownNow(); + } + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void deleteContentInRootIsUnsupported(FileFormat format) throws IOException { + InputFile root = + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(deleteManifest("deletes.avro"))); + + // delete content is only produced by upgraded trees; that path is not yet implemented + FilePlanner planner = + FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS, TABLE_LOCATION).build(); + assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("v3 and earlier deletes are not yet supported"); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void deleteContentInLeafIsUnsupported(FileFormat format) throws IOException { + InputFile leaf = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFile("leaf-data.parquet", EMPTY_PARTITION_DATA), + deleteManifest("leaf-deletes.avro"))); + InputFile root = + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); + + FilePlanner planner = + FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS, TABLE_LOCATION).build(); + assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Unsupported file type in leaf manifest: DELETE_MANIFEST"); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void nestedDataManifestInLeafIsRejected(FileFormat format) throws IOException { + InputFile leaf = + writeManifest( + format, EMPTY_PARTITION, ImmutableList.of(dataManifest("nested-leaf.parquet"))); + InputFile root = + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); + + // a leaf holds only data entries in a two-level tree; a nested manifest is rejected + FilePlanner planner = + FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS, TABLE_LOCATION).build(); + assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Unsupported file type in leaf manifest: DATA_MANIFEST"); + } + + @Test + void emptyRootYieldsNoTasks() throws IOException { + // an empty root has no plannable entries regardless of manifest format; AVRO is used because + // the Parquet writer does not materialize a file when no records are appended + InputFile root = writeManifest(FileFormat.AVRO, EMPTY_PARTITION, ImmutableList.of()); + + assertThat(plan(root, UNPARTITIONED_SPECS)).isEmpty(); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void planFilesAcrossMultipleSpecs(FileFormat format) throws IOException { + PartitionSpec spec0 = + PartitionSpec.builderFor(TABLE_SCHEMA) + .withSpecId(0) + .add(1, 1000, "id", Transforms.identity()) + .build(); + PartitionSpec spec1 = + PartitionSpec.builderFor(TABLE_SCHEMA) + .withSpecId(1) + .add(2, 1001, "data", Transforms.identity()) + .build(); + Map specsById = + ImmutableMap.of(spec0.specId(), spec0, spec1.specId(), spec1); + Types.StructType unionType = Partitioning.unionPartitionTypes(specsById.values()); + + TrackedFile spec0Keep = + dataFile("spec0-keep.parquet", spec0.specId(), unionPartition(unionType, 1, null), null); + TrackedFile spec0Prune = + dataFile("spec0-prune.parquet", spec0.specId(), unionPartition(unionType, 2, null), null); + TrackedFile spec1File = + dataFile("spec1.parquet", spec1.specId(), unionPartition(unionType, null, "x"), null); + InputFile root = + writeManifest(format, unionType, ImmutableList.of(spec0Keep, spec0Prune, spec1File)); + + // id = 1 prunes the spec0 file partitioned on id=2; spec1 is not partitioned by id, so its + // residual keeps the predicate. A per-spec residual mis-keying would surface here. + List tasks = + plan(root, specsById, planner -> planner.filterData(Expressions.equal("id", 1))); + + assertThat(tasks) + .extracting( + task -> task.file().location(), + task -> task.spec().specId(), + task -> task.residual().toString()) + .containsExactlyInAnyOrder( + tuple(resolved("spec0-keep.parquet"), 0, Expressions.alwaysTrue().toString()), + tuple(resolved("spec1.parquet"), 1, Expressions.equal("id", 1).toString())); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void planFilesReportsScanMetrics(FileFormat format) throws IOException { + TrackedFile fileWithDv = + dataFile( + "with-dv.parquet", + EMPTY_PARTITION_DATA, + deletionVector(DV_LOCATION, DV_OFFSET, DV_SIZE_IN_BYTES, DV_CARDINALITY)); + InputFile root = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of(dataFile("plain.parquet", EMPTY_PARTITION_DATA), fileWithDv)); + + ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); + List tasks = + plan(root, UNPARTITIONED_SPECS, planner -> planner.scanMetrics(metrics)); + + assertThat(tasks).hasSize(2); + assertThat(metrics.scannedDataManifests().value()) + .as("the root is scanned; there are no leaves") + .isEqualTo(1L); + assertThat(metrics.resultDataFiles().value()).isEqualTo(2L); + assertThat(metrics.totalFileSizeInBytes().value()).isEqualTo(2 * FILE_SIZE_IN_BYTES); + assertThat(metrics.resultDeleteFiles().value()) + .as("only the file with a colocated DV contributes a delete file") + .isEqualTo(1L); + assertThat(metrics.totalDeleteFileSizeInBytes().value()) + .as("the DV delete contributes its size") + .isEqualTo(DV_SIZE_IN_BYTES); + } + + private List plan(InputFile root, Map specsById) + throws IOException { + return plan(root, specsById, UnaryOperator.identity()); + } + + private List plan( + InputFile root, + Map specsById, + UnaryOperator configure) + throws IOException { + FilePlanner planner = + configure + .apply(FilePlanner.builder(fileIO, asManifest(root), specsById, TABLE_LOCATION)) + .build(); + try (CloseableIterable tasks = planner.planFiles()) { + return Lists.newArrayList(tasks); + } + } + + // the root manifest is supplied to the planner by its caller; the reader only needs its location + // and format version, so a lightweight stand-in stands in for a table-metadata-sourced root + private static ManifestFile asManifest(InputFile file) { + return new RootManifestFile(file, SNAPSHOT_ID, /* keyMetadata= */ null); + } + + private static TrackedFile dataFile(String location, PartitionData partition) { + return dataFile(location, specId(partition), partition, null); + } + + private static TrackedFile dataFile(String location, PartitionData partition, DeletionVector dv) { + return dataFile(location, specId(partition), partition, dv); + } + + private static TrackedFile dataFile( + String location, Integer specId, PartitionData partition, DeletionVector dv) { + return trackedFile(addedTracking(), FileContent.DATA, location, specId, partition, dv, null); + } + + private static TrackedFile dataManifest(String location) { + return trackedFile( + addedTracking(), FileContent.DATA_MANIFEST, location, null, null, null, dataManifestInfo()); + } + + private static ManifestInfo dataManifestInfo() { + return ManifestInfoStruct.builder() + .addedFilesCount(1) + .existingFilesCount(0) + .deletedFilesCount(0) + .replacedFilesCount(0) + .addedRowsCount(RECORD_COUNT) + .existingRowsCount(0) + .deletedRowsCount(0) + .replacedRowsCount(0) + .minSequenceNumber(0L) + .build(); + } + + private static TrackedFile deleteManifest(String location) { + return trackedFile( + addedTracking(), FileContent.DELETE_MANIFEST, location, null, null, null, null); + } + + private static TrackedFile trackedFile( + TrackingStruct tracking, + FileContent contentType, + String location, + Integer specId, + PartitionData partition, + DeletionVector dv, + ManifestInfo manifestInfo) { + return new TrackedFileStruct( + tracking, + contentType, + WRITER_FORMAT_VERSION, + location, + FileFormat.fromFileName(location), + RECORD_COUNT, + FILE_SIZE_IN_BYTES, + specId, + partition, + null, // contentStats + null, // sortOrderId + dv, // deletionVector + manifestInfo, + null, // keyMetadata + null, // splitOffsets + null); // equalityIds + } + + private static TrackingStruct addedTracking() { + return new TrackingStruct( + EntryStatus.ADDED, + SNAPSHOT_ID, + null, // dataSequenceNumber + null, // fileSequenceNumber + null, // dvSnapshotId + null, // firstRowId + null, // deletedPositions + null); // replacedPositions + } + + private static Integer specId(PartitionData partition) { + boolean unpartitioned = partition.size() == 0; + return unpartitioned ? PartitionSpec.unpartitioned().specId() : SPEC.specId(); + } + + private static PartitionData partition(int id) { + PartitionData partition = new PartitionData(PARTITION_TYPE); + partition.set(0, id); + return partition; + } + + // the location a relative fixture resolves to once read against TABLE_LOCATION + private static String resolved(String location) { + return LocationUtil.resolveLocation(TABLE_LOCATION, location); + } + + private static PartitionData unionPartition(Types.StructType unionType, Integer id, String data) { + PartitionData partition = new PartitionData(unionType); + partition.set(0, id); + partition.set(1, data); + return partition; + } + + private static DeletionVector deletionVector( + String location, long offset, long sizeInBytes, long cardinality) { + return DeletionVectorStruct.builder() + .location(location) + .offset(offset) + .sizeInBytes(sizeInBytes) + .cardinality(cardinality) + .build(); + } + + private InputFile writeManifest( + FileFormat format, Types.StructType partitionType, Iterable files) + throws IOException { + Schema writeSchema = TrackedFile.schema(partitionType, Types.StructType.of()); + // write under the table location so a leaf's resolved reference round-trips to the file on disk + OutputFile out = + fileIO.newOutputFile( + TABLE_LOCATION + + "/metadata/manifest-" + + System.nanoTime() + + "." + + format.name().toLowerCase(Locale.ROOT)); + try (FileAppender appender = + InternalData.write(format, out).schema(writeSchema).named("tracked_file").build()) { + for (TrackedFile file : files) { + appender.add((StructLike) file); + } + } + + return out.toInputFile(); + } +} diff --git a/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java b/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java new file mode 100644 index 000000000000..51dfc781068e --- /dev/null +++ b/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java @@ -0,0 +1,108 @@ +/* + * 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 static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.nio.ByteBuffer; +import org.apache.iceberg.inmemory.InMemoryInputFile; +import org.apache.iceberg.io.InputFile; +import org.junit.jupiter.api.Test; + +class TestRootManifestFile { + private static final long SNAPSHOT_ID = 42L; + private static final byte[] CONTENTS = new byte[] {1, 2, 3, 4, 5}; + private static final String LOCATION = "s3://bucket/db/table/metadata/root.avro"; + + private final InputFile file = new InMemoryInputFile(LOCATION, CONTENTS); + + @Test + void exposesLocationLengthAndSnapshotId() { + RootManifestFile root = new RootManifestFile(file, SNAPSHOT_ID, /* keyMetadata= */ null); + + assertThat(root.path()).isEqualTo(LOCATION); + assertThat(root.length()).isEqualTo(CONTENTS.length); + assertThat(root.snapshotId()).isEqualTo(SNAPSHOT_ID); + } + + @Test + void readableAsAV4DataManifest() { + RootManifestFile root = new RootManifestFile(file, SNAPSHOT_ID, null); + + assertThat(root.content()).isEqualTo(ManifestContent.DATA); + assertThat(root.formatVersion()).isEqualTo(4); + } + + @Test + void keyMetadataRoundTrips() { + assertThat(new RootManifestFile(file, SNAPSHOT_ID, null).keyMetadata()).isNull(); + + ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {9, 8, 7}); + assertThat(new RootManifestFile(file, SNAPSHOT_ID, keyMetadata).keyMetadata()) + .isEqualTo(keyMetadata); + } + + @Test + void copyPreservesValues() { + ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {9, 8, 7}); + ManifestFile copy = new RootManifestFile(file, SNAPSHOT_ID, keyMetadata).copy(); + + assertThat(copy.path()).isEqualTo(LOCATION); + assertThat(copy.length()).isEqualTo(CONTENTS.length); + assertThat(copy.snapshotId()).isEqualTo(SNAPSHOT_ID); + assertThat(copy.keyMetadata()).isEqualTo(keyMetadata); + } + + @Test + void unsupportedAccessorsThrow() { + RootManifestFile root = new RootManifestFile(file, SNAPSHOT_ID, null); + + assertThatThrownBy(root::partitionSpecId) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("Root manifest has no partition spec"); + assertThatThrownBy(root::sequenceNumber) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("Root manifest has no sequence number"); + assertThatThrownBy(root::minSequenceNumber) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("Root manifest has no minimum sequence number"); + assertThatThrownBy(root::addedFilesCount) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("Root manifest has no added files count"); + assertThatThrownBy(root::addedRowsCount) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("Root manifest has no added rows count"); + assertThatThrownBy(root::existingFilesCount) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("Root manifest has no existing files count"); + assertThatThrownBy(root::existingRowsCount) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("Root manifest has no existing rows count"); + assertThatThrownBy(root::deletedFilesCount) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("Root manifest has no deleted files count"); + assertThatThrownBy(root::deletedRowsCount) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("Root manifest has no deleted rows count"); + assertThatThrownBy(root::partitions) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("Root manifest has no partition summaries"); + } +} diff --git a/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java b/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java index d3af965ec1dc..8e53acdbca16 100644 --- a/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java +++ b/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java @@ -481,6 +481,45 @@ void dvDeleteFileAdapterRejectsNullDeletionVector() { .hasMessage("Cannot create DV delete file: no deletion vector"); } + @Test + void dvDeleteFileFromDataFileUnwrapsTrackedFile() { + DeletionVector dv = deletionVector(); + TrackedFile file = + new TrackedFileStruct( + null, // tracking + FileContent.DATA, + FORMAT_VERSION_V4, + DATA_FILE_LOCATION, + FileFormat.PARQUET, + 100L, // recordCount + 1024L, // fileSizeInBytes + PARTITIONED_SPEC_ID, + PARTITION, + null, // contentStats + null, // sortOrderId + dv, // deletionVector + null, // manifestInfo + null, // keyMetadata + null, // splitOffsets + null); // equalityIds + + DataFile dataFile = TrackedFileAdapters.asDataFile(file, specsById(PARTITIONED_SPEC)); + DeleteFile dvFile = TrackedFileAdapters.asDVDeleteFile(dataFile); + + assertThat(dvFile.content()).isEqualTo(FileContent.POSITION_DELETES); + assertThat(dvFile.referencedDataFile()).isEqualTo(DATA_FILE_LOCATION); + assertThat(dvFile.recordCount()).isEqualTo(dv.cardinality()); + assertThat(dvFile.contentOffset()).isEqualTo(dv.offset()); + assertThat(dvFile.contentSizeInBytes()).isEqualTo(dv.sizeInBytes()); + } + + @Test + void dvDeleteFileFromDataFileRejectsNonTrackedDataFile() { + assertThatThrownBy(() -> TrackedFileAdapters.asDVDeleteFile(mock(DataFile.class))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Cannot create DV delete file from DataFile"); + } + @ParameterizedTest @EnumSource( value = FileContent.class, From 90b8a03292b3d68abc54e1186ce189123f49647d Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Fri, 11 Sep 2026 14:16:14 -0700 Subject: [PATCH 02/21] Core: Make FilePlanner tableLocation a builder setter Move tableLocation from a required factory argument to a tableLocation() configure-time setter, so a caller supplies it only when relative paths need resolving. --- .../java/org/apache/iceberg/FilePlanner.java | 18 ++++++++++-------- .../org/apache/iceberg/TestFilePlanner.java | 10 +++++----- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/FilePlanner.java b/core/src/main/java/org/apache/iceberg/FilePlanner.java index fbee78772e5e..26dac21b7c5a 100644 --- a/core/src/main/java/org/apache/iceberg/FilePlanner.java +++ b/core/src/main/java/org/apache/iceberg/FilePlanner.java @@ -79,9 +79,8 @@ private FilePlanner( this.executorService = executorService; } - static Builder builder( - FileIO io, ManifestFile root, Map specsById, String tableLocation) { - return new Builder(io, root, specsById, tableLocation); + static Builder builder(FileIO io, ManifestFile root, Map specsById) { + return new Builder(io, root, specsById); } CloseableIterable planFiles() { @@ -190,23 +189,26 @@ static class Builder { private final FileIO io; private final ManifestFile root; private final Map specsById; - private final String tableLocation; + private String tableLocation = null; private Expression dataFilter = Expressions.alwaysTrue(); private boolean ignoreResiduals = false; private boolean caseSensitive = true; private ScanMetrics scanMetrics = ScanMetrics.noop(); private ExecutorService executorService = null; - private Builder( - FileIO io, ManifestFile root, Map specsById, String tableLocation) { + private Builder(FileIO io, ManifestFile root, Map specsById) { Preconditions.checkArgument(io != null, "Invalid file IO: null"); Preconditions.checkArgument(root != null, "Invalid root manifest: null"); Preconditions.checkArgument(specsById != null, "Invalid specs by ID: null"); - Preconditions.checkArgument(tableLocation != null, "Invalid table location: null"); this.io = io; this.root = root; this.specsById = ImmutableMap.copyOf(specsById); - this.tableLocation = tableLocation; + } + + Builder tableLocation(String newTableLocation) { + Preconditions.checkArgument(newTableLocation != null, "Invalid table location: null"); + this.tableLocation = newTableLocation; + return this; } /** Sets the filter used for partition pruning and residual evaluation. */ diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index 13099f080c25..3b5dd80efeed 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -250,7 +250,7 @@ void buildingAndClosingWithoutIteratingDoesNotScanLeaves(FileFormat format) thro ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS, TABLE_LOCATION) + FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS).tableLocation(TABLE_LOCATION) .scanMetrics(metrics) .build(); // the root is read eagerly to route its entries; leaf readers open lazily, so closing the plan @@ -317,7 +317,7 @@ void deleteContentInRootIsUnsupported(FileFormat format) throws IOException { // delete content is only produced by upgraded trees; that path is not yet implemented FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS, TABLE_LOCATION).build(); + FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS).tableLocation(TABLE_LOCATION).build(); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) .isInstanceOf(UnsupportedOperationException.class) .hasMessage("v3 and earlier deletes are not yet supported"); @@ -337,7 +337,7 @@ void deleteContentInLeafIsUnsupported(FileFormat format) throws IOException { writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS, TABLE_LOCATION).build(); + FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS).tableLocation(TABLE_LOCATION).build(); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Unsupported file type in leaf manifest: DELETE_MANIFEST"); @@ -354,7 +354,7 @@ void nestedDataManifestInLeafIsRejected(FileFormat format) throws IOException { // a leaf holds only data entries in a two-level tree; a nested manifest is rejected FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS, TABLE_LOCATION).build(); + FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS).tableLocation(TABLE_LOCATION).build(); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Unsupported file type in leaf manifest: DATA_MANIFEST"); @@ -454,7 +454,7 @@ private List plan( throws IOException { FilePlanner planner = configure - .apply(FilePlanner.builder(fileIO, asManifest(root), specsById, TABLE_LOCATION)) + .apply(FilePlanner.builder(fileIO, asManifest(root), specsById).tableLocation(TABLE_LOCATION)) .build(); try (CloseableIterable tasks = planner.planFiles()) { return Lists.newArrayList(tasks); From c89b7c865d71d98e873cf159d9f07d9afa4f1494 Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Fri, 11 Sep 2026 14:33:32 -0700 Subject: [PATCH 03/21] Core: Count colocated DVs, fix root copy, rename leafPlanTask - Count each colocated deletion vector in scan metrics via ScanMetricsUtil.indexedDeleteFile; dvs()/indexedDeleteFiles() were left at zero while the DV was attached to the task. - RootManifestFile.copy() clones the key-metadata backing array instead of sharing it, honoring the ManifestFile.copy() defensive-copy contract. - Rename expandLeaf to leafPlanTask (and the list to leafPlanTasks) to match the server-side planning terminology. --- .../java/org/apache/iceberg/FilePlanner.java | 24 ++++++++++--------- .../org/apache/iceberg/RootManifestFile.java | 3 ++- .../apache/iceberg/TestRootManifestFile.java | 6 ++++- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/FilePlanner.java b/core/src/main/java/org/apache/iceberg/FilePlanner.java index 26dac21b7c5a..d90f7f8b8089 100644 --- a/core/src/main/java/org/apache/iceberg/FilePlanner.java +++ b/core/src/main/java/org/apache/iceberg/FilePlanner.java @@ -107,19 +107,19 @@ CloseableIterable planFiles() { throw new UncheckedIOException("Failed to close root manifest: " + root.path(), e); } - List> leafFiles = Lists.newArrayList(); + List> leafPlanTasks = Lists.newArrayList(); for (ManifestFile leaf : leafManifests) { - leafFiles.add(expandLeaf(leaf)); + leafPlanTasks.add(leafPlanTask(leaf)); } - CloseableIterable expandedLeafFiles = + CloseableIterable leafFiles = executorService != null && leafManifests.size() > 1 - ? new ParallelIterable<>(leafFiles, executorService) - : CloseableIterable.concat(leafFiles); + ? new ParallelIterable<>(leafPlanTasks, executorService) + : CloseableIterable.concat(leafPlanTasks); CloseableIterable files = CloseableIterable.concat( - ImmutableList.of(CloseableIterable.withNoopClose(rootDataFiles), expandedLeafFiles)); + ImmutableList.of(CloseableIterable.withNoopClose(rootDataFiles), leafFiles)); CloseableIterable dataFiles = CloseableIterable.transform(files, file -> TrackedFileAdapters.asDataFile(file, specsById)); @@ -127,7 +127,7 @@ CloseableIterable planFiles() { return CloseableIterable.transform(dataFiles, this::createTask); } - private CloseableIterable expandLeaf(ManifestFile leaf) { + private CloseableIterable leafPlanTask(ManifestFile leaf) { return CloseableIterable.transform( reader(leaf), entry -> { @@ -152,10 +152,12 @@ private FileScanTask createTask(DataFile dataFile) { TaskContext context = taskContextsBySpec.computeIfAbsent(dataFile.specId(), this::newTaskContext); - DeleteFile[] deletes = - dataFile.deletionVector() != null - ? new DeleteFile[] {TrackedFileAdapters.asDVDeleteFile(dataFile)} - : NO_DELETES; + DeleteFile[] deletes = NO_DELETES; + if (dataFile.deletionVector() != null) { + DeleteFile dv = TrackedFileAdapters.asDVDeleteFile(dataFile); + ScanMetricsUtil.indexedDeleteFile(scanMetrics, dv); + deletes = new DeleteFile[] {dv}; + } ScanMetricsUtil.fileTask(scanMetrics, dataFile, deletes); diff --git a/core/src/main/java/org/apache/iceberg/RootManifestFile.java b/core/src/main/java/org/apache/iceberg/RootManifestFile.java index 196cab48dfe9..1d268c4ba8b8 100644 --- a/core/src/main/java/org/apache/iceberg/RootManifestFile.java +++ b/core/src/main/java/org/apache/iceberg/RootManifestFile.java @@ -133,6 +133,7 @@ public int formatVersion() { @Override public ManifestFile copy() { - return new RootManifestFile(file, snapshotId, keyMetadata()); + ByteBuffer keyMetadataCopy = keyMetadata == null ? null : ByteBuffer.wrap(keyMetadata.clone()); + return new RootManifestFile(file, snapshotId, keyMetadataCopy); } } diff --git a/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java b/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java index 51dfc781068e..8d02c939afef 100644 --- a/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java +++ b/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java @@ -62,12 +62,16 @@ void keyMetadataRoundTrips() { @Test void copyPreservesValues() { ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {9, 8, 7}); - ManifestFile copy = new RootManifestFile(file, SNAPSHOT_ID, keyMetadata).copy(); + RootManifestFile original = new RootManifestFile(file, SNAPSHOT_ID, keyMetadata); + ManifestFile copy = original.copy(); assertThat(copy.path()).isEqualTo(LOCATION); assertThat(copy.length()).isEqualTo(CONTENTS.length); assertThat(copy.snapshotId()).isEqualTo(SNAPSHOT_ID); assertThat(copy.keyMetadata()).isEqualTo(keyMetadata); + assertThat(copy.keyMetadata().array()) + .as("copy should not share the key metadata backing array") + .isNotSameAs(original.keyMetadata().array()); } @Test From ae194b671e403efdbbc4900b55be83eb9fb930c3 Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Fri, 11 Sep 2026 14:47:54 -0700 Subject: [PATCH 04/21] Core: Drop redundant leaf content-type check in FilePlanner A leaf entry's content type is already validated by asDataFile downstream, so the leaf check duplicated it. Read leaves directly through reader(leaf) and remove the leafPlanTask wrapper; the reader is already a lazy CloseableIterable. --- .../main/java/org/apache/iceberg/FilePlanner.java | 14 +------------- .../java/org/apache/iceberg/TestFilePlanner.java | 4 ++-- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/FilePlanner.java b/core/src/main/java/org/apache/iceberg/FilePlanner.java index d90f7f8b8089..fde5f3a76847 100644 --- a/core/src/main/java/org/apache/iceberg/FilePlanner.java +++ b/core/src/main/java/org/apache/iceberg/FilePlanner.java @@ -109,7 +109,7 @@ CloseableIterable planFiles() { List> leafPlanTasks = Lists.newArrayList(); for (ManifestFile leaf : leafManifests) { - leafPlanTasks.add(leafPlanTask(leaf)); + leafPlanTasks.add(reader(leaf)); } CloseableIterable leafFiles = @@ -127,18 +127,6 @@ CloseableIterable planFiles() { return CloseableIterable.transform(dataFiles, this::createTask); } - private CloseableIterable leafPlanTask(ManifestFile leaf) { - return CloseableIterable.transform( - reader(leaf), - entry -> { - Preconditions.checkArgument( - entry.contentType() == FileContent.DATA, - "Unsupported file type in leaf manifest: %s", - entry.contentType()); - return entry; - }); - } - private CloseableIterable reader(ManifestFile manifest) { return V4ManifestReader.builder(manifest, io, specsById, tableLocation) .forScanPlanning() diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index 3b5dd80efeed..e81ce4456848 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -340,7 +340,7 @@ void deleteContentInLeafIsUnsupported(FileFormat format) throws IOException { FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS).tableLocation(TABLE_LOCATION).build(); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Unsupported file type in leaf manifest: DELETE_MANIFEST"); + .hasMessage("Invalid content type for DataFile: DELETE_MANIFEST"); } @ParameterizedTest @@ -357,7 +357,7 @@ void nestedDataManifestInLeafIsRejected(FileFormat format) throws IOException { FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS).tableLocation(TABLE_LOCATION).build(); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Unsupported file type in leaf manifest: DATA_MANIFEST"); + .hasMessage("Invalid content type for DataFile: DATA_MANIFEST"); } @Test From 97a83adc1702cae047a2db223118ddbea15d2ee9 Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Fri, 11 Sep 2026 16:36:47 -0700 Subject: [PATCH 05/21] cleanups --- .../main/java/org/apache/iceberg/RootManifestFile.java | 8 +------- .../main/java/org/apache/iceberg/TrackedFileAdapters.java | 2 -- .../java/org/apache/iceberg/TestRootManifestFile.java | 2 +- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/RootManifestFile.java b/core/src/main/java/org/apache/iceberg/RootManifestFile.java index 1d268c4ba8b8..87866cc250be 100644 --- a/core/src/main/java/org/apache/iceberg/RootManifestFile.java +++ b/core/src/main/java/org/apache/iceberg/RootManifestFile.java @@ -24,13 +24,7 @@ import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.util.ByteBuffers; -/** - * A {@link ManifestFile} for the root manifest of a v4 adaptive metadata tree. - * - *

The root is referenced by location (like a manifest list), spans partition specs, and does not - * store per-manifest aggregates. Accessors for values it does not carry throw {@link - * UnsupportedOperationException}. - */ +/** A {@link ManifestFile} for the root manifest of an adaptive metadata tree. */ class RootManifestFile implements ManifestFile { private static final int FORMAT_VERSION = 4; diff --git a/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java b/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java index 1657c499345a..455690dd809e 100644 --- a/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java +++ b/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java @@ -47,8 +47,6 @@ static DeleteFile asDVDeleteFile(TrackedFile file, Map s } static DeleteFile asDVDeleteFile(DataFile dataFile) { - // a DataFile produced by asDataFile is a wrapped TrackedFile; recover it rather than - // introducing a separate DataFile-backed deletion vector wrapper Preconditions.checkArgument( dataFile instanceof TrackedDataFile, "Cannot create DV delete file from DataFile: %s", diff --git a/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java b/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java index 8d02c939afef..1376bbc6af22 100644 --- a/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java +++ b/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java @@ -29,7 +29,7 @@ class TestRootManifestFile { private static final long SNAPSHOT_ID = 42L; private static final byte[] CONTENTS = new byte[] {1, 2, 3, 4, 5}; - private static final String LOCATION = "s3://bucket/db/table/metadata/root.avro"; + private static final String LOCATION = "s3://bucket/db/table/metadata/root.parquet"; private final InputFile file = new InMemoryInputFile(LOCATION, CONTENTS); From 54cfbf783dc06ba8231c1d9d7dd8d2d4a59d7486 Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Fri, 11 Sep 2026 16:54:56 -0700 Subject: [PATCH 06/21] Core: Trim redundant planner tests --- .../org/apache/iceberg/TestFilePlanner.java | 62 +++++-------------- .../iceberg/TestTrackedFileAdapters.java | 2 - 2 files changed, 14 insertions(+), 50 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index e81ce4456848..52466ad2b2ae 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -167,31 +167,9 @@ void deletionVectorAttachedToTask(FileFormat format) throws IOException { delete -> { assertThat(delete.content()).isEqualTo(FileContent.POSITION_DELETES); assertThat(delete.referencedDataFile()).isEqualTo(resolved("with-dv.parquet")); - assertThat(delete.recordCount()).isEqualTo(DV_CARDINALITY); - assertThat(delete.contentOffset()).isEqualTo(DV_OFFSET); - assertThat(delete.contentSizeInBytes()).isEqualTo(DV_SIZE_IN_BYTES); }); } - @ParameterizedTest - @FieldSource("MANIFEST_FORMATS") - void partitionFilterPrunesFiles(FileFormat format) throws IOException { - InputFile root = - writeManifest( - format, - PARTITION_TYPE, - ImmutableList.of( - dataFile("keep.parquet", partition(1)), dataFile("prune.parquet", partition(2)))); - - List tasks = - plan(root, PARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("id", 1))); - - assertThat(tasks) - .hasSize(1) - .extracting(task -> task.file().location()) - .containsExactly(resolved("keep.parquet")); - } - @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void residualAttachedFromFilter(FileFormat format) throws IOException { @@ -250,7 +228,8 @@ void buildingAndClosingWithoutIteratingDoesNotScanLeaves(FileFormat format) thro ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS).tableLocation(TABLE_LOCATION) + FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS) + .tableLocation(TABLE_LOCATION) .scanMetrics(metrics) .build(); // the root is read eagerly to route its entries; leaf readers open lazily, so closing the plan @@ -317,7 +296,9 @@ void deleteContentInRootIsUnsupported(FileFormat format) throws IOException { // delete content is only produced by upgraded trees; that path is not yet implemented FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS).tableLocation(TABLE_LOCATION).build(); + FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS) + .tableLocation(TABLE_LOCATION) + .build(); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) .isInstanceOf(UnsupportedOperationException.class) .hasMessage("v3 and earlier deletes are not yet supported"); @@ -325,36 +306,19 @@ void deleteContentInRootIsUnsupported(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void deleteContentInLeafIsUnsupported(FileFormat format) throws IOException { - InputFile leaf = - writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of( - dataFile("leaf-data.parquet", EMPTY_PARTITION_DATA), - deleteManifest("leaf-deletes.avro"))); - InputFile root = - writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); - - FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS).tableLocation(TABLE_LOCATION).build(); - assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Invalid content type for DataFile: DELETE_MANIFEST"); - } - - @ParameterizedTest - @FieldSource("MANIFEST_FORMATS") - void nestedDataManifestInLeafIsRejected(FileFormat format) throws IOException { + void nonDataEntryInLeafFailsPlanning(FileFormat format) throws IOException { + // the planner feeds every leaf entry to the data-file adapter, so a non-data leaf entry + // (here a nested manifest) fails planning InputFile leaf = writeManifest( format, EMPTY_PARTITION, ImmutableList.of(dataManifest("nested-leaf.parquet"))); InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); - // a leaf holds only data entries in a two-level tree; a nested manifest is rejected FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS).tableLocation(TABLE_LOCATION).build(); + FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS) + .tableLocation(TABLE_LOCATION) + .build(); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Invalid content type for DataFile: DATA_MANIFEST"); @@ -454,7 +418,9 @@ private List plan( throws IOException { FilePlanner planner = configure - .apply(FilePlanner.builder(fileIO, asManifest(root), specsById).tableLocation(TABLE_LOCATION)) + .apply( + FilePlanner.builder(fileIO, asManifest(root), specsById) + .tableLocation(TABLE_LOCATION)) .build(); try (CloseableIterable tasks = planner.planFiles()) { return Lists.newArrayList(tasks); diff --git a/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java b/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java index 8e53acdbca16..a476e6e1cf93 100644 --- a/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java +++ b/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java @@ -509,8 +509,6 @@ void dvDeleteFileFromDataFileUnwrapsTrackedFile() { assertThat(dvFile.content()).isEqualTo(FileContent.POSITION_DELETES); assertThat(dvFile.referencedDataFile()).isEqualTo(DATA_FILE_LOCATION); assertThat(dvFile.recordCount()).isEqualTo(dv.cardinality()); - assertThat(dvFile.contentOffset()).isEqualTo(dv.offset()); - assertThat(dvFile.contentSizeInBytes()).isEqualTo(dv.sizeInBytes()); } @Test From 40291fbfbb973a17f3ef4ef5664ff173fc8ff646 Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Sat, 12 Sep 2026 11:51:21 -0700 Subject: [PATCH 07/21] Core: Drop unhelpful filterData javadoc --- core/src/main/java/org/apache/iceberg/FilePlanner.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/main/java/org/apache/iceberg/FilePlanner.java b/core/src/main/java/org/apache/iceberg/FilePlanner.java index fde5f3a76847..84af06508134 100644 --- a/core/src/main/java/org/apache/iceberg/FilePlanner.java +++ b/core/src/main/java/org/apache/iceberg/FilePlanner.java @@ -201,7 +201,6 @@ Builder tableLocation(String newTableLocation) { return this; } - /** Sets the filter used for partition pruning and residual evaluation. */ Builder filterData(Expression expr) { Preconditions.checkArgument(expr != null, "Invalid filter: null"); this.dataFilter = expr; From e767da42b7583cc52c9b1597c362097abc399202 Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Sun, 13 Sep 2026 08:17:25 -0700 Subject: [PATCH 08/21] Fix --- core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java b/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java index 455690dd809e..66010119417d 100644 --- a/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java +++ b/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java @@ -50,7 +50,7 @@ static DeleteFile asDVDeleteFile(DataFile dataFile) { Preconditions.checkArgument( dataFile instanceof TrackedDataFile, "Cannot create DV delete file from DataFile: %s", - dataFile.getClass().getName()); + dataFile); TrackedDataFile tracked = (TrackedDataFile) dataFile; return new TrackedDVDeleteFile(tracked.file(), tracked.specId()); } From bc2b0637d5139c08bd09db02defbf3fd87831c68 Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Tue, 15 Sep 2026 12:11:51 -0700 Subject: [PATCH 09/21] Iterate on tests --- .../org/apache/iceberg/TestFilePlanner.java | 20 ++++++++++++------- .../iceberg/TestTrackedFileAdapters.java | 2 ++ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index 52466ad2b2ae..c65ae460c185 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -166,7 +166,11 @@ void deletionVectorAttachedToTask(FileFormat format) throws IOException { .allSatisfy( delete -> { assertThat(delete.content()).isEqualTo(FileContent.POSITION_DELETES); + assertThat(delete.location()).isEqualTo(DV_LOCATION); assertThat(delete.referencedDataFile()).isEqualTo(resolved("with-dv.parquet")); + assertThat(delete.recordCount()).isEqualTo(DV_CARDINALITY); + assertThat(delete.contentOffset()).isEqualTo(DV_OFFSET); + assertThat(delete.contentSizeInBytes()).isEqualTo(DV_SIZE_IN_BYTES); }); } @@ -180,7 +184,6 @@ void residualAttachedFromFilter(FileFormat format) throws IOException { List withResidual = plan( root, PARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("data", "x"))); - // the identity partition on id leaves the data predicate as a residual assertThat(withResidual.get(0).residual()) .hasToString(Expressions.equal("data", "x").toString()); @@ -294,7 +297,6 @@ void deleteContentInRootIsUnsupported(FileFormat format) throws IOException { InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(deleteManifest("deletes.avro"))); - // delete content is only produced by upgraded trees; that path is not yet implemented FilePlanner planner = FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS) .tableLocation(TABLE_LOCATION) @@ -326,8 +328,8 @@ void nonDataEntryInLeafFailsPlanning(FileFormat format) throws IOException { @Test void emptyRootYieldsNoTasks() throws IOException { - // an empty root has no plannable entries regardless of manifest format; AVRO is used because - // the Parquet writer does not materialize a file when no records are appended + // Avro is used because the Parquet writer does not materialize a file when no records are + // appended. InputFile root = writeManifest(FileFormat.AVRO, EMPTY_PARTITION, ImmutableList.of()); assertThat(plan(root, UNPARTITIONED_SPECS)).isEmpty(); @@ -372,6 +374,13 @@ void planFilesAcrossMultipleSpecs(FileFormat format) throws IOException { .containsExactlyInAnyOrder( tuple(resolved("spec0-keep.parquet"), 0, Expressions.alwaysTrue().toString()), tuple(resolved("spec1.parquet"), 1, Expressions.equal("id", 1).toString())); + + // data sits at union position 1 but spec1 position 0; the emitted task must expose the + // partition in its own spec order so residual evaluation and partition-constant injection read + // the right value (this reads id=null without the projection) + FileScanTask spec1Task = + tasks.stream().filter(task -> task.spec().specId() == 1).findFirst().orElseThrow(); + assertThat(spec1Task.file().partition().get(0, CharSequence.class)).hasToString("x"); } @ParameterizedTest @@ -427,8 +436,6 @@ private List plan( } } - // the root manifest is supplied to the planner by its caller; the reader only needs its location - // and format version, so a lightweight stand-in stands in for a table-metadata-sourced root private static ManifestFile asManifest(InputFile file) { return new RootManifestFile(file, SNAPSHOT_ID, /* keyMetadata= */ null); } @@ -546,7 +553,6 @@ private InputFile writeManifest( FileFormat format, Types.StructType partitionType, Iterable files) throws IOException { Schema writeSchema = TrackedFile.schema(partitionType, Types.StructType.of()); - // write under the table location so a leaf's resolved reference round-trips to the file on disk OutputFile out = fileIO.newOutputFile( TABLE_LOCATION diff --git a/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java b/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java index a476e6e1cf93..eee2d4c19773 100644 --- a/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java +++ b/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java @@ -508,7 +508,9 @@ void dvDeleteFileFromDataFileUnwrapsTrackedFile() { assertThat(dvFile.content()).isEqualTo(FileContent.POSITION_DELETES); assertThat(dvFile.referencedDataFile()).isEqualTo(DATA_FILE_LOCATION); + assertThat(dvFile.location()).isEqualTo(dv.location()); assertThat(dvFile.recordCount()).isEqualTo(dv.cardinality()); + assertThat(dvFile.contentOffset()).isEqualTo(dv.offset()); } @Test From e2e7a54b8de3e40cba6d537e8a264270397b48af Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Tue, 15 Sep 2026 14:06:16 -0700 Subject: [PATCH 10/21] Clean up tests --- .../org/apache/iceberg/TestFilePlanner.java | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index c65ae460c185..f492a628b314 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -235,27 +235,27 @@ void buildingAndClosingWithoutIteratingDoesNotScanLeaves(FileFormat format) thro .tableLocation(TABLE_LOCATION) .scanMetrics(metrics) .build(); - // the root is read eagerly to route its entries; leaf readers open lazily, so closing the plan - // without iterating scans only the root, not the leaf planner.planFiles().close(); - assertThat(metrics.scannedDataManifests().value()).isEqualTo(1L); + assertThat(metrics.scannedDataManifests().value()) + .as( + "the root is read eagerly but leaf readers open lazily, so closing " + + "without iterating scans only the root, not the leaf") + .isEqualTo(1L); } @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void parallelPlanningMatchesSequential(FileFormat format) throws IOException { - // a DV and a residual-bearing filter in the tree so parity actually exercises createTask's DV - // branch and per-spec residual keying on worker threads, not just locations TrackedFile withDv = dataFile( - "leaf1.parquet", + "leaf1-data.parquet", partition(1), deletionVector(DV_LOCATION, DV_OFFSET, DV_SIZE_IN_BYTES, DV_CARDINALITY)); InputFile leaf1 = writeManifest(format, PARTITION_TYPE, ImmutableList.of(withDv)); InputFile leaf2 = writeManifest( - format, PARTITION_TYPE, ImmutableList.of(dataFile("leaf2.parquet", partition(1)))); + format, PARTITION_TYPE, ImmutableList.of(dataFile("leaf2-data.parquet", partition(1)))); InputFile root = writeManifest( format, @@ -309,8 +309,6 @@ void deleteContentInRootIsUnsupported(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void nonDataEntryInLeafFailsPlanning(FileFormat format) throws IOException { - // the planner feeds every leaf entry to the data-file adapter, so a non-data leaf entry - // (here a nested manifest) fails planning InputFile leaf = writeManifest( format, EMPTY_PARTITION, ImmutableList.of(dataManifest("nested-leaf.parquet"))); @@ -362,7 +360,7 @@ void planFilesAcrossMultipleSpecs(FileFormat format) throws IOException { writeManifest(format, unionType, ImmutableList.of(spec0Keep, spec0Prune, spec1File)); // id = 1 prunes the spec0 file partitioned on id=2; spec1 is not partitioned by id, so its - // residual keeps the predicate. A per-spec residual mis-keying would surface here. + // residual keeps the predicate. List tasks = plan(root, specsById, planner -> planner.filterData(Expressions.equal("id", 1))); @@ -375,9 +373,8 @@ void planFilesAcrossMultipleSpecs(FileFormat format) throws IOException { tuple(resolved("spec0-keep.parquet"), 0, Expressions.alwaysTrue().toString()), tuple(resolved("spec1.parquet"), 1, Expressions.equal("id", 1).toString())); - // data sits at union position 1 but spec1 position 0; the emitted task must expose the - // partition in its own spec order so residual evaluation and partition-constant injection read - // the right value (this reads id=null without the projection) + // `data` sits at union position 1 but spec1 position 0; the emitted task must expose the + // partition in its own spec order FileScanTask spec1Task = tasks.stream().filter(task -> task.spec().specId() == 1).findFirst().orElseThrow(); assertThat(spec1Task.file().partition().get(0, CharSequence.class)).hasToString("x"); From 47a6b839e3d58100d6f9c447b072abfa879bf72a Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Mon, 21 Sep 2026 11:39:49 -0700 Subject: [PATCH 11/21] Add schema --- .../java/org/apache/iceberg/FilePlanner.java | 32 +++++++++++++------ .../org/apache/iceberg/TestFilePlanner.java | 8 ++--- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/FilePlanner.java b/core/src/main/java/org/apache/iceberg/FilePlanner.java index 84af06508134..26e5526224df 100644 --- a/core/src/main/java/org/apache/iceberg/FilePlanner.java +++ b/core/src/main/java/org/apache/iceberg/FilePlanner.java @@ -49,6 +49,7 @@ class FilePlanner { private final FileIO io; private final ManifestFile root; + private final Schema tableSchema; private final Map specsById; private final String tableLocation; private final Expression dataFilter; @@ -61,6 +62,7 @@ class FilePlanner { private FilePlanner( FileIO io, ManifestFile root, + Schema tableSchema, Map specsById, String tableLocation, Expression dataFilter, @@ -70,6 +72,7 @@ private FilePlanner( ExecutorService executorService) { this.io = io; this.root = root; + this.tableSchema = tableSchema; this.specsById = specsById; this.tableLocation = tableLocation; this.dataFilter = dataFilter; @@ -79,8 +82,9 @@ private FilePlanner( this.executorService = executorService; } - static Builder builder(FileIO io, ManifestFile root, Map specsById) { - return new Builder(io, root, specsById); + static Builder builder( + FileIO io, ManifestFile root, Schema tableSchema, Map specsById) { + return new Builder(io, root, tableSchema, specsById); } CloseableIterable planFiles() { @@ -128,12 +132,17 @@ CloseableIterable planFiles() { } private CloseableIterable reader(ManifestFile manifest) { - return V4ManifestReader.builder(manifest, io, specsById, tableLocation) - .forScanPlanning() - .filter(dataFilter) - .caseSensitive(caseSensitive) - .scanMetrics(scanMetrics) - .build(); + V4ManifestReader.Builder builder = + V4ManifestReader.builder(manifest, io, tableSchema, specsById) + .forScanPlanning() + .filter(dataFilter) + .caseSensitive(caseSensitive) + .scanMetrics(scanMetrics); + if (tableLocation != null) { + builder.tableLocation(tableLocation); + } + + return builder.build(); } private FileScanTask createTask(DataFile dataFile) { @@ -178,6 +187,7 @@ private TaskContext(String schemaAsString, String specAsString, ResidualEvaluato static class Builder { private final FileIO io; private final ManifestFile root; + private final Schema tableSchema; private final Map specsById; private String tableLocation = null; private Expression dataFilter = Expressions.alwaysTrue(); @@ -186,12 +196,15 @@ static class Builder { private ScanMetrics scanMetrics = ScanMetrics.noop(); private ExecutorService executorService = null; - private Builder(FileIO io, ManifestFile root, Map specsById) { + private Builder( + FileIO io, ManifestFile root, Schema tableSchema, Map specsById) { Preconditions.checkArgument(io != null, "Invalid file IO: null"); Preconditions.checkArgument(root != null, "Invalid root manifest: null"); + Preconditions.checkArgument(tableSchema != null, "Invalid table schema: null"); Preconditions.checkArgument(specsById != null, "Invalid specs by ID: null"); this.io = io; this.root = root; + this.tableSchema = tableSchema; this.specsById = ImmutableMap.copyOf(specsById); } @@ -233,6 +246,7 @@ FilePlanner build() { return new FilePlanner( io, root, + tableSchema, specsById, tableLocation, dataFilter, diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index f492a628b314..d986c3e7e240 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -231,7 +231,7 @@ void buildingAndClosingWithoutIteratingDoesNotScanLeaves(FileFormat format) thro ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS) + FilePlanner.builder(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) .tableLocation(TABLE_LOCATION) .scanMetrics(metrics) .build(); @@ -298,7 +298,7 @@ void deleteContentInRootIsUnsupported(FileFormat format) throws IOException { writeManifest(format, EMPTY_PARTITION, ImmutableList.of(deleteManifest("deletes.avro"))); FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS) + FilePlanner.builder(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) .tableLocation(TABLE_LOCATION) .build(); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) @@ -316,7 +316,7 @@ void nonDataEntryInLeafFailsPlanning(FileFormat format) throws IOException { writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), UNPARTITIONED_SPECS) + FilePlanner.builder(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) .tableLocation(TABLE_LOCATION) .build(); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) @@ -425,7 +425,7 @@ private List plan( FilePlanner planner = configure .apply( - FilePlanner.builder(fileIO, asManifest(root), specsById) + FilePlanner.builder(fileIO, asManifest(root), TABLE_SCHEMA, specsById) .tableLocation(TABLE_LOCATION)) .build(); try (CloseableIterable tasks = planner.planFiles()) { From 2ac60cb37163950c3a0ef97ddde0de8f5a8c4f2a Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Mon, 21 Sep 2026 13:50:03 -0700 Subject: [PATCH 12/21] Added tests for stats-based pruning --- .../org/apache/iceberg/TestFilePlanner.java | 112 +++++++++++++++++- 1 file changed, 110 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index d986c3e7e240..19280b673052 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -39,6 +39,7 @@ import org.apache.iceberg.metrics.ScanMetrics; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.relocated.com.google.common.collect.Iterables; import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.transforms.Transforms; import org.apache.iceberg.types.Types; @@ -75,6 +76,11 @@ class TestFilePlanner { private static final List MANIFEST_FORMATS = ImmutableList.of(FileFormat.AVRO, FileFormat.PARQUET); + private static final MetricsConfig METRICS_CONFIG = + MetricsConfig.from(ImmutableMap.of(), TABLE_SCHEMA, null); + private static final Types.StructType STATS_TYPE = + StatsUtil.statsWriteSchema(TABLE_SCHEMA, METRICS_CONFIG); + private final InMemoryFileIO fileIO = new InMemoryFileIO(); @ParameterizedTest @@ -412,6 +418,56 @@ void planFilesReportsScanMetrics(FileFormat format) throws IOException { .isEqualTo(DV_SIZE_IN_BYTES); } + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void filterAppliesToLeafManifestFiles(FileFormat format) throws IOException { + InputFile leaf = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFileWithStats("leaf-keep.parquet", idStats(0, 99)), + dataFileWithStats("leaf-prune.parquet", idStats(100, 199)))); + InputFile root = + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); + + List tasks = + plan(root, UNPARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("id", 50))); + + assertThat(Iterables.getOnlyElement(tasks).file().location()) + .as("only the leaf file whose stats match the filter survives") + .isEqualTo(resolved("leaf-keep.parquet")); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void prunedLeafManifestIsNotExpanded(FileFormat format) throws IOException { + InputFile root = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataManifestWithStats( + "unopened-leaf." + format.name().toLowerCase(Locale.ROOT), idStats(100, 199)), + dataFileWithStats("root-keep.parquet", idStats(0, 99)))); + + ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); + List tasks = + plan( + root, + UNPARTITIONED_SPECS, + planner -> planner.filterData(Expressions.equal("id", 50)).scanMetrics(metrics)); + + assertThat(Iterables.getOnlyElement(tasks).file().location()) + .isEqualTo(resolved("root-keep.parquet")); + assertThat(metrics.skippedDataManifests().value()) + .as("the leaf manifest ref is pruned by its stats") + .isEqualTo(1L); + assertThat(metrics.scannedDataManifests().value()) + .as("only the root is scanned; the pruned leaf is never opened") + .isEqualTo(1L); + } + private List plan(InputFile root, Map specsById) throws IOException { return plan(root, specsById, UnaryOperator.identity()); @@ -450,11 +506,51 @@ private static TrackedFile dataFile( return trackedFile(addedTracking(), FileContent.DATA, location, specId, partition, dv, null); } + private static TrackedFile dataFileWithStats(String location, ContentStats stats) { + return trackedFile( + addedTracking(), + FileContent.DATA, + location, + specId(EMPTY_PARTITION_DATA), + EMPTY_PARTITION_DATA, + stats, + null, // dv + null); // manifestInfo + } + private static TrackedFile dataManifest(String location) { return trackedFile( addedTracking(), FileContent.DATA_MANIFEST, location, null, null, null, dataManifestInfo()); } + private static TrackedFile dataManifestWithStats(String location, ContentStats stats) { + return trackedFile( + addedTracking(), + FileContent.DATA_MANIFEST, + location, + null, // specId + null, // partition + stats, + null, // dv + dataManifestInfo()); + } + + private static ContentStats idStats(int lower, int upper) { + ContentStatsStruct stats = new ContentStatsStruct(STATS_TYPE); + stats.setStats( + TABLE_SCHEMA.findField("id").fieldId(), + new FieldStatsStruct<>( + STATS_TYPE.fieldType("id").asStructType(), + lower, + upper, + true, // tightBounds + RECORD_COUNT, + 0, // nullValueCount + 0, // nanValueCount + null)); // avgValueSize + return stats; + } + private static ManifestInfo dataManifestInfo() { return ManifestInfoStruct.builder() .addedFilesCount(1) @@ -482,6 +578,18 @@ private static TrackedFile trackedFile( PartitionData partition, DeletionVector dv, ManifestInfo manifestInfo) { + return trackedFile(tracking, contentType, location, specId, partition, null, dv, manifestInfo); + } + + private static TrackedFile trackedFile( + TrackingStruct tracking, + FileContent contentType, + String location, + Integer specId, + PartitionData partition, + ContentStats contentStats, + DeletionVector dv, + ManifestInfo manifestInfo) { return new TrackedFileStruct( tracking, contentType, @@ -492,7 +600,7 @@ private static TrackedFile trackedFile( FILE_SIZE_IN_BYTES, specId, partition, - null, // contentStats + contentStats, null, // sortOrderId dv, // deletionVector manifestInfo, @@ -549,7 +657,7 @@ private static DeletionVector deletionVector( private InputFile writeManifest( FileFormat format, Types.StructType partitionType, Iterable files) throws IOException { - Schema writeSchema = TrackedFile.schema(partitionType, Types.StructType.of()); + Schema writeSchema = TrackedFile.schema(partitionType, STATS_TYPE); OutputFile out = fileIO.newOutputFile( TABLE_LOCATION From 242707fe3ef8fc09d9b8bb56d14e30a8acfb4542 Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Fri, 25 Sep 2026 09:49:27 -0700 Subject: [PATCH 13/21] PR feedback --- .../java/org/apache/iceberg/FilePlanner.java | 167 ++++++------------ .../org/apache/iceberg/RootManifestFile.java | 20 ++- .../apache/iceberg/TrackedFileAdapters.java | 11 +- .../org/apache/iceberg/TestFilePlanner.java | 58 +++--- .../apache/iceberg/TestRootManifestFile.java | 33 ++-- .../iceberg/TestTrackedFileAdapters.java | 39 ---- 6 files changed, 124 insertions(+), 204 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/FilePlanner.java b/core/src/main/java/org/apache/iceberg/FilePlanner.java index 26e5526224df..e147cd920172 100644 --- a/core/src/main/java/org/apache/iceberg/FilePlanner.java +++ b/core/src/main/java/org/apache/iceberg/FilePlanner.java @@ -36,6 +36,7 @@ import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.util.ParallelIterable; +import org.apache.iceberg.util.ThreadPools; /** * Plans {@link FileScanTask}s from a V4 root manifest. @@ -51,51 +52,70 @@ class FilePlanner { private final ManifestFile root; private final Schema tableSchema; private final Map specsById; - private final String tableLocation; - private final Expression dataFilter; - private final boolean ignoreResiduals; - private final boolean caseSensitive; - private final ScanMetrics scanMetrics; - private final ExecutorService executorService; private final Map taskContextsBySpec = Maps.newConcurrentMap(); - private FilePlanner( - FileIO io, - ManifestFile root, - Schema tableSchema, - Map specsById, - String tableLocation, - Expression dataFilter, - boolean ignoreResiduals, - boolean caseSensitive, - ScanMetrics scanMetrics, - ExecutorService executorService) { + private String tableLocation = null; + private Expression dataFilter = Expressions.alwaysTrue(); + private boolean ignoreResiduals = false; + private boolean caseSensitive = true; + private ScanMetrics scanMetrics = ScanMetrics.noop(); + private ExecutorService executorService = ThreadPools.getWorkerPool(); + + FilePlanner( + FileIO io, ManifestFile root, Schema tableSchema, Map specsById) { + Preconditions.checkArgument(io != null, "Invalid file IO: null"); + Preconditions.checkArgument(root != null, "Invalid root manifest: null"); + Preconditions.checkArgument(tableSchema != null, "Invalid table schema: null"); + Preconditions.checkArgument(specsById != null, "Invalid specs by ID: null"); this.io = io; this.root = root; this.tableSchema = tableSchema; - this.specsById = specsById; - this.tableLocation = tableLocation; - this.dataFilter = dataFilter; - this.ignoreResiduals = ignoreResiduals; - this.caseSensitive = caseSensitive; - this.scanMetrics = scanMetrics; - this.executorService = executorService; + this.specsById = ImmutableMap.copyOf(specsById); } - static Builder builder( - FileIO io, ManifestFile root, Schema tableSchema, Map specsById) { - return new Builder(io, root, tableSchema, specsById); + FilePlanner tableLocation(String newTableLocation) { + Preconditions.checkArgument(newTableLocation != null, "Invalid table location: null"); + this.tableLocation = newTableLocation; + return this; + } + + FilePlanner filterData(Expression expr) { + Preconditions.checkArgument(expr != null, "Invalid filter: null"); + this.dataFilter = expr; + return this; + } + + FilePlanner ignoreResiduals() { + this.ignoreResiduals = true; + return this; + } + + FilePlanner caseSensitive(boolean newCaseSensitive) { + this.caseSensitive = newCaseSensitive; + return this; + } + + FilePlanner scanMetrics(ScanMetrics newScanMetrics) { + Preconditions.checkArgument(newScanMetrics != null, "Invalid scan metrics: null"); + this.scanMetrics = newScanMetrics; + return this; + } + + FilePlanner planWith(ExecutorService newExecutorService) { + Preconditions.checkArgument(newExecutorService != null, "Invalid executor service: null"); + this.executorService = newExecutorService; + return this; } CloseableIterable planFiles() { - List rootDataFiles = Lists.newArrayList(); + List rootDataFiles = Lists.newArrayList(); List leafManifests = Lists.newArrayList(); try (CloseableIterable rootEntries = reader(root)) { for (TrackedFile entry : rootEntries) { switch (entry.contentType()) { case DATA: - rootDataFiles.add(entry); + rootDataFiles.add(TrackedFileAdapters.asDataFile(entry, specsById)); break; case DATA_MANIFEST: leafManifests.add(TrackedFileAdapters.asManifestFile(entry)); @@ -117,16 +137,17 @@ CloseableIterable planFiles() { } CloseableIterable leafFiles = - executorService != null && leafManifests.size() > 1 + leafManifests.size() > 1 ? new ParallelIterable<>(leafPlanTasks, executorService) : CloseableIterable.concat(leafPlanTasks); - CloseableIterable files = - CloseableIterable.concat( - ImmutableList.of(CloseableIterable.withNoopClose(rootDataFiles), leafFiles)); + CloseableIterable leafDataFiles = + CloseableIterable.transform( + leafFiles, file -> TrackedFileAdapters.asDataFile(file, specsById)); CloseableIterable dataFiles = - CloseableIterable.transform(files, file -> TrackedFileAdapters.asDataFile(file, specsById)); + CloseableIterable.concat( + ImmutableList.of(CloseableIterable.withNoopClose(rootDataFiles), leafDataFiles)); return CloseableIterable.transform(dataFiles, this::createTask); } @@ -151,8 +172,9 @@ private FileScanTask createTask(DataFile dataFile) { DeleteFile[] deletes = NO_DELETES; if (dataFile.deletionVector() != null) { - DeleteFile dv = TrackedFileAdapters.asDVDeleteFile(dataFile); - ScanMetricsUtil.indexedDeleteFile(scanMetrics, dv); + TrackedFile tracked = ((TrackedFileAdapters.TrackedDataFile) dataFile).file(); + DeleteFile dv = TrackedFileAdapters.asDVDeleteFile(tracked, specsById); + scanMetrics.dvs().increment(); deletes = new DeleteFile[] {dv}; } @@ -183,77 +205,4 @@ private TaskContext(String schemaAsString, String specAsString, ResidualEvaluato this.residuals = residuals; } } - - static class Builder { - private final FileIO io; - private final ManifestFile root; - private final Schema tableSchema; - private final Map specsById; - private String tableLocation = null; - private Expression dataFilter = Expressions.alwaysTrue(); - private boolean ignoreResiduals = false; - private boolean caseSensitive = true; - private ScanMetrics scanMetrics = ScanMetrics.noop(); - private ExecutorService executorService = null; - - private Builder( - FileIO io, ManifestFile root, Schema tableSchema, Map specsById) { - Preconditions.checkArgument(io != null, "Invalid file IO: null"); - Preconditions.checkArgument(root != null, "Invalid root manifest: null"); - Preconditions.checkArgument(tableSchema != null, "Invalid table schema: null"); - Preconditions.checkArgument(specsById != null, "Invalid specs by ID: null"); - this.io = io; - this.root = root; - this.tableSchema = tableSchema; - this.specsById = ImmutableMap.copyOf(specsById); - } - - Builder tableLocation(String newTableLocation) { - Preconditions.checkArgument(newTableLocation != null, "Invalid table location: null"); - this.tableLocation = newTableLocation; - return this; - } - - Builder filterData(Expression expr) { - Preconditions.checkArgument(expr != null, "Invalid filter: null"); - this.dataFilter = expr; - return this; - } - - Builder ignoreResiduals() { - this.ignoreResiduals = true; - return this; - } - - Builder caseSensitive(boolean newCaseSensitive) { - this.caseSensitive = newCaseSensitive; - return this; - } - - Builder scanMetrics(ScanMetrics newScanMetrics) { - Preconditions.checkArgument(newScanMetrics != null, "Invalid scan metrics: null"); - this.scanMetrics = newScanMetrics; - return this; - } - - Builder planWith(ExecutorService newExecutorService) { - Preconditions.checkArgument(newExecutorService != null, "Invalid executor service: null"); - this.executorService = newExecutorService; - return this; - } - - FilePlanner build() { - return new FilePlanner( - io, - root, - tableSchema, - specsById, - tableLocation, - dataFilter, - ignoreResiduals, - caseSensitive, - scanMetrics, - executorService); - } - } } diff --git a/core/src/main/java/org/apache/iceberg/RootManifestFile.java b/core/src/main/java/org/apache/iceberg/RootManifestFile.java index 87866cc250be..6eb52302f63c 100644 --- a/core/src/main/java/org/apache/iceberg/RootManifestFile.java +++ b/core/src/main/java/org/apache/iceberg/RootManifestFile.java @@ -30,13 +30,22 @@ class RootManifestFile implements ManifestFile { private final InputFile file; private final long snapshotId; + private final long sequenceNumber; + private final Long firstRowId; private final byte[] keyMetadata; private Long length; - RootManifestFile(InputFile file, long snapshotId, ByteBuffer keyMetadata) { + RootManifestFile( + InputFile file, + long snapshotId, + long sequenceNumber, + Long firstRowId, + ByteBuffer keyMetadata) { Preconditions.checkArgument(file != null, "Invalid file: null"); this.file = file; this.snapshotId = snapshotId; + this.sequenceNumber = sequenceNumber; + this.firstRowId = firstRowId; this.keyMetadata = ByteBuffers.toByteArray(keyMetadata); this.length = null; } @@ -67,7 +76,7 @@ public ManifestContent content() { @Override public long sequenceNumber() { - throw new UnsupportedOperationException("Root manifest has no sequence number"); + return sequenceNumber; } @Override @@ -120,6 +129,11 @@ public ByteBuffer keyMetadata() { return keyMetadata == null ? null : ByteBuffer.wrap(keyMetadata); } + @Override + public Long firstRowId() { + return firstRowId; + } + @Override public int formatVersion() { return FORMAT_VERSION; @@ -128,6 +142,6 @@ public int formatVersion() { @Override public ManifestFile copy() { ByteBuffer keyMetadataCopy = keyMetadata == null ? null : ByteBuffer.wrap(keyMetadata.clone()); - return new RootManifestFile(file, snapshotId, keyMetadataCopy); + return new RootManifestFile(file, snapshotId, sequenceNumber, firstRowId, keyMetadataCopy); } } diff --git a/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java b/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java index 66010119417d..175e1e00bf4a 100644 --- a/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java +++ b/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java @@ -46,15 +46,6 @@ static DeleteFile asDVDeleteFile(TrackedFile file, Map s return new TrackedDVDeleteFile(file, resolveSpecId(file, specsById)); } - static DeleteFile asDVDeleteFile(DataFile dataFile) { - Preconditions.checkArgument( - dataFile instanceof TrackedDataFile, - "Cannot create DV delete file from DataFile: %s", - dataFile); - TrackedDataFile tracked = (TrackedDataFile) dataFile; - return new TrackedDVDeleteFile(tracked.file(), tracked.specId()); - } - static DeleteFile asEqualityDeleteFile(TrackedFile file, Map specsById) { Preconditions.checkArgument( file.contentType() == FileContent.EQUALITY_DELETES, @@ -214,7 +205,7 @@ public Map upperBounds() { } /** Adapts a TrackedFile DATA entry to the {@link DataFile} interface. */ - private static class TrackedDataFile extends TrackedContentFile implements DataFile { + static class TrackedDataFile extends TrackedContentFile implements DataFile { private TrackedDataFile(TrackedFile file, int specId) { super(file, specId); } diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index 19280b673052..884581632bb9 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -51,6 +51,8 @@ class TestFilePlanner { private static final long SNAPSHOT_ID = 42L; + private static final long SEQUENCE_NUMBER = 7L; + private static final long FIRST_ROW_ID = 1000L; private static final int WRITER_FORMAT_VERSION = 4; private static final long RECORD_COUNT = 100L; private static final long FILE_SIZE_IN_BYTES = 1024L; @@ -237,10 +239,9 @@ void buildingAndClosingWithoutIteratingDoesNotScanLeaves(FileFormat format) thro ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) + new FilePlanner(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) .tableLocation(TABLE_LOCATION) - .scanMetrics(metrics) - .build(); + .scanMetrics(metrics); planner.planFiles().close(); assertThat(metrics.scannedDataManifests().value()) @@ -252,7 +253,7 @@ void buildingAndClosingWithoutIteratingDoesNotScanLeaves(FileFormat format) thro @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void parallelPlanningMatchesSequential(FileFormat format) throws IOException { + void parallelLeafExpansionYieldsAllFiles(FileFormat format) throws IOException { TrackedFile withDv = dataFile( "leaf1-data.parquet", @@ -268,30 +269,22 @@ void parallelPlanningMatchesSequential(FileFormat format) throws IOException { PARTITION_TYPE, ImmutableList.of(dataManifest(leaf1.location()), dataManifest(leaf2.location()))); - List sequential = - plan( - root, PARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("data", "x"))); - ExecutorService pool = ThreadPools.newFixedThreadPool("test-scan-task-planner", 2); try { - List parallel = + List tasks = plan( root, PARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("data", "x")).planWith(pool)); - assertThat(parallel) + String residual = Expressions.equal("data", "x").toString(); + assertThat(tasks) .extracting( task -> task.file().location(), task -> task.residual().toString(), task -> task.deletes().size()) - .containsExactlyInAnyOrderElementsOf( - Lists.transform( - sequential, - task -> - tuple( - task.file().location(), - task.residual().toString(), - task.deletes().size()))); + .containsExactlyInAnyOrder( + tuple(resolved("leaf1-data.parquet"), residual, 1), + tuple(resolved("leaf2-data.parquet"), residual, 0)); } finally { pool.shutdownNow(); } @@ -304,9 +297,8 @@ void deleteContentInRootIsUnsupported(FileFormat format) throws IOException { writeManifest(format, EMPTY_PARTITION, ImmutableList.of(deleteManifest("deletes.avro"))); FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) - .tableLocation(TABLE_LOCATION) - .build(); + new FilePlanner(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) + .tableLocation(TABLE_LOCATION); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) .isInstanceOf(UnsupportedOperationException.class) .hasMessage("v3 and earlier deletes are not yet supported"); @@ -322,9 +314,8 @@ void nonDataEntryInLeafFailsPlanning(FileFormat format) throws IOException { writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); FilePlanner planner = - FilePlanner.builder(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) - .tableLocation(TABLE_LOCATION) - .build(); + new FilePlanner(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) + .tableLocation(TABLE_LOCATION); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Invalid content type for DataFile: DATA_MANIFEST"); @@ -416,6 +407,10 @@ void planFilesReportsScanMetrics(FileFormat format) throws IOException { assertThat(metrics.totalDeleteFileSizeInBytes().value()) .as("the DV delete contributes its size") .isEqualTo(DV_SIZE_IN_BYTES); + assertThat(metrics.dvs().value()).isEqualTo(1L); + assertThat(metrics.indexedDeleteFiles().value()) + .as("co-located DVs are not indexed delete files") + .isEqualTo(0L); } @ParameterizedTest @@ -474,23 +469,20 @@ private List plan(InputFile root, Map spec } private List plan( - InputFile root, - Map specsById, - UnaryOperator configure) + InputFile root, Map specsById, UnaryOperator configure) throws IOException { FilePlanner planner = - configure - .apply( - FilePlanner.builder(fileIO, asManifest(root), TABLE_SCHEMA, specsById) - .tableLocation(TABLE_LOCATION)) - .build(); + configure.apply( + new FilePlanner(fileIO, asManifest(root), TABLE_SCHEMA, specsById) + .tableLocation(TABLE_LOCATION)); try (CloseableIterable tasks = planner.planFiles()) { return Lists.newArrayList(tasks); } } private static ManifestFile asManifest(InputFile file) { - return new RootManifestFile(file, SNAPSHOT_ID, /* keyMetadata= */ null); + return new RootManifestFile( + file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, /* keyMetadata= */ null); } private static TrackedFile dataFile(String location, PartitionData partition) { diff --git a/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java b/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java index 1376bbc6af22..68664d085053 100644 --- a/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java +++ b/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java @@ -28,23 +28,30 @@ class TestRootManifestFile { private static final long SNAPSHOT_ID = 42L; + private static final long SEQUENCE_NUMBER = 7L; + private static final long FIRST_ROW_ID = 1000L; private static final byte[] CONTENTS = new byte[] {1, 2, 3, 4, 5}; private static final String LOCATION = "s3://bucket/db/table/metadata/root.parquet"; private final InputFile file = new InMemoryInputFile(LOCATION, CONTENTS); @Test - void exposesLocationLengthAndSnapshotId() { - RootManifestFile root = new RootManifestFile(file, SNAPSHOT_ID, /* keyMetadata= */ null); + void exposesFileMetadata() { + RootManifestFile root = + new RootManifestFile( + file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, /* keyMetadata= */ null); assertThat(root.path()).isEqualTo(LOCATION); assertThat(root.length()).isEqualTo(CONTENTS.length); assertThat(root.snapshotId()).isEqualTo(SNAPSHOT_ID); + assertThat(root.sequenceNumber()).isEqualTo(SEQUENCE_NUMBER); + assertThat(root.firstRowId()).isEqualTo(FIRST_ROW_ID); } @Test void readableAsAV4DataManifest() { - RootManifestFile root = new RootManifestFile(file, SNAPSHOT_ID, null); + RootManifestFile root = + new RootManifestFile(file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, null); assertThat(root.content()).isEqualTo(ManifestContent.DATA); assertThat(root.formatVersion()).isEqualTo(4); @@ -52,22 +59,30 @@ void readableAsAV4DataManifest() { @Test void keyMetadataRoundTrips() { - assertThat(new RootManifestFile(file, SNAPSHOT_ID, null).keyMetadata()).isNull(); + assertThat( + new RootManifestFile(file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, null) + .keyMetadata()) + .isNull(); ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {9, 8, 7}); - assertThat(new RootManifestFile(file, SNAPSHOT_ID, keyMetadata).keyMetadata()) + assertThat( + new RootManifestFile(file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, keyMetadata) + .keyMetadata()) .isEqualTo(keyMetadata); } @Test void copyPreservesValues() { ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {9, 8, 7}); - RootManifestFile original = new RootManifestFile(file, SNAPSHOT_ID, keyMetadata); + RootManifestFile original = + new RootManifestFile(file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, keyMetadata); ManifestFile copy = original.copy(); assertThat(copy.path()).isEqualTo(LOCATION); assertThat(copy.length()).isEqualTo(CONTENTS.length); assertThat(copy.snapshotId()).isEqualTo(SNAPSHOT_ID); + assertThat(copy.sequenceNumber()).isEqualTo(SEQUENCE_NUMBER); + assertThat(copy.firstRowId()).isEqualTo(FIRST_ROW_ID); assertThat(copy.keyMetadata()).isEqualTo(keyMetadata); assertThat(copy.keyMetadata().array()) .as("copy should not share the key metadata backing array") @@ -76,14 +91,12 @@ void copyPreservesValues() { @Test void unsupportedAccessorsThrow() { - RootManifestFile root = new RootManifestFile(file, SNAPSHOT_ID, null); + RootManifestFile root = + new RootManifestFile(file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, null); assertThatThrownBy(root::partitionSpecId) .isInstanceOf(UnsupportedOperationException.class) .hasMessage("Root manifest has no partition spec"); - assertThatThrownBy(root::sequenceNumber) - .isInstanceOf(UnsupportedOperationException.class) - .hasMessage("Root manifest has no sequence number"); assertThatThrownBy(root::minSequenceNumber) .isInstanceOf(UnsupportedOperationException.class) .hasMessage("Root manifest has no minimum sequence number"); diff --git a/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java b/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java index eee2d4c19773..d3af965ec1dc 100644 --- a/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java +++ b/core/src/test/java/org/apache/iceberg/TestTrackedFileAdapters.java @@ -481,45 +481,6 @@ void dvDeleteFileAdapterRejectsNullDeletionVector() { .hasMessage("Cannot create DV delete file: no deletion vector"); } - @Test - void dvDeleteFileFromDataFileUnwrapsTrackedFile() { - DeletionVector dv = deletionVector(); - TrackedFile file = - new TrackedFileStruct( - null, // tracking - FileContent.DATA, - FORMAT_VERSION_V4, - DATA_FILE_LOCATION, - FileFormat.PARQUET, - 100L, // recordCount - 1024L, // fileSizeInBytes - PARTITIONED_SPEC_ID, - PARTITION, - null, // contentStats - null, // sortOrderId - dv, // deletionVector - null, // manifestInfo - null, // keyMetadata - null, // splitOffsets - null); // equalityIds - - DataFile dataFile = TrackedFileAdapters.asDataFile(file, specsById(PARTITIONED_SPEC)); - DeleteFile dvFile = TrackedFileAdapters.asDVDeleteFile(dataFile); - - assertThat(dvFile.content()).isEqualTo(FileContent.POSITION_DELETES); - assertThat(dvFile.referencedDataFile()).isEqualTo(DATA_FILE_LOCATION); - assertThat(dvFile.location()).isEqualTo(dv.location()); - assertThat(dvFile.recordCount()).isEqualTo(dv.cardinality()); - assertThat(dvFile.contentOffset()).isEqualTo(dv.offset()); - } - - @Test - void dvDeleteFileFromDataFileRejectsNonTrackedDataFile() { - assertThatThrownBy(() -> TrackedFileAdapters.asDVDeleteFile(mock(DataFile.class))) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Cannot create DV delete file from DataFile"); - } - @ParameterizedTest @EnumSource( value = FileContent.class, From 9996abd0b8b08350dba3fdd64e082287a91dc71f Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Tue, 29 Sep 2026 21:32:45 -0700 Subject: [PATCH 14/21] Code feedback --- core/src/main/java/org/apache/iceberg/FilePlanner.java | 6 +++--- core/src/main/java/org/apache/iceberg/RootManifestFile.java | 3 +-- .../main/java/org/apache/iceberg/TrackedFileAdapters.java | 6 +++++- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/FilePlanner.java b/core/src/main/java/org/apache/iceberg/FilePlanner.java index e147cd920172..346578659ac7 100644 --- a/core/src/main/java/org/apache/iceberg/FilePlanner.java +++ b/core/src/main/java/org/apache/iceberg/FilePlanner.java @@ -52,7 +52,7 @@ class FilePlanner { private final ManifestFile root; private final Schema tableSchema; private final Map specsById; - private final Map taskContextsBySpec = Maps.newConcurrentMap(); + private final Map taskContextsBySpec = Maps.newHashMap(); private String tableLocation = null; private Expression dataFilter = Expressions.alwaysTrue(); @@ -147,7 +147,7 @@ CloseableIterable planFiles() { CloseableIterable dataFiles = CloseableIterable.concat( - ImmutableList.of(CloseableIterable.withNoopClose(rootDataFiles), leafDataFiles)); + ImmutableList.of(CloseableIterable.of(rootDataFiles), leafDataFiles)); return CloseableIterable.transform(dataFiles, this::createTask); } @@ -172,7 +172,7 @@ private FileScanTask createTask(DataFile dataFile) { DeleteFile[] deletes = NO_DELETES; if (dataFile.deletionVector() != null) { - TrackedFile tracked = ((TrackedFileAdapters.TrackedDataFile) dataFile).file(); + TrackedFile tracked = TrackedFileAdapters.asTrackedFile(dataFile); DeleteFile dv = TrackedFileAdapters.asDVDeleteFile(tracked, specsById); scanMetrics.dvs().increment(); deletes = new DeleteFile[] {dv}; diff --git a/core/src/main/java/org/apache/iceberg/RootManifestFile.java b/core/src/main/java/org/apache/iceberg/RootManifestFile.java index 6eb52302f63c..4a4df8c554de 100644 --- a/core/src/main/java/org/apache/iceberg/RootManifestFile.java +++ b/core/src/main/java/org/apache/iceberg/RootManifestFile.java @@ -33,7 +33,7 @@ class RootManifestFile implements ManifestFile { private final long sequenceNumber; private final Long firstRowId; private final byte[] keyMetadata; - private Long length; + private Long length = null; RootManifestFile( InputFile file, @@ -47,7 +47,6 @@ class RootManifestFile implements ManifestFile { this.sequenceNumber = sequenceNumber; this.firstRowId = firstRowId; this.keyMetadata = ByteBuffers.toByteArray(keyMetadata); - this.length = null; } @Override diff --git a/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java b/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java index 175e1e00bf4a..7936b2ef31b9 100644 --- a/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java +++ b/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java @@ -38,6 +38,10 @@ static DataFile asDataFile(TrackedFile file, Map specsBy return new TrackedDataFile(file, resolveSpecId(file, specsById)); } + static TrackedFile asTrackedFile(DataFile file) { + return ((TrackedDataFile) file).file(); + } + static DeleteFile asDVDeleteFile(TrackedFile file, Map specsById) { Preconditions.checkArgument( file.contentType() == FileContent.DATA, @@ -205,7 +209,7 @@ public Map upperBounds() { } /** Adapts a TrackedFile DATA entry to the {@link DataFile} interface. */ - static class TrackedDataFile extends TrackedContentFile implements DataFile { + private static class TrackedDataFile extends TrackedContentFile implements DataFile { private TrackedDataFile(TrackedFile file, int specId) { super(file, specId); } From 69a42756e88ab6d078e24077bf72ec6a4d5e729b Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Thu, 1 Oct 2026 10:10:26 -0700 Subject: [PATCH 15/21] accept null executor --- core/src/main/java/org/apache/iceberg/FilePlanner.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/FilePlanner.java b/core/src/main/java/org/apache/iceberg/FilePlanner.java index 346578659ac7..78c2206b4793 100644 --- a/core/src/main/java/org/apache/iceberg/FilePlanner.java +++ b/core/src/main/java/org/apache/iceberg/FilePlanner.java @@ -102,7 +102,6 @@ FilePlanner scanMetrics(ScanMetrics newScanMetrics) { } FilePlanner planWith(ExecutorService newExecutorService) { - Preconditions.checkArgument(newExecutorService != null, "Invalid executor service: null"); this.executorService = newExecutorService; return this; } @@ -137,7 +136,7 @@ CloseableIterable planFiles() { } CloseableIterable leafFiles = - leafManifests.size() > 1 + executorService != null && leafManifests.size() > 1 ? new ParallelIterable<>(leafPlanTasks, executorService) : CloseableIterable.concat(leafPlanTasks); From 34f3b2b157998bb035a2c65f5c0ae3ce17136839 Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Thu, 1 Oct 2026 11:12:42 -0700 Subject: [PATCH 16/21] Test reorg --- .../org/apache/iceberg/TestFilePlanner.java | 617 +++++++++++------- 1 file changed, 364 insertions(+), 253 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index 884581632bb9..cf8510d2ab43 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -27,25 +27,28 @@ import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.concurrent.ExecutorService; +import java.util.Set; import java.util.function.UnaryOperator; +import org.apache.iceberg.exceptions.ValidationException; +import org.apache.iceberg.expressions.Expression; import org.apache.iceberg.expressions.Expressions; import org.apache.iceberg.inmemory.InMemoryFileIO; import org.apache.iceberg.io.CloseableIterable; +import org.apache.iceberg.io.CloseableIterator; import org.apache.iceberg.io.FileAppender; +import org.apache.iceberg.io.FileIO; import org.apache.iceberg.io.InputFile; import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.io.SeekableInputStream; import org.apache.iceberg.metrics.DefaultMetricsContext; import org.apache.iceberg.metrics.ScanMetrics; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.relocated.com.google.common.collect.Iterables; import org.apache.iceberg.relocated.com.google.common.collect.Lists; -import org.apache.iceberg.transforms.Transforms; +import org.apache.iceberg.relocated.com.google.common.collect.Sets; import org.apache.iceberg.types.Types; import org.apache.iceberg.util.LocationUtil; -import org.apache.iceberg.util.ThreadPools; -import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.FieldSource; @@ -67,14 +70,21 @@ class TestFilePlanner { optional(1, "id", Types.IntegerType.get()), optional(2, "data", Types.StringType.get())); private static final PartitionSpec SPEC = PartitionSpec.builderFor(TABLE_SCHEMA).identity("id").build(); - private static final Types.StructType PARTITION_TYPE = SPEC.partitionType(); private static final Types.StructType EMPTY_PARTITION = Types.StructType.of(); private static final PartitionData EMPTY_PARTITION_DATA = new PartitionData(EMPTY_PARTITION); - private static final Map PARTITIONED_SPECS = - ImmutableMap.of(SPEC.specId(), SPEC); private static final Map UNPARTITIONED_SPECS = ImmutableMap.of(PartitionSpec.unpartitioned().specId(), PartitionSpec.unpartitioned()); + // a table that evolved from unpartitioned (spec 0) to identity(id) (spec 1) + private static final PartitionSpec UNPARTITIONED_WITH_SCHEMA = + PartitionSpec.builderFor(TABLE_SCHEMA).withSpecId(0).build(); + private static final PartitionSpec BY_ID_SPEC = + PartitionSpec.builderFor(TABLE_SCHEMA).withSpecId(1).identity("id").build(); + private static final Map MIXED_SPECS = + ImmutableMap.of( + UNPARTITIONED_WITH_SCHEMA.specId(), UNPARTITIONED_WITH_SCHEMA, + BY_ID_SPEC.specId(), BY_ID_SPEC); + private static final List MANIFEST_FORMATS = ImmutableList.of(FileFormat.AVRO, FileFormat.PARQUET); @@ -87,7 +97,7 @@ class TestFilePlanner { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void rootWithDirectDataEntries(FileFormat format) throws IOException { + void rootWithOnlyDataFiles(FileFormat format) throws IOException { InputFile root = writeManifest( format, @@ -107,58 +117,129 @@ void rootWithDirectDataEntries(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void rootWithLeafManifestEntriesOnly(FileFormat format) throws IOException { - InputFile leaf = + void rootWithOnlyLeafManifests(FileFormat format) throws IOException { + InputFile leafA = writeManifest( format, EMPTY_PARTITION, - ImmutableList.of( - dataFile("leaf-a.parquet", EMPTY_PARTITION_DATA), - dataFile("leaf-b.parquet", EMPTY_PARTITION_DATA))); + ImmutableList.of(dataFile("leaf-a.parquet", EMPTY_PARTITION_DATA))); + InputFile leafB = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of(dataFile("leaf-b.parquet", EMPTY_PARTITION_DATA))); InputFile root = - writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of(dataManifest(leafA.location()), dataManifest(leafB.location()))); - ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); - List tasks = - plan(root, UNPARTITIONED_SPECS, planner -> planner.scanMetrics(metrics)); + List tasks = plan(root, UNPARTITIONED_SPECS); assertThat(tasks) .hasSize(2) .extracting(task -> task.file().location()) .containsExactlyInAnyOrder(resolved("leaf-a.parquet"), resolved("leaf-b.parquet")); - assertThat(metrics.scannedDataManifests().value()) - .as("the root and the expanded leaf are each counted as a scanned manifest") - .isEqualTo(2L); } @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void mixedRootDataAndLeafManifest(FileFormat format) throws IOException { - InputFile leaf = + void rootWithMixedDataAndLeafManifests(FileFormat format) throws IOException { + InputFile leaf1 = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of(dataFile("leaf-file-1.parquet", EMPTY_PARTITION_DATA))); + InputFile leaf2 = writeManifest( format, EMPTY_PARTITION, - ImmutableList.of(dataFile("leaf-data-file.parquet", EMPTY_PARTITION_DATA))); + ImmutableList.of(dataFile("leaf-file-2.parquet", EMPTY_PARTITION_DATA))); InputFile root = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( dataFile("root-data-file.parquet", EMPTY_PARTITION_DATA), - dataManifest(leaf.location()))); + dataManifest(leaf1.location()), + dataManifest(leaf2.location()))); List tasks = plan(root, UNPARTITIONED_SPECS); assertThat(tasks) - .hasSize(2) + .hasSize(3) .extracting(task -> task.file().location()) .containsExactlyInAnyOrder( - resolved("root-data-file.parquet"), resolved("leaf-data-file.parquet")); + resolved("root-data-file.parquet"), + resolved("leaf-file-1.parquet"), + resolved("leaf-file-2.parquet")); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void filterSkipsAndMatchesRootDataFilesAndManifests(FileFormat format) throws IOException { + InputFile root = filterableRoot(format); + + List tasks = + plan(root, UNPARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("id", 50))); + + assertThat(tasks) + .extracting(task -> task.file().location()) + .as( + "matched root data file and the file in the matched leaf survive; pruned data file and leaf do not") + .containsExactlyInAnyOrder(resolved("root-keep.parquet"), resolved("leaf-file.parquet")); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void filterSkipsAndMatchesLeafDataFiles(FileFormat format) throws IOException { + InputFile leaf = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFileWithStats("leaf-keep.parquet", idStats(0, 99)), + dataFileWithStats("leaf-prune.parquet", idStats(100, 199)))); + InputFile root = + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location(), 2))); + + List tasks = + plan(root, UNPARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("id", 50))); + + assertThat(Iterables.getOnlyElement(tasks).file().location()) + .as("only the leaf file whose stats match the filter survives") + .isEqualTo(resolved("leaf-keep.parquet")); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void filterHonorsCaseSensitivity(FileFormat format) throws IOException { + InputFile root = filterableRoot(format); + + List tasks = + plan( + root, + UNPARTITIONED_SPECS, + planner -> planner.filterData(Expressions.equal("ID", 50)).caseSensitive(false)); + assertThat(tasks) + .extracting(task -> task.file().location()) + .as("a case-insensitive filter resolves \"ID\" to \"id\" everywhere the filter is applied") + .containsExactlyInAnyOrder(resolved("root-keep.parquet"), resolved("leaf-file.parquet")); + + assertThatThrownBy( + () -> + plan( + root, + UNPARTITIONED_SPECS, + planner -> planner.filterData(Expressions.equal("ID", 50)))) + .as("the default case-sensitive filter cannot resolve \"ID\"") + .isInstanceOf(ValidationException.class) + .hasMessageContaining("Cannot find field 'ID'"); } @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void deletionVectorAttachedToTask(FileFormat format) throws IOException { + void colocatedDvAttachedAsPositionDeleteFile(FileFormat format) throws IOException { TrackedFile fileWithDv = dataFile( "with-dv.parquet", @@ -171,64 +252,69 @@ void deletionVectorAttachedToTask(FileFormat format) throws IOException { assertThat(tasks).hasSize(1); assertThat(tasks.get(0).deletes()) .hasSize(1) - .allSatisfy( - delete -> { - assertThat(delete.content()).isEqualTo(FileContent.POSITION_DELETES); - assertThat(delete.location()).isEqualTo(DV_LOCATION); - assertThat(delete.referencedDataFile()).isEqualTo(resolved("with-dv.parquet")); - assertThat(delete.recordCount()).isEqualTo(DV_CARDINALITY); - assertThat(delete.contentOffset()).isEqualTo(DV_OFFSET); - assertThat(delete.contentSizeInBytes()).isEqualTo(DV_SIZE_IN_BYTES); - }); + .allSatisfy(delete -> assertThat(delete.location()).isEqualTo(DV_LOCATION)); } @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void residualAttachedFromFilter(FileFormat format) throws IOException { - InputFile root = - writeManifest( - format, PARTITION_TYPE, ImmutableList.of(dataFile("keep.parquet", partition(1)))); + void residualForMixedPartitionedAndUnpartitioned(FileFormat format) throws IOException { + InputFile root = mixedPartitionedAndUnpartitionedRoot(format); - List withResidual = - plan( - root, PARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("data", "x"))); - assertThat(withResidual.get(0).residual()) - .hasToString(Expressions.equal("data", "x").toString()); + Expression filter = Expressions.and(Expressions.equal("id", 1), Expressions.equal("data", "x")); + List tasks = plan(root, MIXED_SPECS, planner -> planner.filterData(filter)); - List ignored = - plan( - root, - PARTITIONED_SPECS, - planner -> planner.filterData(Expressions.equal("data", "x")).ignoreResiduals()); - assertThat(ignored.get(0).residual()).isEqualTo(Expressions.alwaysTrue()); + assertThat(tasks) + .extracting(task -> task.file().location(), task -> task.residual().toString()) + .as( + "the id predicate drops out of the partitioned file's residual but not the unpartitioned") + .containsExactlyInAnyOrder( + tuple(resolved("partitioned.parquet"), Expressions.equal("data", "x").toString()), + tuple(resolved("unpartitioned.parquet"), filter.toString())); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void ignoreResidualsProducesAlwaysTrueResidual(FileFormat format) throws IOException { + InputFile root = mixedPartitionedAndUnpartitionedRoot(format); + + Expression filter = Expressions.and(Expressions.equal("id", 1), Expressions.equal("data", "x")); + List tasks = + plan(root, MIXED_SPECS, planner -> planner.filterData(filter).ignoreResiduals()); + + assertThat(tasks) + .as("ignoreResiduals forces an alwaysTrue residual for every file regardless of spec") + .hasSize(2) + .allSatisfy(task -> assertThat(task.residual()).isEqualTo(Expressions.alwaysTrue())); } @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void sameSpecFilesShareContext(FileFormat format) throws IOException { + void tableLocationForwardedToRootAndLeafReaders(FileFormat format) throws IOException { + InputFile leaf = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of(dataFile("leaf-data.parquet", EMPTY_PARTITION_DATA))); InputFile root = writeManifest( format, - PARTITION_TYPE, + EMPTY_PARTITION, ImmutableList.of( - dataFile("a.parquet", partition(1)), dataFile("b.parquet", partition(1)))); + dataFile("root-data.parquet", EMPTY_PARTITION_DATA), + dataManifest(leaf.location()))); - List tasks = - plan( - root, PARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("data", "x"))); + List tasks = plan(root, UNPARTITIONED_SPECS); - // both files share spec 0, so both tasks resolve from the same shared context - assertThat(tasks).hasSize(2); - FileScanTask first = tasks.get(0); - FileScanTask second = tasks.get(1); - assertThat(second.schema()).isSameAs(first.schema()); - assertThat(second.spec()).isSameAs(first.spec()); - assertThat(second.residual()).isSameAs(first.residual()); + assertThat(tasks) + .extracting(task -> task.file().location()) + .as("relative paths resolve against the table location in both the root and leaf readers") + .allSatisfy(location -> assertThat(location).startsWith(TABLE_LOCATION)) + .containsExactlyInAnyOrder(resolved("root-data.parquet"), resolved("leaf-data.parquet")); } @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void buildingAndClosingWithoutIteratingDoesNotScanLeaves(FileFormat format) throws IOException { + void closingWithoutIteratingOpensOnlyRoot(FileFormat format) throws IOException { InputFile leaf = writeManifest( format, @@ -237,62 +323,71 @@ void buildingAndClosingWithoutIteratingDoesNotScanLeaves(FileFormat format) thro InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); - ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); + RecordingFileIO recordingIO = new RecordingFileIO(fileIO); FilePlanner planner = - new FilePlanner(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) - .tableLocation(TABLE_LOCATION) - .scanMetrics(metrics); + new FilePlanner(recordingIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) + .tableLocation(TABLE_LOCATION); planner.planFiles().close(); - assertThat(metrics.scannedDataManifests().value()) - .as( - "the root is read eagerly but leaf readers open lazily, so closing " - + "without iterating scans only the root, not the leaf") - .isEqualTo(1L); + assertThat(recordingIO.opened(root.location())).as("the root is read eagerly").isTrue(); + assertThat(recordingIO.opened(leaf.location())) + .as("leaf readers open lazily, so closing without iterating leaves them unopened") + .isFalse(); } @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void parallelLeafExpansionYieldsAllFiles(FileFormat format) throws IOException { - TrackedFile withDv = - dataFile( - "leaf1-data.parquet", - partition(1), - deletionVector(DV_LOCATION, DV_OFFSET, DV_SIZE_IN_BYTES, DV_CARDINALITY)); - InputFile leaf1 = writeManifest(format, PARTITION_TYPE, ImmutableList.of(withDv)); + void leafOpenedOnlyWhenPrecedingLeafExhausted(FileFormat format) throws IOException { + InputFile leaf1 = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFile("leaf1-a.parquet", EMPTY_PARTITION_DATA), + dataFile("leaf1-b.parquet", EMPTY_PARTITION_DATA))); InputFile leaf2 = writeManifest( - format, PARTITION_TYPE, ImmutableList.of(dataFile("leaf2-data.parquet", partition(1)))); + format, + EMPTY_PARTITION, + ImmutableList.of(dataFile("leaf2.parquet", EMPTY_PARTITION_DATA))); InputFile root = writeManifest( format, - PARTITION_TYPE, - ImmutableList.of(dataManifest(leaf1.location()), dataManifest(leaf2.location()))); - - ExecutorService pool = ThreadPools.newFixedThreadPool("test-scan-task-planner", 2); - try { - List tasks = - plan( - root, - PARTITIONED_SPECS, - planner -> planner.filterData(Expressions.equal("data", "x")).planWith(pool)); - String residual = Expressions.equal("data", "x").toString(); - assertThat(tasks) - .extracting( - task -> task.file().location(), - task -> task.residual().toString(), - task -> task.deletes().size()) - .containsExactlyInAnyOrder( - tuple(resolved("leaf1-data.parquet"), residual, 1), - tuple(resolved("leaf2-data.parquet"), residual, 0)); - } finally { - pool.shutdownNow(); + EMPTY_PARTITION, + ImmutableList.of(dataManifest(leaf1.location(), 2), dataManifest(leaf2.location()))); + + RecordingFileIO recordingIO = new RecordingFileIO(fileIO); + FilePlanner planner = + new FilePlanner(recordingIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) + .tableLocation(TABLE_LOCATION) + .planWith(null); + + try (CloseableIterable plan = planner.planFiles(); + CloseableIterator tasks = plan.iterator()) { + assertThat(tasks.next().file().location()).isEqualTo(resolved("leaf1-a.parquet")); + assertThat(recordingIO.opened(leaf1.location())) + .as("leaf1 opens to produce the first task") + .isTrue(); + assertThat(recordingIO.opened(leaf2.location())) + .as("leaf2 stays closed until leaf1 is exhausted") + .isFalse(); + + assertThat(tasks.next().file().location()).isEqualTo(resolved("leaf1-b.parquet")); + assertThat(recordingIO.opened(leaf2.location())) + .as("reading the last file in leaf1 does not open leaf2") + .isFalse(); + + assertThat(tasks.next().file().location()).isEqualTo(resolved("leaf2.parquet")); + assertThat(recordingIO.opened(leaf2.location())) + .as("leaf2 opens once leaf1 is drained") + .isTrue(); + assertThat(tasks).isExhausted(); } } @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void deleteContentInRootIsUnsupported(FileFormat format) throws IOException { + void deleteManifestInRootUnsupported(FileFormat format) throws IOException { InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(deleteManifest("deletes.avro"))); @@ -306,145 +401,42 @@ void deleteContentInRootIsUnsupported(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void nonDataEntryInLeafFailsPlanning(FileFormat format) throws IOException { - InputFile leaf = - writeManifest( - format, EMPTY_PARTITION, ImmutableList.of(dataManifest("nested-leaf.parquet"))); - InputFile root = - writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); + void v3ManifestLeafUnsupported(FileFormat format) throws IOException { + String leafLocation = "v3-leaf." + format.name().toLowerCase(Locale.ROOT); + TrackedFile v3Leaf = + new TrackedFileStruct( + addedTracking(), + FileContent.DATA_MANIFEST, + 3, // format version the v4 reader rejects + leafLocation, + FileFormat.fromFileName(leafLocation), + RECORD_COUNT, + FILE_SIZE_IN_BYTES, + null, // specId + null, // partition + null, // contentStats + null, // sortOrderId + null, // deletionVector + dataManifestInfo(1), + null, // keyMetadata + null, // splitOffsets + null); // equalityIds + InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(v3Leaf)); FilePlanner planner = new FilePlanner(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) .tableLocation(TABLE_LOCATION); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Invalid content type for DataFile: DATA_MANIFEST"); - } - - @Test - void emptyRootYieldsNoTasks() throws IOException { - // Avro is used because the Parquet writer does not materialize a file when no records are - // appended. - InputFile root = writeManifest(FileFormat.AVRO, EMPTY_PARTITION, ImmutableList.of()); - - assertThat(plan(root, UNPARTITIONED_SPECS)).isEmpty(); - } - - @ParameterizedTest - @FieldSource("MANIFEST_FORMATS") - void planFilesAcrossMultipleSpecs(FileFormat format) throws IOException { - PartitionSpec spec0 = - PartitionSpec.builderFor(TABLE_SCHEMA) - .withSpecId(0) - .add(1, 1000, "id", Transforms.identity()) - .build(); - PartitionSpec spec1 = - PartitionSpec.builderFor(TABLE_SCHEMA) - .withSpecId(1) - .add(2, 1001, "data", Transforms.identity()) - .build(); - Map specsById = - ImmutableMap.of(spec0.specId(), spec0, spec1.specId(), spec1); - Types.StructType unionType = Partitioning.unionPartitionTypes(specsById.values()); - - TrackedFile spec0Keep = - dataFile("spec0-keep.parquet", spec0.specId(), unionPartition(unionType, 1, null), null); - TrackedFile spec0Prune = - dataFile("spec0-prune.parquet", spec0.specId(), unionPartition(unionType, 2, null), null); - TrackedFile spec1File = - dataFile("spec1.parquet", spec1.specId(), unionPartition(unionType, null, "x"), null); - InputFile root = - writeManifest(format, unionType, ImmutableList.of(spec0Keep, spec0Prune, spec1File)); - - // id = 1 prunes the spec0 file partitioned on id=2; spec1 is not partitioned by id, so its - // residual keeps the predicate. - List tasks = - plan(root, specsById, planner -> planner.filterData(Expressions.equal("id", 1))); - - assertThat(tasks) - .extracting( - task -> task.file().location(), - task -> task.spec().specId(), - task -> task.residual().toString()) - .containsExactlyInAnyOrder( - tuple(resolved("spec0-keep.parquet"), 0, Expressions.alwaysTrue().toString()), - tuple(resolved("spec1.parquet"), 1, Expressions.equal("id", 1).toString())); - - // `data` sits at union position 1 but spec1 position 0; the emitted task must expose the - // partition in its own spec order - FileScanTask spec1Task = - tasks.stream().filter(task -> task.spec().specId() == 1).findFirst().orElseThrow(); - assertThat(spec1Task.file().partition().get(0, CharSequence.class)).hasToString("x"); - } - - @ParameterizedTest - @FieldSource("MANIFEST_FORMATS") - void planFilesReportsScanMetrics(FileFormat format) throws IOException { - TrackedFile fileWithDv = - dataFile( - "with-dv.parquet", - EMPTY_PARTITION_DATA, - deletionVector(DV_LOCATION, DV_OFFSET, DV_SIZE_IN_BYTES, DV_CARDINALITY)); - InputFile root = - writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of(dataFile("plain.parquet", EMPTY_PARTITION_DATA), fileWithDv)); - - ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); - List tasks = - plan(root, UNPARTITIONED_SPECS, planner -> planner.scanMetrics(metrics)); - - assertThat(tasks).hasSize(2); - assertThat(metrics.scannedDataManifests().value()) - .as("the root is scanned; there are no leaves") - .isEqualTo(1L); - assertThat(metrics.resultDataFiles().value()).isEqualTo(2L); - assertThat(metrics.totalFileSizeInBytes().value()).isEqualTo(2 * FILE_SIZE_IN_BYTES); - assertThat(metrics.resultDeleteFiles().value()) - .as("only the file with a colocated DV contributes a delete file") - .isEqualTo(1L); - assertThat(metrics.totalDeleteFileSizeInBytes().value()) - .as("the DV delete contributes its size") - .isEqualTo(DV_SIZE_IN_BYTES); - assertThat(metrics.dvs().value()).isEqualTo(1L); - assertThat(metrics.indexedDeleteFiles().value()) - .as("co-located DVs are not indexed delete files") - .isEqualTo(0L); - } - - @ParameterizedTest - @FieldSource("MANIFEST_FORMATS") - void filterAppliesToLeafManifestFiles(FileFormat format) throws IOException { - InputFile leaf = - writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of( - dataFileWithStats("leaf-keep.parquet", idStats(0, 99)), - dataFileWithStats("leaf-prune.parquet", idStats(100, 199)))); - InputFile root = - writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); - - List tasks = - plan(root, UNPARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("id", 50))); - - assertThat(Iterables.getOnlyElement(tasks).file().location()) - .as("only the leaf file whose stats match the filter survives") - .isEqualTo(resolved("leaf-keep.parquet")); + .hasMessage("Cannot read manifest with format version 3: only 4 is supported"); } @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void prunedLeafManifestIsNotExpanded(FileFormat format) throws IOException { + void scanMetricsForComplexFilteredPlan(FileFormat format) throws IOException { InputFile root = - writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of( - dataManifestWithStats( - "unopened-leaf." + format.name().toLowerCase(Locale.ROOT), idStats(100, 199)), - dataFileWithStats("root-keep.parquet", idStats(0, 99)))); + filterableRoot( + format, deletionVector(DV_LOCATION, DV_OFFSET, DV_SIZE_IN_BYTES, DV_CARDINALITY)); ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); List tasks = @@ -453,14 +445,28 @@ void prunedLeafManifestIsNotExpanded(FileFormat format) throws IOException { UNPARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("id", 50)).scanMetrics(metrics)); - assertThat(Iterables.getOnlyElement(tasks).file().location()) - .isEqualTo(resolved("root-keep.parquet")); + assertThat(tasks) + .extracting(task -> task.file().location()) + .containsExactlyInAnyOrder(resolved("root-keep.parquet"), resolved("leaf-file.parquet")); + assertThat(metrics.resultDataFiles().value()).isEqualTo(2L); + assertThat(metrics.totalFileSizeInBytes().value()).isEqualTo(2 * FILE_SIZE_IN_BYTES); + assertThat(metrics.scannedDataManifests().value()) + .as("the root and the matched leaf are scanned") + .isEqualTo(2L); assertThat(metrics.skippedDataManifests().value()) - .as("the leaf manifest ref is pruned by its stats") + .as("the pruned leaf ref is skipped") .isEqualTo(1L); - assertThat(metrics.scannedDataManifests().value()) - .as("only the root is scanned; the pruned leaf is never opened") + assertThat(metrics.skippedDataFiles().value()) + .as("the pruned root and leaf data files are skipped") + .isEqualTo(2L); + assertThat(metrics.resultDeleteFiles().value()) + .as("only the colocated DV contributes a delete file") .isEqualTo(1L); + assertThat(metrics.totalDeleteFileSizeInBytes().value()).isEqualTo(DV_SIZE_IN_BYTES); + assertThat(metrics.dvs().value()).isEqualTo(1L); + assertThat(metrics.indexedDeleteFiles().value()) + .as("colocated DVs are not indexed delete files") + .isEqualTo(0L); } private List plan(InputFile root, Map specsById) @@ -499,6 +505,11 @@ private static TrackedFile dataFile( } private static TrackedFile dataFileWithStats(String location, ContentStats stats) { + return dataFileWithStats(location, stats, null); + } + + private static TrackedFile dataFileWithStats( + String location, ContentStats stats, DeletionVector dv) { return trackedFile( addedTracking(), FileContent.DATA, @@ -506,13 +517,23 @@ private static TrackedFile dataFileWithStats(String location, ContentStats stats specId(EMPTY_PARTITION_DATA), EMPTY_PARTITION_DATA, stats, - null, // dv + dv, null); // manifestInfo } private static TrackedFile dataManifest(String location) { + return dataManifest(location, 1); + } + + private static TrackedFile dataManifest(String location, int addedFiles) { return trackedFile( - addedTracking(), FileContent.DATA_MANIFEST, location, null, null, null, dataManifestInfo()); + addedTracking(), + FileContent.DATA_MANIFEST, + location, + null, // specId + null, // partition + null, // dv + dataManifestInfo(addedFiles)); } private static TrackedFile dataManifestWithStats(String location, ContentStats stats) { @@ -524,7 +545,7 @@ private static TrackedFile dataManifestWithStats(String location, ContentStats s null, // partition stats, null, // dv - dataManifestInfo()); + dataManifestInfo(1)); } private static ContentStats idStats(int lower, int upper) { @@ -543,16 +564,18 @@ private static ContentStats idStats(int lower, int upper) { return stats; } - private static ManifestInfo dataManifestInfo() { + private static ManifestInfo dataManifestInfo(int addedFiles) { return ManifestInfoStruct.builder() - .addedFilesCount(1) + .addedFilesCount(addedFiles) .existingFilesCount(0) .deletedFilesCount(0) .replacedFilesCount(0) - .addedRowsCount(RECORD_COUNT) + .modifiedFilesCount(0) + .addedRowsCount(addedFiles * RECORD_COUNT) .existingRowsCount(0) .deletedRowsCount(0) .replacedRowsCount(0) + .modifiedRowsCount(0) .minSequenceNumber(0L) .build(); } @@ -618,24 +641,16 @@ private static Integer specId(PartitionData partition) { return unpartitioned ? PartitionSpec.unpartitioned().specId() : SPEC.specId(); } - private static PartitionData partition(int id) { - PartitionData partition = new PartitionData(PARTITION_TYPE); + private static PartitionData idPartition(Types.StructType unionType, Integer id) { + PartitionData partition = new PartitionData(unionType); partition.set(0, id); return partition; } - // the location a relative fixture resolves to once read against TABLE_LOCATION private static String resolved(String location) { return LocationUtil.resolveLocation(TABLE_LOCATION, location); } - private static PartitionData unionPartition(Types.StructType unionType, Integer id, String data) { - PartitionData partition = new PartitionData(unionType); - partition.set(0, id); - partition.set(1, data); - return partition; - } - private static DeletionVector deletionVector( String location, long offset, long sizeInBytes, long cardinality) { return DeletionVectorStruct.builder() @@ -666,4 +681,100 @@ private InputFile writeManifest( return out.toInputFile(); } + + private InputFile filterableRoot(FileFormat format) throws IOException { + return filterableRoot(format, null); + } + + private InputFile filterableRoot(FileFormat format, DeletionVector dv) throws IOException { + InputFile matchedLeaf = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFileWithStats("leaf-file.parquet", idStats(0, 99)), + dataFileWithStats("leaf-prune.parquet", idStats(100, 199)))); + return writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFileWithStats("root-keep.parquet", idStats(0, 99), dv), + dataFileWithStats("root-prune.parquet", idStats(100, 199)), + dataManifestWithStats(matchedLeaf.location(), idStats(0, 199)), + dataManifestWithStats( + "pruned-leaf." + format.name().toLowerCase(Locale.ROOT), idStats(100, 199)))); + } + + private InputFile mixedPartitionedAndUnpartitionedRoot(FileFormat format) throws IOException { + Types.StructType unionType = Partitioning.unionPartitionTypes(MIXED_SPECS.values()); + return writeManifest( + format, + unionType, + ImmutableList.of( + dataFile("partitioned.parquet", BY_ID_SPEC.specId(), idPartition(unionType, 1), null), + dataFile( + "unpartitioned.parquet", + UNPARTITIONED_WITH_SCHEMA.specId(), + idPartition(unionType, null), + null))); + } + + private static class RecordingFileIO implements FileIO { + private final FileIO delegate; + private final Set openedPaths = Sets.newHashSet(); + + private RecordingFileIO(FileIO delegate) { + this.delegate = delegate; + } + + private boolean opened(String location) { + return openedPaths.contains(location); + } + + @Override + public InputFile newInputFile(String path) { + return new RecordingInputFile(delegate.newInputFile(path), openedPaths); + } + + @Override + public OutputFile newOutputFile(String path) { + return delegate.newOutputFile(path); + } + + @Override + public void deleteFile(String path) { + delegate.deleteFile(path); + } + } + + private static class RecordingInputFile implements InputFile { + private final InputFile delegate; + private final Set openedPaths; + + private RecordingInputFile(InputFile delegate, Set openedPaths) { + this.delegate = delegate; + this.openedPaths = openedPaths; + } + + @Override + public long getLength() { + return delegate.getLength(); + } + + @Override + public SeekableInputStream newStream() { + openedPaths.add(delegate.location()); + return delegate.newStream(); + } + + @Override + public String location() { + return delegate.location(); + } + + @Override + public boolean exists() { + return delegate.exists(); + } + } } From f18f27646e9001b0d7f80659ee42a47cf24d6e6c Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Mon, 5 Oct 2026 17:34:14 -0700 Subject: [PATCH 17/21] incorporate feedback --- .../org/apache/iceberg/RootManifestFile.java | 28 +++++++------ .../org/apache/iceberg/TestFilePlanner.java | 30 ++++++------- .../apache/iceberg/TestRootManifestFile.java | 42 +++++++------------ 3 files changed, 42 insertions(+), 58 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/RootManifestFile.java b/core/src/main/java/org/apache/iceberg/RootManifestFile.java index 4a4df8c554de..5c7bf8bbc9b6 100644 --- a/core/src/main/java/org/apache/iceberg/RootManifestFile.java +++ b/core/src/main/java/org/apache/iceberg/RootManifestFile.java @@ -19,8 +19,8 @@ package org.apache.iceberg; import java.nio.ByteBuffer; +import java.util.Arrays; import java.util.List; -import org.apache.iceberg.io.InputFile; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.util.ByteBuffers; @@ -28,21 +28,23 @@ class RootManifestFile implements ManifestFile { private static final int FORMAT_VERSION = 4; - private final InputFile file; + private final String location; + private final long length; private final long snapshotId; private final long sequenceNumber; private final Long firstRowId; private final byte[] keyMetadata; - private Long length = null; RootManifestFile( - InputFile file, + String location, + long length, long snapshotId, long sequenceNumber, Long firstRowId, ByteBuffer keyMetadata) { - Preconditions.checkArgument(file != null, "Invalid file: null"); - this.file = file; + Preconditions.checkArgument(location != null, "Invalid location: null"); + this.location = location; + this.length = length; this.snapshotId = snapshotId; this.sequenceNumber = sequenceNumber; this.firstRowId = firstRowId; @@ -51,15 +53,11 @@ class RootManifestFile implements ManifestFile { @Override public String path() { - return file.location(); + return location; } @Override public long length() { - if (length == null) { - this.length = file.getLength(); - } - return length; } @@ -140,7 +138,11 @@ public int formatVersion() { @Override public ManifestFile copy() { - ByteBuffer keyMetadataCopy = keyMetadata == null ? null : ByteBuffer.wrap(keyMetadata.clone()); - return new RootManifestFile(file, snapshotId, sequenceNumber, firstRowId, keyMetadataCopy); + ByteBuffer keyMetadataCopy = + keyMetadata == null + ? null + : ByteBuffer.wrap(Arrays.copyOf(keyMetadata, keyMetadata.length)); + return new RootManifestFile( + location, length, snapshotId, sequenceNumber, firstRowId, keyMetadataCopy); } } diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index cf8510d2ab43..8e22fb3e3d38 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -60,10 +60,8 @@ class TestFilePlanner { private static final long RECORD_COUNT = 100L; private static final long FILE_SIZE_IN_BYTES = 1024L; private static final String TABLE_LOCATION = "s3://bucket/db/table"; - private static final String DV_LOCATION = "s3://bucket/db/table/dv.puffin"; - private static final long DV_OFFSET = 100L; - private static final long DV_SIZE_IN_BYTES = 50L; - private static final long DV_CARDINALITY = 5L; + private static final DeletionVector DV = + deletionVector("s3://bucket/db/table/dv.puffin", 100L, 50L, 5L); private static final Schema TABLE_SCHEMA = new Schema( @@ -112,7 +110,6 @@ void rootWithOnlyDataFiles(FileFormat format) throws IOException { .hasSize(2) .extracting(task -> task.file().location()) .containsExactlyInAnyOrder(resolved("a.parquet"), resolved("b.parquet")); - assertThat(tasks).allSatisfy(task -> assertThat(task.deletes()).isEmpty()); } @ParameterizedTest @@ -239,12 +236,8 @@ void filterHonorsCaseSensitivity(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") - void colocatedDvAttachedAsPositionDeleteFile(FileFormat format) throws IOException { - TrackedFile fileWithDv = - dataFile( - "with-dv.parquet", - EMPTY_PARTITION_DATA, - deletionVector(DV_LOCATION, DV_OFFSET, DV_SIZE_IN_BYTES, DV_CARDINALITY)); + void taskAttachesColocatedDV(FileFormat format) throws IOException { + TrackedFile fileWithDv = dataFile("with-dv.parquet", EMPTY_PARTITION_DATA, DV); InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(fileWithDv)); List tasks = plan(root, UNPARTITIONED_SPECS); @@ -252,7 +245,7 @@ void colocatedDvAttachedAsPositionDeleteFile(FileFormat format) throws IOExcepti assertThat(tasks).hasSize(1); assertThat(tasks.get(0).deletes()) .hasSize(1) - .allSatisfy(delete -> assertThat(delete.location()).isEqualTo(DV_LOCATION)); + .allSatisfy(delete -> assertThat(delete.location()).isEqualTo(DV.location())); } @ParameterizedTest @@ -434,9 +427,7 @@ void v3ManifestLeafUnsupported(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void scanMetricsForComplexFilteredPlan(FileFormat format) throws IOException { - InputFile root = - filterableRoot( - format, deletionVector(DV_LOCATION, DV_OFFSET, DV_SIZE_IN_BYTES, DV_CARDINALITY)); + InputFile root = filterableRoot(format, DV); ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); List tasks = @@ -462,7 +453,7 @@ void scanMetricsForComplexFilteredPlan(FileFormat format) throws IOException { assertThat(metrics.resultDeleteFiles().value()) .as("only the colocated DV contributes a delete file") .isEqualTo(1L); - assertThat(metrics.totalDeleteFileSizeInBytes().value()).isEqualTo(DV_SIZE_IN_BYTES); + assertThat(metrics.totalDeleteFileSizeInBytes().value()).isEqualTo(DV.sizeInBytes()); assertThat(metrics.dvs().value()).isEqualTo(1L); assertThat(metrics.indexedDeleteFiles().value()) .as("colocated DVs are not indexed delete files") @@ -488,7 +479,12 @@ private List plan( private static ManifestFile asManifest(InputFile file) { return new RootManifestFile( - file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, /* keyMetadata= */ null); + file.location(), + file.getLength(), + SNAPSHOT_ID, + SEQUENCE_NUMBER, + FIRST_ROW_ID, + /* keyMetadata= */ null); } private static TrackedFile dataFile(String location, PartitionData partition) { diff --git a/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java b/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java index 68664d085053..107383fc9a04 100644 --- a/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java +++ b/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java @@ -22,64 +22,50 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.nio.ByteBuffer; -import org.apache.iceberg.inmemory.InMemoryInputFile; -import org.apache.iceberg.io.InputFile; import org.junit.jupiter.api.Test; class TestRootManifestFile { private static final long SNAPSHOT_ID = 42L; private static final long SEQUENCE_NUMBER = 7L; private static final long FIRST_ROW_ID = 1000L; - private static final byte[] CONTENTS = new byte[] {1, 2, 3, 4, 5}; + private static final long LENGTH = 1024L; private static final String LOCATION = "s3://bucket/db/table/metadata/root.parquet"; - private final InputFile file = new InMemoryInputFile(LOCATION, CONTENTS); - @Test - void exposesFileMetadata() { + void fieldAccess() { + ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {9, 8, 7}); RootManifestFile root = new RootManifestFile( - file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, /* keyMetadata= */ null); + LOCATION, LENGTH, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, keyMetadata); assertThat(root.path()).isEqualTo(LOCATION); - assertThat(root.length()).isEqualTo(CONTENTS.length); + assertThat(root.length()).isEqualTo(LENGTH); assertThat(root.snapshotId()).isEqualTo(SNAPSHOT_ID); assertThat(root.sequenceNumber()).isEqualTo(SEQUENCE_NUMBER); assertThat(root.firstRowId()).isEqualTo(FIRST_ROW_ID); + assertThat(root.keyMetadata()).isEqualTo(keyMetadata); } @Test - void readableAsAV4DataManifest() { + void defaultedFields() { RootManifestFile root = - new RootManifestFile(file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, null); + new RootManifestFile(LOCATION, LENGTH, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, null); assertThat(root.content()).isEqualTo(ManifestContent.DATA); assertThat(root.formatVersion()).isEqualTo(4); + assertThat(root.keyMetadata()).isNull(); } @Test - void keyMetadataRoundTrips() { - assertThat( - new RootManifestFile(file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, null) - .keyMetadata()) - .isNull(); - - ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {9, 8, 7}); - assertThat( - new RootManifestFile(file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, keyMetadata) - .keyMetadata()) - .isEqualTo(keyMetadata); - } - - @Test - void copyPreservesValues() { + void copy() { ByteBuffer keyMetadata = ByteBuffer.wrap(new byte[] {9, 8, 7}); RootManifestFile original = - new RootManifestFile(file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, keyMetadata); + new RootManifestFile( + LOCATION, LENGTH, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, keyMetadata); ManifestFile copy = original.copy(); assertThat(copy.path()).isEqualTo(LOCATION); - assertThat(copy.length()).isEqualTo(CONTENTS.length); + assertThat(copy.length()).isEqualTo(LENGTH); assertThat(copy.snapshotId()).isEqualTo(SNAPSHOT_ID); assertThat(copy.sequenceNumber()).isEqualTo(SEQUENCE_NUMBER); assertThat(copy.firstRowId()).isEqualTo(FIRST_ROW_ID); @@ -92,7 +78,7 @@ void copyPreservesValues() { @Test void unsupportedAccessorsThrow() { RootManifestFile root = - new RootManifestFile(file, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, null); + new RootManifestFile(LOCATION, LENGTH, SNAPSHOT_ID, SEQUENCE_NUMBER, FIRST_ROW_ID, null); assertThatThrownBy(root::partitionSpecId) .isInstanceOf(UnsupportedOperationException.class) From cbda9d07192e0c9550227068b7c282bc52197352 Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Thu, 8 Oct 2026 10:51:54 -0700 Subject: [PATCH 18/21] Core: extend V4TestHelpers for the file planner tests Add a complete data-file factory (explicit specId, partition, stats, and deletion vector) and an OutputFile overload of writeTrackedFiles so the caller controls the manifest location. The file planner resolves specId and relativizes leaf references against the table location, so it needs both; the manifest reader did not. --- .../org/apache/iceberg/V4TestHelpers.java | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/org/apache/iceberg/V4TestHelpers.java b/core/src/test/java/org/apache/iceberg/V4TestHelpers.java index 1b922b576ca8..6d79bcef377a 100644 --- a/core/src/test/java/org/apache/iceberg/V4TestHelpers.java +++ b/core/src/test/java/org/apache/iceberg/V4TestHelpers.java @@ -43,6 +43,15 @@ static TrackedFile dataFile(String location, Integer specId, PartitionData parti return dataFile(ADDED_TRACKING, location, specId, partition, null, null); } + static TrackedFile dataFile( + String location, + Integer specId, + PartitionData partition, + ContentStats stats, + DeletionVector dv) { + return dataFile(ADDED_TRACKING, location, specId, partition, stats, dv); + } + static TrackedFile dataFileWithStats(String location, ContentStats stats) { return dataFile(ADDED_TRACKING, location, null, null, stats, null); } @@ -165,8 +174,18 @@ static OutputFile writeTrackedFiles( Types.StructType statsType, Iterable files) throws IOException { - Schema writeSchema = TrackedFile.schema(partitionType, statsType); OutputFile out = io.newOutputFile(format.addExtension("manifest." + System.nanoTime())); + return writeTrackedFiles(out, format, partitionType, statsType, files); + } + + static OutputFile writeTrackedFiles( + OutputFile out, + FileFormat format, + Types.StructType partitionType, + Types.StructType statsType, + Iterable files) + throws IOException { + Schema writeSchema = TrackedFile.schema(partitionType, statsType); try (FileAppender appender = InternalData.write(format, out).schema(writeSchema).named("tracked_file").build()) { for (TrackedFile file : files) { From 012992af0f75b43018b43cd5c7474f425dd08d2e Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Thu, 8 Oct 2026 10:51:55 -0700 Subject: [PATCH 19/21] Core: migrate TestFilePlanner to the shared V4TestHelpers Build the data files, manifest references, deletion vectors, and manifests through V4TestHelpers instead of local copies, removing the duplicated tracked-file construction. Fixtures are built inline with their id-range stats visible at each test so the filter keep/skip outcomes are obvious. --- .../org/apache/iceberg/TestFilePlanner.java | 294 ++++++------------ 1 file changed, 103 insertions(+), 191 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index 8e22fb3e3d38..caafe6751129 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -35,7 +35,6 @@ import org.apache.iceberg.inmemory.InMemoryFileIO; import org.apache.iceberg.io.CloseableIterable; import org.apache.iceberg.io.CloseableIterator; -import org.apache.iceberg.io.FileAppender; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.io.InputFile; import org.apache.iceberg.io.OutputFile; @@ -48,7 +47,6 @@ import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Sets; import org.apache.iceberg.types.Types; -import org.apache.iceberg.util.LocationUtil; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.FieldSource; @@ -56,12 +54,18 @@ class TestFilePlanner { private static final long SNAPSHOT_ID = 42L; private static final long SEQUENCE_NUMBER = 7L; private static final long FIRST_ROW_ID = 1000L; - private static final int WRITER_FORMAT_VERSION = 4; private static final long RECORD_COUNT = 100L; private static final long FILE_SIZE_IN_BYTES = 1024L; private static final String TABLE_LOCATION = "s3://bucket/db/table"; private static final DeletionVector DV = - deletionVector("s3://bucket/db/table/dv.puffin", 100L, 50L, 5L); + V4TestHelpers.deletionVector("s3://bucket/db/table/dv.puffin", 100L, 50L, 5L); + + private static final String FILE_A = "s3://bucket/db/table/file-a.parquet"; + private static final String FILE_B = "s3://bucket/db/table/file-b.parquet"; + private static final String FILE_C = "s3://bucket/db/table/file-c.parquet"; + private static final String FILE_D = "s3://bucket/db/table/file-d.parquet"; + private static final String PARTITIONED_FILE = "s3://bucket/db/table/partitioned.parquet"; + private static final String UNPARTITIONED_FILE = "s3://bucket/db/table/unpartitioned.parquet"; private static final Schema TABLE_SCHEMA = new Schema( @@ -101,15 +105,14 @@ void rootWithOnlyDataFiles(FileFormat format) throws IOException { format, EMPTY_PARTITION, ImmutableList.of( - dataFile("a.parquet", EMPTY_PARTITION_DATA), - dataFile("b.parquet", EMPTY_PARTITION_DATA))); + dataFile(FILE_A, EMPTY_PARTITION_DATA), dataFile(FILE_B, EMPTY_PARTITION_DATA))); List tasks = plan(root, UNPARTITIONED_SPECS); assertThat(tasks) .hasSize(2) .extracting(task -> task.file().location()) - .containsExactlyInAnyOrder(resolved("a.parquet"), resolved("b.parquet")); + .containsExactlyInAnyOrder(FILE_A, FILE_B); } @ParameterizedTest @@ -117,14 +120,10 @@ void rootWithOnlyDataFiles(FileFormat format) throws IOException { void rootWithOnlyLeafManifests(FileFormat format) throws IOException { InputFile leafA = writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of(dataFile("leaf-a.parquet", EMPTY_PARTITION_DATA))); + format, EMPTY_PARTITION, ImmutableList.of(dataFile(FILE_A, EMPTY_PARTITION_DATA))); InputFile leafB = writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of(dataFile("leaf-b.parquet", EMPTY_PARTITION_DATA))); + format, EMPTY_PARTITION, ImmutableList.of(dataFile(FILE_B, EMPTY_PARTITION_DATA))); InputFile root = writeManifest( format, @@ -136,7 +135,7 @@ void rootWithOnlyLeafManifests(FileFormat format) throws IOException { assertThat(tasks) .hasSize(2) .extracting(task -> task.file().location()) - .containsExactlyInAnyOrder(resolved("leaf-a.parquet"), resolved("leaf-b.parquet")); + .containsExactlyInAnyOrder(FILE_A, FILE_B); } @ParameterizedTest @@ -144,20 +143,16 @@ void rootWithOnlyLeafManifests(FileFormat format) throws IOException { void rootWithMixedDataAndLeafManifests(FileFormat format) throws IOException { InputFile leaf1 = writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of(dataFile("leaf-file-1.parquet", EMPTY_PARTITION_DATA))); + format, EMPTY_PARTITION, ImmutableList.of(dataFile(FILE_C, EMPTY_PARTITION_DATA))); InputFile leaf2 = writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of(dataFile("leaf-file-2.parquet", EMPTY_PARTITION_DATA))); + format, EMPTY_PARTITION, ImmutableList.of(dataFile(FILE_D, EMPTY_PARTITION_DATA))); InputFile root = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( - dataFile("root-data-file.parquet", EMPTY_PARTITION_DATA), + dataFile(FILE_A, EMPTY_PARTITION_DATA), dataManifest(leaf1.location()), dataManifest(leaf2.location()))); @@ -166,16 +161,29 @@ void rootWithMixedDataAndLeafManifests(FileFormat format) throws IOException { assertThat(tasks) .hasSize(3) .extracting(task -> task.file().location()) - .containsExactlyInAnyOrder( - resolved("root-data-file.parquet"), - resolved("leaf-file-1.parquet"), - resolved("leaf-file-2.parquet")); + .containsExactlyInAnyOrder(FILE_A, FILE_C, FILE_D); } @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void filterSkipsAndMatchesRootDataFilesAndManifests(FileFormat format) throws IOException { - InputFile root = filterableRoot(format); + InputFile matchedLeaf = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFileWithStats(FILE_C, idStats(0, 99)), + dataFileWithStats(FILE_D, idStats(100, 199)))); + InputFile root = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFileWithStats(FILE_A, idStats(0, 99)), + dataFileWithStats(FILE_B, idStats(100, 199)), + dataManifestWithStats(matchedLeaf.location(), idStats(0, 199)), + dataManifestWithStats( + "s3://bucket/db/table/pruned-leaf.parquet", idStats(100, 199)))); List tasks = plan(root, UNPARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("id", 50))); @@ -183,8 +191,8 @@ void filterSkipsAndMatchesRootDataFilesAndManifests(FileFormat format) throws IO assertThat(tasks) .extracting(task -> task.file().location()) .as( - "matched root data file and the file in the matched leaf survive; pruned data file and leaf do not") - .containsExactlyInAnyOrder(resolved("root-keep.parquet"), resolved("leaf-file.parquet")); + "the matched root data file and the file in the matched leaf survive; the pruned data file and leaf do not") + .containsExactlyInAnyOrder(FILE_A, FILE_C); } @ParameterizedTest @@ -195,8 +203,8 @@ void filterSkipsAndMatchesLeafDataFiles(FileFormat format) throws IOException { format, EMPTY_PARTITION, ImmutableList.of( - dataFileWithStats("leaf-keep.parquet", idStats(0, 99)), - dataFileWithStats("leaf-prune.parquet", idStats(100, 199)))); + dataFileWithStats(FILE_C, idStats(0, 99)), + dataFileWithStats(FILE_D, idStats(100, 199)))); InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location(), 2))); @@ -205,13 +213,19 @@ void filterSkipsAndMatchesLeafDataFiles(FileFormat format) throws IOException { assertThat(Iterables.getOnlyElement(tasks).file().location()) .as("only the leaf file whose stats match the filter survives") - .isEqualTo(resolved("leaf-keep.parquet")); + .isEqualTo(FILE_C); } @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void filterHonorsCaseSensitivity(FileFormat format) throws IOException { - InputFile root = filterableRoot(format); + InputFile root = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFileWithStats(FILE_A, idStats(0, 99)), + dataFileWithStats(FILE_B, idStats(100, 199)))); List tasks = plan( @@ -220,8 +234,8 @@ void filterHonorsCaseSensitivity(FileFormat format) throws IOException { planner -> planner.filterData(Expressions.equal("ID", 50)).caseSensitive(false)); assertThat(tasks) .extracting(task -> task.file().location()) - .as("a case-insensitive filter resolves \"ID\" to \"id\" everywhere the filter is applied") - .containsExactlyInAnyOrder(resolved("root-keep.parquet"), resolved("leaf-file.parquet")); + .as("a case-insensitive filter resolves \"ID\" to \"id\" so the matching file survives") + .containsExactlyInAnyOrder(FILE_A); assertThatThrownBy( () -> @@ -237,8 +251,9 @@ void filterHonorsCaseSensitivity(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void taskAttachesColocatedDV(FileFormat format) throws IOException { - TrackedFile fileWithDv = dataFile("with-dv.parquet", EMPTY_PARTITION_DATA, DV); - InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(fileWithDv)); + TrackedFile fileWithDV = + dataFile("s3://bucket/db/table/with-dv.parquet", EMPTY_PARTITION_DATA, DV); + InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(fileWithDV)); List tasks = plan(root, UNPARTITIONED_SPECS); @@ -261,8 +276,8 @@ void residualForMixedPartitionedAndUnpartitioned(FileFormat format) throws IOExc .as( "the id predicate drops out of the partitioned file's residual but not the unpartitioned") .containsExactlyInAnyOrder( - tuple(resolved("partitioned.parquet"), Expressions.equal("data", "x").toString()), - tuple(resolved("unpartitioned.parquet"), filter.toString())); + tuple(PARTITIONED_FILE, Expressions.equal("data", "x").toString()), + tuple(UNPARTITIONED_FILE, filter.toString())); } @ParameterizedTest @@ -283,6 +298,8 @@ void ignoreResidualsProducesAlwaysTrueResidual(FileFormat format) throws IOExcep @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void tableLocationForwardedToRootAndLeafReaders(FileFormat format) throws IOException { + // the only test that uses relative paths: it verifies the reader relativizes them against the + // table location in both the root and the lazily-opened leaf InputFile leaf = writeManifest( format, @@ -301,8 +318,8 @@ void tableLocationForwardedToRootAndLeafReaders(FileFormat format) throws IOExce assertThat(tasks) .extracting(task -> task.file().location()) .as("relative paths resolve against the table location in both the root and leaf readers") - .allSatisfy(location -> assertThat(location).startsWith(TABLE_LOCATION)) - .containsExactlyInAnyOrder(resolved("root-data.parquet"), resolved("leaf-data.parquet")); + .containsExactlyInAnyOrder( + "s3://bucket/db/table/root-data.parquet", "s3://bucket/db/table/leaf-data.parquet"); } @ParameterizedTest @@ -310,9 +327,7 @@ void tableLocationForwardedToRootAndLeafReaders(FileFormat format) throws IOExce void closingWithoutIteratingOpensOnlyRoot(FileFormat format) throws IOException { InputFile leaf = writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of(dataFile("leaf.parquet", EMPTY_PARTITION_DATA))); + format, EMPTY_PARTITION, ImmutableList.of(dataFile(FILE_A, EMPTY_PARTITION_DATA))); InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); @@ -336,13 +351,10 @@ void leafOpenedOnlyWhenPrecedingLeafExhausted(FileFormat format) throws IOExcept format, EMPTY_PARTITION, ImmutableList.of( - dataFile("leaf1-a.parquet", EMPTY_PARTITION_DATA), - dataFile("leaf1-b.parquet", EMPTY_PARTITION_DATA))); + dataFile(FILE_A, EMPTY_PARTITION_DATA), dataFile(FILE_B, EMPTY_PARTITION_DATA))); InputFile leaf2 = writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of(dataFile("leaf2.parquet", EMPTY_PARTITION_DATA))); + format, EMPTY_PARTITION, ImmutableList.of(dataFile(FILE_C, EMPTY_PARTITION_DATA))); InputFile root = writeManifest( format, @@ -357,7 +369,7 @@ void leafOpenedOnlyWhenPrecedingLeafExhausted(FileFormat format) throws IOExcept try (CloseableIterable plan = planner.planFiles(); CloseableIterator tasks = plan.iterator()) { - assertThat(tasks.next().file().location()).isEqualTo(resolved("leaf1-a.parquet")); + assertThat(tasks.next().file().location()).isEqualTo(FILE_A); assertThat(recordingIO.opened(leaf1.location())) .as("leaf1 opens to produce the first task") .isTrue(); @@ -365,12 +377,12 @@ void leafOpenedOnlyWhenPrecedingLeafExhausted(FileFormat format) throws IOExcept .as("leaf2 stays closed until leaf1 is exhausted") .isFalse(); - assertThat(tasks.next().file().location()).isEqualTo(resolved("leaf1-b.parquet")); + assertThat(tasks.next().file().location()).isEqualTo(FILE_B); assertThat(recordingIO.opened(leaf2.location())) .as("reading the last file in leaf1 does not open leaf2") .isFalse(); - assertThat(tasks.next().file().location()).isEqualTo(resolved("leaf2.parquet")); + assertThat(tasks.next().file().location()).isEqualTo(FILE_C); assertThat(recordingIO.opened(leaf2.location())) .as("leaf2 opens once leaf1 is drained") .isTrue(); @@ -382,7 +394,10 @@ void leafOpenedOnlyWhenPrecedingLeafExhausted(FileFormat format) throws IOExcept @FieldSource("MANIFEST_FORMATS") void deleteManifestInRootUnsupported(FileFormat format) throws IOException { InputFile root = - writeManifest(format, EMPTY_PARTITION, ImmutableList.of(deleteManifest("deletes.avro"))); + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of(deleteManifest("s3://bucket/db/table/deletes.avro"))); FilePlanner planner = new FilePlanner(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) @@ -395,10 +410,10 @@ void deleteManifestInRootUnsupported(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void v3ManifestLeafUnsupported(FileFormat format) throws IOException { - String leafLocation = "v3-leaf." + format.name().toLowerCase(Locale.ROOT); + String leafLocation = "s3://bucket/db/table/v3-leaf." + format.name().toLowerCase(Locale.ROOT); TrackedFile v3Leaf = new TrackedFileStruct( - addedTracking(), + V4TestHelpers.ADDED_TRACKING, FileContent.DATA_MANIFEST, 3, // format version the v4 reader rejects leafLocation, @@ -427,7 +442,23 @@ void v3ManifestLeafUnsupported(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void scanMetricsForComplexFilteredPlan(FileFormat format) throws IOException { - InputFile root = filterableRoot(format, DV); + InputFile matchedLeaf = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFileWithStats(FILE_C, idStats(0, 99)), + dataFileWithStats(FILE_D, idStats(100, 199)))); + InputFile root = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFileWithStats(FILE_A, idStats(0, 99), DV), + dataFileWithStats(FILE_B, idStats(100, 199)), + dataManifestWithStats(matchedLeaf.location(), idStats(0, 199)), + dataManifestWithStats( + "s3://bucket/db/table/pruned-leaf.parquet", idStats(100, 199)))); ScanMetrics metrics = ScanMetrics.of(new DefaultMetricsContext()); List tasks = @@ -438,7 +469,7 @@ void scanMetricsForComplexFilteredPlan(FileFormat format) throws IOException { assertThat(tasks) .extracting(task -> task.file().location()) - .containsExactlyInAnyOrder(resolved("root-keep.parquet"), resolved("leaf-file.parquet")); + .containsExactlyInAnyOrder(FILE_A, FILE_C); assertThat(metrics.resultDataFiles().value()).isEqualTo(2L); assertThat(metrics.totalFileSizeInBytes().value()).isEqualTo(2 * FILE_SIZE_IN_BYTES); assertThat(metrics.scannedDataManifests().value()) @@ -488,16 +519,11 @@ private static ManifestFile asManifest(InputFile file) { } private static TrackedFile dataFile(String location, PartitionData partition) { - return dataFile(location, specId(partition), partition, null); + return V4TestHelpers.dataFile(location, specId(partition), partition, null, null); } private static TrackedFile dataFile(String location, PartitionData partition, DeletionVector dv) { - return dataFile(location, specId(partition), partition, dv); - } - - private static TrackedFile dataFile( - String location, Integer specId, PartitionData partition, DeletionVector dv) { - return trackedFile(addedTracking(), FileContent.DATA, location, specId, partition, dv, null); + return V4TestHelpers.dataFile(location, specId(partition), partition, null, dv); } private static TrackedFile dataFileWithStats(String location, ContentStats stats) { @@ -506,15 +532,8 @@ private static TrackedFile dataFileWithStats(String location, ContentStats stats private static TrackedFile dataFileWithStats( String location, ContentStats stats, DeletionVector dv) { - return trackedFile( - addedTracking(), - FileContent.DATA, - location, - specId(EMPTY_PARTITION_DATA), - EMPTY_PARTITION_DATA, - stats, - dv, - null); // manifestInfo + return V4TestHelpers.dataFile( + location, specId(EMPTY_PARTITION_DATA), EMPTY_PARTITION_DATA, stats, dv); } private static TrackedFile dataManifest(String location) { @@ -522,26 +541,13 @@ private static TrackedFile dataManifest(String location) { } private static TrackedFile dataManifest(String location, int addedFiles) { - return trackedFile( - addedTracking(), - FileContent.DATA_MANIFEST, - location, - null, // specId - null, // partition - null, // dv - dataManifestInfo(addedFiles)); + return V4TestHelpers.manifestRef( + FileContent.DATA_MANIFEST, location, dataManifestInfo(addedFiles)); } private static TrackedFile dataManifestWithStats(String location, ContentStats stats) { - return trackedFile( - addedTracking(), - FileContent.DATA_MANIFEST, - location, - null, // specId - null, // partition - stats, - null, // dv - dataManifestInfo(1)); + return V4TestHelpers.manifestRefWithStats( + FileContent.DATA_MANIFEST, location, stats, dataManifestInfo(1)); } private static ContentStats idStats(int lower, int upper) { @@ -577,59 +583,7 @@ private static ManifestInfo dataManifestInfo(int addedFiles) { } private static TrackedFile deleteManifest(String location) { - return trackedFile( - addedTracking(), FileContent.DELETE_MANIFEST, location, null, null, null, null); - } - - private static TrackedFile trackedFile( - TrackingStruct tracking, - FileContent contentType, - String location, - Integer specId, - PartitionData partition, - DeletionVector dv, - ManifestInfo manifestInfo) { - return trackedFile(tracking, contentType, location, specId, partition, null, dv, manifestInfo); - } - - private static TrackedFile trackedFile( - TrackingStruct tracking, - FileContent contentType, - String location, - Integer specId, - PartitionData partition, - ContentStats contentStats, - DeletionVector dv, - ManifestInfo manifestInfo) { - return new TrackedFileStruct( - tracking, - contentType, - WRITER_FORMAT_VERSION, - location, - FileFormat.fromFileName(location), - RECORD_COUNT, - FILE_SIZE_IN_BYTES, - specId, - partition, - contentStats, - null, // sortOrderId - dv, // deletionVector - manifestInfo, - null, // keyMetadata - null, // splitOffsets - null); // equalityIds - } - - private static TrackingStruct addedTracking() { - return new TrackingStruct( - EntryStatus.ADDED, - SNAPSHOT_ID, - null, // dataSequenceNumber - null, // fileSequenceNumber - null, // dvSnapshotId - null, // firstRowId - null, // deletedPositions - null); // replacedPositions + return V4TestHelpers.manifestRef(FileContent.DELETE_MANIFEST, location, null); } private static Integer specId(PartitionData partition) { @@ -643,24 +597,9 @@ private static PartitionData idPartition(Types.StructType unionType, Integer id) return partition; } - private static String resolved(String location) { - return LocationUtil.resolveLocation(TABLE_LOCATION, location); - } - - private static DeletionVector deletionVector( - String location, long offset, long sizeInBytes, long cardinality) { - return DeletionVectorStruct.builder() - .location(location) - .offset(offset) - .sizeInBytes(sizeInBytes) - .cardinality(cardinality) - .build(); - } - private InputFile writeManifest( FileFormat format, Types.StructType partitionType, Iterable files) throws IOException { - Schema writeSchema = TrackedFile.schema(partitionType, STATS_TYPE); OutputFile out = fileIO.newOutputFile( TABLE_LOCATION @@ -668,37 +607,8 @@ private InputFile writeManifest( + System.nanoTime() + "." + format.name().toLowerCase(Locale.ROOT)); - try (FileAppender appender = - InternalData.write(format, out).schema(writeSchema).named("tracked_file").build()) { - for (TrackedFile file : files) { - appender.add((StructLike) file); - } - } - - return out.toInputFile(); - } - - private InputFile filterableRoot(FileFormat format) throws IOException { - return filterableRoot(format, null); - } - - private InputFile filterableRoot(FileFormat format, DeletionVector dv) throws IOException { - InputFile matchedLeaf = - writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of( - dataFileWithStats("leaf-file.parquet", idStats(0, 99)), - dataFileWithStats("leaf-prune.parquet", idStats(100, 199)))); - return writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of( - dataFileWithStats("root-keep.parquet", idStats(0, 99), dv), - dataFileWithStats("root-prune.parquet", idStats(100, 199)), - dataManifestWithStats(matchedLeaf.location(), idStats(0, 199)), - dataManifestWithStats( - "pruned-leaf." + format.name().toLowerCase(Locale.ROOT), idStats(100, 199)))); + return V4TestHelpers.writeTrackedFiles(out, format, partitionType, STATS_TYPE, files) + .toInputFile(); } private InputFile mixedPartitionedAndUnpartitionedRoot(FileFormat format) throws IOException { @@ -707,11 +617,13 @@ private InputFile mixedPartitionedAndUnpartitionedRoot(FileFormat format) throws format, unionType, ImmutableList.of( - dataFile("partitioned.parquet", BY_ID_SPEC.specId(), idPartition(unionType, 1), null), - dataFile( - "unpartitioned.parquet", + V4TestHelpers.dataFile( + PARTITIONED_FILE, BY_ID_SPEC.specId(), idPartition(unionType, 1), null, null), + V4TestHelpers.dataFile( + UNPARTITIONED_FILE, UNPARTITIONED_WITH_SCHEMA.specId(), idPartition(unionType, null), + null, null))); } From 291e82b048cb930f11940ec8ddada837167ed23e Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Thu, 8 Oct 2026 11:22:15 -0700 Subject: [PATCH 20/21] Core: Expand V4 file planner test coverage for case sensitivity and tasks Add a filtered data-only root case so the filter tests mirror all three base layouts (data-only, leaf, and mixed roots). Extend the case-sensitive filter test to a leaf manifest so the setting is verified to reach leaf readers, not only the root. Add task-construction coverage for a data file without a colocated deletion vector having no deletes, and for each task exposing its file's partition spec. --- .../org/apache/iceberg/TestFilePlanner.java | 175 +++++++++++------- 1 file changed, 112 insertions(+), 63 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index caafe6751129..8699a5062866 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -18,6 +18,15 @@ */ package org.apache.iceberg; +import static org.apache.iceberg.V4TestHelpers.ADDED_TRACKING; +import static org.apache.iceberg.V4TestHelpers.FILE_SIZE_IN_BYTES; +import static org.apache.iceberg.V4TestHelpers.RECORD_COUNT; +import static org.apache.iceberg.V4TestHelpers.SNAPSHOT_ID; +import static org.apache.iceberg.V4TestHelpers.dataFile; +import static org.apache.iceberg.V4TestHelpers.deletionVector; +import static org.apache.iceberg.V4TestHelpers.manifestRef; +import static org.apache.iceberg.V4TestHelpers.manifestRefWithStats; +import static org.apache.iceberg.V4TestHelpers.writeTrackedFiles; import static org.apache.iceberg.types.Types.NestedField.optional; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -51,14 +60,11 @@ import org.junit.jupiter.params.provider.FieldSource; class TestFilePlanner { - private static final long SNAPSHOT_ID = 42L; private static final long SEQUENCE_NUMBER = 7L; private static final long FIRST_ROW_ID = 1000L; - private static final long RECORD_COUNT = 100L; - private static final long FILE_SIZE_IN_BYTES = 1024L; private static final String TABLE_LOCATION = "s3://bucket/db/table"; private static final DeletionVector DV = - V4TestHelpers.deletionVector("s3://bucket/db/table/dv.puffin", 100L, 50L, 5L); + deletionVector("s3://bucket/db/table/dv.puffin", 100L, 50L, 5L); private static final String FILE_A = "s3://bucket/db/table/file-a.parquet"; private static final String FILE_B = "s3://bucket/db/table/file-b.parquet"; @@ -70,14 +76,12 @@ class TestFilePlanner { private static final Schema TABLE_SCHEMA = new Schema( optional(1, "id", Types.IntegerType.get()), optional(2, "data", Types.StringType.get())); - private static final PartitionSpec SPEC = - PartitionSpec.builderFor(TABLE_SCHEMA).identity("id").build(); + private static final int UNPARTITIONED_SPEC_ID = PartitionSpec.unpartitioned().specId(); private static final Types.StructType EMPTY_PARTITION = Types.StructType.of(); private static final PartitionData EMPTY_PARTITION_DATA = new PartitionData(EMPTY_PARTITION); private static final Map UNPARTITIONED_SPECS = - ImmutableMap.of(PartitionSpec.unpartitioned().specId(), PartitionSpec.unpartitioned()); + ImmutableMap.of(UNPARTITIONED_SPEC_ID, PartitionSpec.unpartitioned()); - // a table that evolved from unpartitioned (spec 0) to identity(id) (spec 1) private static final PartitionSpec UNPARTITIONED_WITH_SCHEMA = PartitionSpec.builderFor(TABLE_SCHEMA).withSpecId(0).build(); private static final PartitionSpec BY_ID_SPEC = @@ -104,8 +108,7 @@ void rootWithOnlyDataFiles(FileFormat format) throws IOException { writeManifest( format, EMPTY_PARTITION, - ImmutableList.of( - dataFile(FILE_A, EMPTY_PARTITION_DATA), dataFile(FILE_B, EMPTY_PARTITION_DATA))); + ImmutableList.of(unpartitionedDataFile(FILE_A), unpartitionedDataFile(FILE_B))); List tasks = plan(root, UNPARTITIONED_SPECS); @@ -119,11 +122,9 @@ void rootWithOnlyDataFiles(FileFormat format) throws IOException { @FieldSource("MANIFEST_FORMATS") void rootWithOnlyLeafManifests(FileFormat format) throws IOException { InputFile leafA = - writeManifest( - format, EMPTY_PARTITION, ImmutableList.of(dataFile(FILE_A, EMPTY_PARTITION_DATA))); + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_A))); InputFile leafB = - writeManifest( - format, EMPTY_PARTITION, ImmutableList.of(dataFile(FILE_B, EMPTY_PARTITION_DATA))); + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_B))); InputFile root = writeManifest( format, @@ -142,17 +143,15 @@ void rootWithOnlyLeafManifests(FileFormat format) throws IOException { @FieldSource("MANIFEST_FORMATS") void rootWithMixedDataAndLeafManifests(FileFormat format) throws IOException { InputFile leaf1 = - writeManifest( - format, EMPTY_PARTITION, ImmutableList.of(dataFile(FILE_C, EMPTY_PARTITION_DATA))); + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_C))); InputFile leaf2 = - writeManifest( - format, EMPTY_PARTITION, ImmutableList.of(dataFile(FILE_D, EMPTY_PARTITION_DATA))); + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_D))); InputFile root = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( - dataFile(FILE_A, EMPTY_PARTITION_DATA), + unpartitionedDataFile(FILE_A), dataManifest(leaf1.location()), dataManifest(leaf2.location()))); @@ -164,6 +163,26 @@ void rootWithMixedDataAndLeafManifests(FileFormat format) throws IOException { .containsExactlyInAnyOrder(FILE_A, FILE_C, FILE_D); } + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void filterSkipsAndMatchesRootDataFiles(FileFormat format) throws IOException { + InputFile root = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFileWithStats(FILE_A, idStats(0, 99)), + dataFileWithStats(FILE_B, idStats(100, 199)))); + + List tasks = + plan(root, UNPARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("id", 50))); + + assertThat(tasks) + .extracting(task -> task.file().location()) + .as("only the root data file whose stats match the filter survives") + .containsExactlyInAnyOrder(FILE_A); + } + @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void filterSkipsAndMatchesRootDataFilesAndManifests(FileFormat format) throws IOException { @@ -219,13 +238,21 @@ void filterSkipsAndMatchesLeafDataFiles(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void filterHonorsCaseSensitivity(FileFormat format) throws IOException { + InputFile leaf = + writeManifest( + format, + EMPTY_PARTITION, + ImmutableList.of( + dataFileWithStats(FILE_C, idStats(0, 99)), + dataFileWithStats(FILE_D, idStats(100, 199)))); InputFile root = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( dataFileWithStats(FILE_A, idStats(0, 99)), - dataFileWithStats(FILE_B, idStats(100, 199)))); + dataFileWithStats(FILE_B, idStats(100, 199)), + dataManifestWithStats(leaf.location(), idStats(0, 199)))); List tasks = plan( @@ -234,8 +261,8 @@ void filterHonorsCaseSensitivity(FileFormat format) throws IOException { planner -> planner.filterData(Expressions.equal("ID", 50)).caseSensitive(false)); assertThat(tasks) .extracting(task -> task.file().location()) - .as("a case-insensitive filter resolves \"ID\" to \"id\" so the matching file survives") - .containsExactlyInAnyOrder(FILE_A); + .as("a case-insensitive filter resolves \"ID\" to \"id\" in both the root and leaf readers") + .containsExactlyInAnyOrder(FILE_A, FILE_C); assertThatThrownBy( () -> @@ -252,15 +279,55 @@ void filterHonorsCaseSensitivity(FileFormat format) throws IOException { @FieldSource("MANIFEST_FORMATS") void taskAttachesColocatedDV(FileFormat format) throws IOException { TrackedFile fileWithDV = - dataFile("s3://bucket/db/table/with-dv.parquet", EMPTY_PARTITION_DATA, DV); - InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(fileWithDV)); + unpartitionedDataFileWithDV("s3://bucket/db/table/with-dv.parquet", DV); + TrackedFile fileWithoutDV = unpartitionedDataFile(FILE_A); + InputFile root = + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(fileWithDV, fileWithoutDV)); List tasks = plan(root, UNPARTITIONED_SPECS); - assertThat(tasks).hasSize(1); - assertThat(tasks.get(0).deletes()) - .hasSize(1) - .allSatisfy(delete -> assertThat(delete.location()).isEqualTo(DV.location())); + assertThat(tasks) + .hasSize(2) + .satisfiesExactlyInAnyOrder( + task -> { + assertThat(task.file().location()).isEqualTo("s3://bucket/db/table/with-dv.parquet"); + assertThat(task.deletes()) + .as("the colocated DV is attached as the file's only delete") + .hasSize(1) + .allSatisfy(delete -> assertThat(delete.location()).isEqualTo(DV.location())); + }, + task -> { + assertThat(task.file().location()).isEqualTo(FILE_A); + assertThat(task.deletes()) + .as("a file without a colocated DV has no deletes") + .isEmpty(); + }); + } + + @ParameterizedTest + @FieldSource("MANIFEST_FORMATS") + void taskExposesFileSchemaAndSpec(FileFormat format) throws IOException { + InputFile root = mixedPartitionedAndUnpartitionedRoot(format); + + List tasks = plan(root, MIXED_SPECS); + + assertThat(tasks) + .hasSize(2) + .satisfiesExactlyInAnyOrder( + task -> { + assertThat(task.file().location()).isEqualTo(PARTITIONED_FILE); + assertThat(task.spec()) + .as("the task carries the full spec its file was written with") + .isEqualTo(BY_ID_SPEC); + assertThat(task.schema().asStruct()).isEqualTo(TABLE_SCHEMA.asStruct()); + }, + task -> { + assertThat(task.file().location()).isEqualTo(UNPARTITIONED_FILE); + assertThat(task.spec()) + .as("the task carries the full spec its file was written with") + .isEqualTo(UNPARTITIONED_WITH_SCHEMA); + assertThat(task.schema().asStruct()).isEqualTo(TABLE_SCHEMA.asStruct()); + }); } @ParameterizedTest @@ -298,20 +365,15 @@ void ignoreResidualsProducesAlwaysTrueResidual(FileFormat format) throws IOExcep @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void tableLocationForwardedToRootAndLeafReaders(FileFormat format) throws IOException { - // the only test that uses relative paths: it verifies the reader relativizes them against the - // table location in both the root and the lazily-opened leaf InputFile leaf = writeManifest( - format, - EMPTY_PARTITION, - ImmutableList.of(dataFile("leaf-data.parquet", EMPTY_PARTITION_DATA))); + format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile("leaf-data.parquet"))); InputFile root = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( - dataFile("root-data.parquet", EMPTY_PARTITION_DATA), - dataManifest(leaf.location()))); + unpartitionedDataFile("root-data.parquet"), dataManifest(leaf.location()))); List tasks = plan(root, UNPARTITIONED_SPECS); @@ -326,8 +388,7 @@ void tableLocationForwardedToRootAndLeafReaders(FileFormat format) throws IOExce @FieldSource("MANIFEST_FORMATS") void closingWithoutIteratingOpensOnlyRoot(FileFormat format) throws IOException { InputFile leaf = - writeManifest( - format, EMPTY_PARTITION, ImmutableList.of(dataFile(FILE_A, EMPTY_PARTITION_DATA))); + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_A))); InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); @@ -350,11 +411,9 @@ void leafOpenedOnlyWhenPrecedingLeafExhausted(FileFormat format) throws IOExcept writeManifest( format, EMPTY_PARTITION, - ImmutableList.of( - dataFile(FILE_A, EMPTY_PARTITION_DATA), dataFile(FILE_B, EMPTY_PARTITION_DATA))); + ImmutableList.of(unpartitionedDataFile(FILE_A), unpartitionedDataFile(FILE_B))); InputFile leaf2 = - writeManifest( - format, EMPTY_PARTITION, ImmutableList.of(dataFile(FILE_C, EMPTY_PARTITION_DATA))); + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_C))); InputFile root = writeManifest( format, @@ -413,7 +472,7 @@ void v3ManifestLeafUnsupported(FileFormat format) throws IOException { String leafLocation = "s3://bucket/db/table/v3-leaf." + format.name().toLowerCase(Locale.ROOT); TrackedFile v3Leaf = new TrackedFileStruct( - V4TestHelpers.ADDED_TRACKING, + ADDED_TRACKING, FileContent.DATA_MANIFEST, 3, // format version the v4 reader rejects leafLocation, @@ -518,12 +577,12 @@ private static ManifestFile asManifest(InputFile file) { /* keyMetadata= */ null); } - private static TrackedFile dataFile(String location, PartitionData partition) { - return V4TestHelpers.dataFile(location, specId(partition), partition, null, null); + private static TrackedFile unpartitionedDataFile(String location) { + return dataFile(location, UNPARTITIONED_SPEC_ID, EMPTY_PARTITION_DATA); } - private static TrackedFile dataFile(String location, PartitionData partition, DeletionVector dv) { - return V4TestHelpers.dataFile(location, specId(partition), partition, null, dv); + private static TrackedFile unpartitionedDataFileWithDV(String location, DeletionVector dv) { + return dataFile(location, UNPARTITIONED_SPEC_ID, EMPTY_PARTITION_DATA, null, dv); } private static TrackedFile dataFileWithStats(String location, ContentStats stats) { @@ -532,8 +591,7 @@ private static TrackedFile dataFileWithStats(String location, ContentStats stats private static TrackedFile dataFileWithStats( String location, ContentStats stats, DeletionVector dv) { - return V4TestHelpers.dataFile( - location, specId(EMPTY_PARTITION_DATA), EMPTY_PARTITION_DATA, stats, dv); + return dataFile(location, UNPARTITIONED_SPEC_ID, EMPTY_PARTITION_DATA, stats, dv); } private static TrackedFile dataManifest(String location) { @@ -541,13 +599,11 @@ private static TrackedFile dataManifest(String location) { } private static TrackedFile dataManifest(String location, int addedFiles) { - return V4TestHelpers.manifestRef( - FileContent.DATA_MANIFEST, location, dataManifestInfo(addedFiles)); + return manifestRef(FileContent.DATA_MANIFEST, location, dataManifestInfo(addedFiles)); } private static TrackedFile dataManifestWithStats(String location, ContentStats stats) { - return V4TestHelpers.manifestRefWithStats( - FileContent.DATA_MANIFEST, location, stats, dataManifestInfo(1)); + return manifestRefWithStats(FileContent.DATA_MANIFEST, location, stats, dataManifestInfo(1)); } private static ContentStats idStats(int lower, int upper) { @@ -583,12 +639,7 @@ private static ManifestInfo dataManifestInfo(int addedFiles) { } private static TrackedFile deleteManifest(String location) { - return V4TestHelpers.manifestRef(FileContent.DELETE_MANIFEST, location, null); - } - - private static Integer specId(PartitionData partition) { - boolean unpartitioned = partition.size() == 0; - return unpartitioned ? PartitionSpec.unpartitioned().specId() : SPEC.specId(); + return manifestRef(FileContent.DELETE_MANIFEST, location, null); } private static PartitionData idPartition(Types.StructType unionType, Integer id) { @@ -607,8 +658,7 @@ private InputFile writeManifest( + System.nanoTime() + "." + format.name().toLowerCase(Locale.ROOT)); - return V4TestHelpers.writeTrackedFiles(out, format, partitionType, STATS_TYPE, files) - .toInputFile(); + return writeTrackedFiles(out, format, partitionType, STATS_TYPE, files).toInputFile(); } private InputFile mixedPartitionedAndUnpartitionedRoot(FileFormat format) throws IOException { @@ -617,9 +667,8 @@ private InputFile mixedPartitionedAndUnpartitionedRoot(FileFormat format) throws format, unionType, ImmutableList.of( - V4TestHelpers.dataFile( - PARTITIONED_FILE, BY_ID_SPEC.specId(), idPartition(unionType, 1), null, null), - V4TestHelpers.dataFile( + dataFile(PARTITIONED_FILE, BY_ID_SPEC.specId(), idPartition(unionType, 1), null, null), + dataFile( UNPARTITIONED_FILE, UNPARTITIONED_WITH_SCHEMA.specId(), idPartition(unionType, null), From 8848a7d53b3669b39b7e2ced222a42a812e23a3d Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Thu, 8 Oct 2026 14:31:50 -0700 Subject: [PATCH 21/21] Incorporate feedback --- .../org/apache/iceberg/TestFilePlanner.java | 189 ++++++++++-------- .../apache/iceberg/TestRootManifestFile.java | 1 - 2 files changed, 105 insertions(+), 85 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java index 8699a5062866..2edeb58dc10c 100644 --- a/core/src/test/java/org/apache/iceberg/TestFilePlanner.java +++ b/core/src/test/java/org/apache/iceberg/TestFilePlanner.java @@ -104,7 +104,7 @@ class TestFilePlanner { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void rootWithOnlyDataFiles(FileFormat format) throws IOException { - InputFile root = + ManifestFile root = writeManifest( format, EMPTY_PARTITION, @@ -121,15 +121,15 @@ void rootWithOnlyDataFiles(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void rootWithOnlyLeafManifests(FileFormat format) throws IOException { - InputFile leafA = + ManifestFile leafA = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_A))); - InputFile leafB = + ManifestFile leafB = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_B))); - InputFile root = + ManifestFile root = writeManifest( format, EMPTY_PARTITION, - ImmutableList.of(dataManifest(leafA.location()), dataManifest(leafB.location()))); + ImmutableList.of(dataManifest(leafA.path()), dataManifest(leafB.path()))); List tasks = plan(root, UNPARTITIONED_SPECS); @@ -142,18 +142,18 @@ void rootWithOnlyLeafManifests(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void rootWithMixedDataAndLeafManifests(FileFormat format) throws IOException { - InputFile leaf1 = + ManifestFile leaf1 = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_C))); - InputFile leaf2 = + ManifestFile leaf2 = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_D))); - InputFile root = + ManifestFile root = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( unpartitionedDataFile(FILE_A), - dataManifest(leaf1.location()), - dataManifest(leaf2.location()))); + dataManifest(leaf1.path()), + dataManifest(leaf2.path()))); List tasks = plan(root, UNPARTITIONED_SPECS); @@ -166,7 +166,7 @@ void rootWithMixedDataAndLeafManifests(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void filterSkipsAndMatchesRootDataFiles(FileFormat format) throws IOException { - InputFile root = + ManifestFile root = writeManifest( format, EMPTY_PARTITION, @@ -186,21 +186,21 @@ void filterSkipsAndMatchesRootDataFiles(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void filterSkipsAndMatchesRootDataFilesAndManifests(FileFormat format) throws IOException { - InputFile matchedLeaf = + ManifestFile matchedLeaf = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( dataFileWithStats(FILE_C, idStats(0, 99)), dataFileWithStats(FILE_D, idStats(100, 199)))); - InputFile root = + ManifestFile root = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( dataFileWithStats(FILE_A, idStats(0, 99)), dataFileWithStats(FILE_B, idStats(100, 199)), - dataManifestWithStats(matchedLeaf.location(), idStats(0, 199)), + dataManifestWithStats(matchedLeaf.path(), idStats(0, 199)), dataManifestWithStats( "s3://bucket/db/table/pruned-leaf.parquet", idStats(100, 199)))); @@ -217,15 +217,15 @@ void filterSkipsAndMatchesRootDataFilesAndManifests(FileFormat format) throws IO @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void filterSkipsAndMatchesLeafDataFiles(FileFormat format) throws IOException { - InputFile leaf = + ManifestFile leaf = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( dataFileWithStats(FILE_C, idStats(0, 99)), dataFileWithStats(FILE_D, idStats(100, 199)))); - InputFile root = - writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location(), 2))); + ManifestFile root = + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.path(), 2))); List tasks = plan(root, UNPARTITIONED_SPECS, planner -> planner.filterData(Expressions.equal("id", 50))); @@ -238,21 +238,21 @@ void filterSkipsAndMatchesLeafDataFiles(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void filterHonorsCaseSensitivity(FileFormat format) throws IOException { - InputFile leaf = + ManifestFile leaf = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( dataFileWithStats(FILE_C, idStats(0, 99)), dataFileWithStats(FILE_D, idStats(100, 199)))); - InputFile root = + ManifestFile root = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( dataFileWithStats(FILE_A, idStats(0, 99)), dataFileWithStats(FILE_B, idStats(100, 199)), - dataManifestWithStats(leaf.location(), idStats(0, 199)))); + dataManifestWithStats(leaf.path(), idStats(0, 199)))); List tasks = plan( @@ -281,7 +281,7 @@ void taskAttachesColocatedDV(FileFormat format) throws IOException { TrackedFile fileWithDV = unpartitionedDataFileWithDV("s3://bucket/db/table/with-dv.parquet", DV); TrackedFile fileWithoutDV = unpartitionedDataFile(FILE_A); - InputFile root = + ManifestFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(fileWithDV, fileWithoutDV)); List tasks = plan(root, UNPARTITIONED_SPECS); @@ -307,7 +307,20 @@ void taskAttachesColocatedDV(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void taskExposesFileSchemaAndSpec(FileFormat format) throws IOException { - InputFile root = mixedPartitionedAndUnpartitionedRoot(format); + Types.StructType unionType = Partitioning.unionPartitionTypes(MIXED_SPECS.values()); + ManifestFile root = + writeManifest( + format, + unionType, + ImmutableList.of( + dataFile( + PARTITIONED_FILE, BY_ID_SPEC.specId(), idPartition(unionType, 1), null, null), + dataFile( + UNPARTITIONED_FILE, + UNPARTITIONED_WITH_SCHEMA.specId(), + idPartition(unionType, null), + null, + null))); List tasks = plan(root, MIXED_SPECS); @@ -333,7 +346,20 @@ void taskExposesFileSchemaAndSpec(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void residualForMixedPartitionedAndUnpartitioned(FileFormat format) throws IOException { - InputFile root = mixedPartitionedAndUnpartitionedRoot(format); + Types.StructType unionType = Partitioning.unionPartitionTypes(MIXED_SPECS.values()); + ManifestFile root = + writeManifest( + format, + unionType, + ImmutableList.of( + dataFile( + PARTITIONED_FILE, BY_ID_SPEC.specId(), idPartition(unionType, 1), null, null), + dataFile( + UNPARTITIONED_FILE, + UNPARTITIONED_WITH_SCHEMA.specId(), + idPartition(unionType, null), + null, + null))); Expression filter = Expressions.and(Expressions.equal("id", 1), Expressions.equal("data", "x")); List tasks = plan(root, MIXED_SPECS, planner -> planner.filterData(filter)); @@ -350,7 +376,20 @@ void residualForMixedPartitionedAndUnpartitioned(FileFormat format) throws IOExc @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void ignoreResidualsProducesAlwaysTrueResidual(FileFormat format) throws IOException { - InputFile root = mixedPartitionedAndUnpartitionedRoot(format); + Types.StructType unionType = Partitioning.unionPartitionTypes(MIXED_SPECS.values()); + ManifestFile root = + writeManifest( + format, + unionType, + ImmutableList.of( + dataFile( + PARTITIONED_FILE, BY_ID_SPEC.specId(), idPartition(unionType, 1), null, null), + dataFile( + UNPARTITIONED_FILE, + UNPARTITIONED_WITH_SCHEMA.specId(), + idPartition(unionType, null), + null, + null))); Expression filter = Expressions.and(Expressions.equal("id", 1), Expressions.equal("data", "x")); List tasks = @@ -365,15 +404,15 @@ void ignoreResidualsProducesAlwaysTrueResidual(FileFormat format) throws IOExcep @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void tableLocationForwardedToRootAndLeafReaders(FileFormat format) throws IOException { - InputFile leaf = + ManifestFile leaf = writeManifest( format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile("leaf-data.parquet"))); - InputFile root = + ManifestFile root = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( - unpartitionedDataFile("root-data.parquet"), dataManifest(leaf.location()))); + unpartitionedDataFile("root-data.parquet"), dataManifest(leaf.path()))); List tasks = plan(root, UNPARTITIONED_SPECS); @@ -387,19 +426,19 @@ void tableLocationForwardedToRootAndLeafReaders(FileFormat format) throws IOExce @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void closingWithoutIteratingOpensOnlyRoot(FileFormat format) throws IOException { - InputFile leaf = + ManifestFile leaf = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_A))); - InputFile root = - writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.location()))); + ManifestFile root = + writeManifest(format, EMPTY_PARTITION, ImmutableList.of(dataManifest(leaf.path()))); RecordingFileIO recordingIO = new RecordingFileIO(fileIO); FilePlanner planner = - new FilePlanner(recordingIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) + new FilePlanner(recordingIO, root, TABLE_SCHEMA, UNPARTITIONED_SPECS) .tableLocation(TABLE_LOCATION); planner.planFiles().close(); - assertThat(recordingIO.opened(root.location())).as("the root is read eagerly").isTrue(); - assertThat(recordingIO.opened(leaf.location())) + assertThat(recordingIO.opened(root.path())).as("the root is read eagerly").isTrue(); + assertThat(recordingIO.opened(leaf.path())) .as("leaf readers open lazily, so closing without iterating leaves them unopened") .isFalse(); } @@ -407,44 +446,42 @@ void closingWithoutIteratingOpensOnlyRoot(FileFormat format) throws IOException @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void leafOpenedOnlyWhenPrecedingLeafExhausted(FileFormat format) throws IOException { - InputFile leaf1 = + ManifestFile leaf1 = writeManifest( format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_A), unpartitionedDataFile(FILE_B))); - InputFile leaf2 = + ManifestFile leaf2 = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(unpartitionedDataFile(FILE_C))); - InputFile root = + ManifestFile root = writeManifest( format, EMPTY_PARTITION, - ImmutableList.of(dataManifest(leaf1.location(), 2), dataManifest(leaf2.location()))); + ImmutableList.of(dataManifest(leaf1.path()), dataManifest(leaf2.path()))); RecordingFileIO recordingIO = new RecordingFileIO(fileIO); FilePlanner planner = - new FilePlanner(recordingIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) + new FilePlanner(recordingIO, root, TABLE_SCHEMA, UNPARTITIONED_SPECS) .tableLocation(TABLE_LOCATION) .planWith(null); - try (CloseableIterable plan = planner.planFiles(); - CloseableIterator tasks = plan.iterator()) { + try (CloseableIterable plan = planner.planFiles()) { + CloseableIterator tasks = plan.iterator(); assertThat(tasks.next().file().location()).isEqualTo(FILE_A); - assertThat(recordingIO.opened(leaf1.location())) + assertThat(recordingIO.opened(leaf1.path())) .as("leaf1 opens to produce the first task") .isTrue(); - assertThat(recordingIO.opened(leaf2.location())) + assertThat(recordingIO.opened(leaf2.path())) .as("leaf2 stays closed until leaf1 is exhausted") .isFalse(); assertThat(tasks.next().file().location()).isEqualTo(FILE_B); - assertThat(recordingIO.opened(leaf2.location())) + assertThat(recordingIO.opened(leaf2.path())) .as("reading the last file in leaf1 does not open leaf2") .isFalse(); assertThat(tasks.next().file().location()).isEqualTo(FILE_C); - assertThat(recordingIO.opened(leaf2.location())) - .as("leaf2 opens once leaf1 is drained") - .isTrue(); + assertThat(recordingIO.opened(leaf2.path())).as("leaf2 opens once leaf1 is drained").isTrue(); assertThat(tasks).isExhausted(); } } @@ -452,14 +489,14 @@ void leafOpenedOnlyWhenPrecedingLeafExhausted(FileFormat format) throws IOExcept @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void deleteManifestInRootUnsupported(FileFormat format) throws IOException { - InputFile root = + ManifestFile root = writeManifest( format, EMPTY_PARTITION, ImmutableList.of(deleteManifest("s3://bucket/db/table/deletes.avro"))); FilePlanner planner = - new FilePlanner(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) + new FilePlanner(fileIO, root, TABLE_SCHEMA, UNPARTITIONED_SPECS) .tableLocation(TABLE_LOCATION); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) .isInstanceOf(UnsupportedOperationException.class) @@ -488,10 +525,10 @@ void v3ManifestLeafUnsupported(FileFormat format) throws IOException { null, // keyMetadata null, // splitOffsets null); // equalityIds - InputFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(v3Leaf)); + ManifestFile root = writeManifest(format, EMPTY_PARTITION, ImmutableList.of(v3Leaf)); FilePlanner planner = - new FilePlanner(fileIO, asManifest(root), TABLE_SCHEMA, UNPARTITIONED_SPECS) + new FilePlanner(fileIO, root, TABLE_SCHEMA, UNPARTITIONED_SPECS) .tableLocation(TABLE_LOCATION); assertThatThrownBy(() -> Lists.newArrayList(planner.planFiles())) .isInstanceOf(IllegalArgumentException.class) @@ -501,21 +538,21 @@ void v3ManifestLeafUnsupported(FileFormat format) throws IOException { @ParameterizedTest @FieldSource("MANIFEST_FORMATS") void scanMetricsForComplexFilteredPlan(FileFormat format) throws IOException { - InputFile matchedLeaf = + ManifestFile matchedLeaf = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( dataFileWithStats(FILE_C, idStats(0, 99)), dataFileWithStats(FILE_D, idStats(100, 199)))); - InputFile root = + ManifestFile root = writeManifest( format, EMPTY_PARTITION, ImmutableList.of( dataFileWithStats(FILE_A, idStats(0, 99), DV), dataFileWithStats(FILE_B, idStats(100, 199)), - dataManifestWithStats(matchedLeaf.location(), idStats(0, 199)), + dataManifestWithStats(matchedLeaf.path(), idStats(0, 199)), dataManifestWithStats( "s3://bucket/db/table/pruned-leaf.parquet", idStats(100, 199)))); @@ -550,33 +587,24 @@ void scanMetricsForComplexFilteredPlan(FileFormat format) throws IOException { .isEqualTo(0L); } - private List plan(InputFile root, Map specsById) + private List plan(ManifestFile root, Map specsById) throws IOException { return plan(root, specsById, UnaryOperator.identity()); } private List plan( - InputFile root, Map specsById, UnaryOperator configure) + ManifestFile root, + Map specsById, + UnaryOperator configure) throws IOException { FilePlanner planner = configure.apply( - new FilePlanner(fileIO, asManifest(root), TABLE_SCHEMA, specsById) - .tableLocation(TABLE_LOCATION)); + new FilePlanner(fileIO, root, TABLE_SCHEMA, specsById).tableLocation(TABLE_LOCATION)); try (CloseableIterable tasks = planner.planFiles()) { return Lists.newArrayList(tasks); } } - private static ManifestFile asManifest(InputFile file) { - return new RootManifestFile( - file.location(), - file.getLength(), - SNAPSHOT_ID, - SEQUENCE_NUMBER, - FIRST_ROW_ID, - /* keyMetadata= */ null); - } - private static TrackedFile unpartitionedDataFile(String location) { return dataFile(location, UNPARTITIONED_SPEC_ID, EMPTY_PARTITION_DATA); } @@ -648,7 +676,7 @@ private static PartitionData idPartition(Types.StructType unionType, Integer id) return partition; } - private InputFile writeManifest( + private ManifestFile writeManifest( FileFormat format, Types.StructType partitionType, Iterable files) throws IOException { OutputFile out = @@ -658,22 +686,15 @@ private InputFile writeManifest( + System.nanoTime() + "." + format.name().toLowerCase(Locale.ROOT)); - return writeTrackedFiles(out, format, partitionType, STATS_TYPE, files).toInputFile(); - } - - private InputFile mixedPartitionedAndUnpartitionedRoot(FileFormat format) throws IOException { - Types.StructType unionType = Partitioning.unionPartitionTypes(MIXED_SPECS.values()); - return writeManifest( - format, - unionType, - ImmutableList.of( - dataFile(PARTITIONED_FILE, BY_ID_SPEC.specId(), idPartition(unionType, 1), null, null), - dataFile( - UNPARTITIONED_FILE, - UNPARTITIONED_WITH_SCHEMA.specId(), - idPartition(unionType, null), - null, - null))); + InputFile written = + writeTrackedFiles(out, format, partitionType, STATS_TYPE, files).toInputFile(); + return new RootManifestFile( + written.location(), + written.getLength(), + SNAPSHOT_ID, + SEQUENCE_NUMBER, + FIRST_ROW_ID, + /* keyMetadata= */ null); } private static class RecordingFileIO implements FileIO { diff --git a/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java b/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java index 107383fc9a04..b69e6a21db2b 100644 --- a/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java +++ b/core/src/test/java/org/apache/iceberg/TestRootManifestFile.java @@ -53,7 +53,6 @@ void defaultedFields() { assertThat(root.content()).isEqualTo(ManifestContent.DATA); assertThat(root.formatVersion()).isEqualTo(4); - assertThat(root.keyMetadata()).isNull(); } @Test