Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 3.5k
Core: Add V4 scan task planner#17541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base:main
Are you sure you want to change the base?
Changes from all commits
93469d0829dad2529c5390ce42432ad2101b48c3d81394ba41ee7d93File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,314 @@ | ||
| /* | ||
| * 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.nio.ByteBuffer; | ||
| 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.CloseableGroup; | ||
| import org.apache.iceberg.io.CloseableIterable; | ||
| import org.apache.iceberg.io.CloseableIterator; | ||
| import org.apache.iceberg.io.FileIO; | ||
| import org.apache.iceberg.io.InputFile; | ||
| 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. | ||
| * | ||
| * <p>Emits a task for each live {@code DATA} entry and expands {@code DATA_MANIFEST} entries into | ||
| * their leaf manifests. A data entry's colocated deletion vector is attached to its task as a | ||
| * {@link DeleteFile}. | ||
| */ | ||
| class ScanTaskPlanner { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming: I don't think this should identify The purpose of this class is to produce matching MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. An alternative is | ||
| private static final DeleteFile[] NO_DELETES = new DeleteFile[0]; | ||
| private final FileIO io; | ||
| private final String rootManifestLocation; | ||
| private final Map<Integer, PartitionSpec> specsById; | ||
| private final String tableLocation; | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed for handling relative paths? MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we need pipe it to the reader so that the path resolution happens. | ||
| private final Expression dataFilter; | ||
| private final boolean ignoreResiduals; | ||
| private final boolean caseSensitive; | ||
| private final ScanMetrics scanMetrics; | ||
| private final ExecutorService executorService; | ||
| private final Map<Integer, TaskContext> taskContextsBySpec = Maps.newConcurrentMap(); | ||
| private ScanTaskPlanner( | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trying to wrap my head around encrypted root manifests. For that we'd need a couple of more inputs here. If I'm not mistaken, it would go through a similar mechanism as the pre-V4 manifest list location: it'd require a I'm wondering if we want to pass the proposed new interface for "files encrypted with key-id" (PR here) instead of MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR doesn't handle encryption yet. In a followup, we will be converting this into a new Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gaborkaszab, we have been moving toward passing files to methods in For the root metadata file specifically, we will need to create a Your PR is also on my TODO list for reviews, so I apologize if you've already thought through this and I'm repeating what you already know. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, passing the right | ||
| FileIO io, | ||
| String rootManifestLocation, | ||
| Map<Integer, PartitionSpec> specsById, | ||
| String tableLocation, | ||
| Expression dataFilter, | ||
| boolean ignoreResiduals, | ||
| boolean caseSensitive, | ||
| ScanMetrics scanMetrics, | ||
| ExecutorService executorService) { | ||
| this.io = io; | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we also want to include a reference to the Table being scanned (that's consistent with other existing implementations). Keeping the FileIO separate is necessary for scan/planning which doesn't use the table's FileIO/credentials. I believe the table reference is also used extensively for things like converting to distributed plan and metadata tables. (This would be required with the existing scan heirerarchy). MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kept the surface minimal here to mirror I'd prefer to add the Table reference when that wiring lands and actually uses it. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is good for now. I generally try to avoid passing | ||
| this.rootManifestLocation = rootManifestLocation; | ||
| this.specsById = specsById; | ||
| this.tableLocation = tableLocation; | ||
| this.dataFilter = dataFilter; | ||
| this.ignoreResiduals = ignoreResiduals; | ||
| this.caseSensitive = caseSensitive; | ||
| this.scanMetrics = scanMetrics; | ||
| this.executorService = executorService; | ||
| } | ||
| static Builder builder( | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What criteria did you use to decide what is passed to create the builder vs passed to configure the builder? MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added factor args for the ones without any sensible default. This is also aligned to the builder in Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm removing | ||
| FileIO io, | ||
| String rootManifestLocation, | ||
| Map<Integer, PartitionSpec> specsById, | ||
| String tableLocation) { | ||
| return new Builder(io, rootManifestLocation, specsById, tableLocation); | ||
| } | ||
| CloseableIterable<FileScanTask> planFiles() { | ||
| List<TrackedFile> rootDataFiles = Lists.newArrayList(); | ||
| List<TrackedFile> leafManifests = Lists.newArrayList(); | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be | ||
| // root is drained into these lists. Leaf references must be buffered so they can be fanned | ||
| // out; direct DATA entries are buffered too, bounded by how many data files a tree keeps | ||
| // directly in the root. Leaf tasks stay lazy: planLeaf opens no reader until iterated. | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment applies to the lists above and the It is also too long. It's obvious from the code that the data files and leaf manifests are kept in a list. It's also pretty obvious that the leaf manifests are going to be read as tasks and that we aren't going to emit the data files immediately. This comment doesn't seem very insightful or helpful to me. | ||
| scanMetrics.scannedDataManifests().increment(); | ||
| try (CloseableIterable<TrackedFile> rootEntries = | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Root manifest file can be encrypted. I see an exception throws for encrypted leaf manifests, for consistency shouldn't we Thor one here, or let the reader fail anyway because it's unable to parse the file? MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will add encryption support soon as a followup. Until then would it be reasonable to just let the reader fail? Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we handle the lack of encryption support consistently. For root manifest we leave the reader/parser to fail, while for leaf manifests we have a check and throw an exception ourselves. | ||
| open(io.newInputFile(rootManifestLocation), /* manifestDv= */ null)) { | ||
| for (TrackedFile entry : rootEntries) { | ||
| switch (entry.contentType()) { | ||
| case DATA: | ||
| rootDataFiles.add(entry); | ||
| break; | ||
| case DATA_MANIFEST: | ||
| leafManifests.add(entry); | ||
| break; | ||
| default: | ||
| // delete manifests appear only on upgraded v2/v3 tables; that 2-phase path is not | ||
| // supported yet, so a natively-written v4 tree is the only supported input for now | ||
| ||
| throw new UnsupportedOperationException( | ||
| "Cannot plan content type in root manifest: " + entry.contentType()); | ||
Comment on lines
+110
to
+116
| ||
| } | ||
| } | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException("Failed to close root manifest: " + rootManifestLocation, e); | ||
| } | ||
| // read each leaf's data entries lazily; only leaf reads are worth the parallel backend, so the | ||
| // already-in-hand root data entries are concatenated in directly | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think this comment makes sense. Only leaf manifests need to be read. Someone reading this code doesn't need to think about the data files that were stored in the root, only that this step is producing | ||
| List<CloseableIterable<TrackedFile>> leafDataEntries = Lists.newArrayList(); | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't all of these known to be files, and specifically data files? Why call them "entries"? Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since these are lazy, I would call this | ||
| for (TrackedFile leaf : leafManifests) { | ||
| leafDataEntries.add(planLeaf(leaf)); | ||
| } | ||
| CloseableIterable<TrackedFile> expandedLeafEntries = | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm not mistaken, this is able to read a 2-level metadata tree: root and leaves. I recall there was a discussion that theoretically we can have more levels in the tree. I'm not sure if there is anything in the proposed spec or anywhere that prevents writers doing that, but if not, I'm wondering we should be flexible enough on the read path to support such a layout. MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. V4 we intentionally are keeping it at two levels. For tables with multi-billion files, we will need to get to three levels or more, but that is outside the scope of v4. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've just checked the spec proposal, and while we don't explicitly sat the tree was to have 2 levels, the way how we describe root manifest and data/delete manifests implies that there can't be more level ATM. | ||
| executorService != null | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did this drop the | ||
| ? new ParallelIterable<>(leafDataEntries, executorService) | ||
| : CloseableIterable.concat(leafDataEntries); | ||
| CloseableIterable<TrackedFile> dataEntries = | ||
| CloseableIterable.concat( | ||
| ImmutableList.of(CloseableIterable.withNoopClose(rootDataFiles), expandedLeafEntries)); | ||
| return CloseableIterable.transform(dataEntries, this::createTask); | ||
| } | ||
| private CloseableIterable<TrackedFile> planLeaf(TrackedFile leaf) { | ||
| // an upgraded tree can reference a legacy-format leaf | ||
| if (leaf.formatVersion() != TableMetadata.MIN_FORMAT_VERSION_ADAPTIVE_MANIFEST_TREE) { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's odd to use Where should this be enforced? I'd prefer it in the | ||
| throw new UnsupportedOperationException( | ||
| "Cannot expand leaf manifest with format version " | ||
| + leaf.formatVersion() | ||
| + ": " | ||
| + leaf.location()); | ||
| } | ||
| if (leaf.keyMetadata() != null) { | ||
| throw new UnsupportedOperationException( | ||
| "Cannot read encrypted leaf manifest: " + leaf.location()); | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be supported by opening the file using a MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. I will do this as a followup. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't the right place to reject non-null key metadata. That is done automatically in Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I don't think it makes sense to duplicate code and fix it in a follow up. Let's use the right abstractions so we don't rewrite what's already working and need to come back to fix it. | ||
| } | ||
| return new LeafDataEntries(leaf); | ||
| } | ||
| /** Lazily reads one leaf manifest's data entries; each iteration owns and counts its reader. */ | ||
| private class LeafDataEntries extends CloseableGroup implements CloseableIterable<TrackedFile> { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that this class is needed. This is also transforming the iterator, but that doesn't need to be done in this iterator and in fact should not be. This is creating an | ||
| private final TrackedFile leaf; | ||
| private LeafDataEntries(TrackedFile leaf) { | ||
| this.leaf = leaf; | ||
| } | ||
| @Override | ||
| public CloseableIterator<TrackedFile> iterator() { | ||
| // pass the known leaf size so the reader sizes the read instead of stat-ing the file | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the responsibility of | ||
| ByteBuffer manifestDv = leaf.manifestInfo() != null ? leaf.manifestInfo().dv() : null; | ||
| CloseableIterable<TrackedFile> entries = | ||
| ||
| open(io.newInputFile(leaf.location(), leaf.fileSizeInBytes()), manifestDv); | ||
| addCloseable(entries); | ||
| scanMetrics.scannedDataManifests().increment(); | ||
| return CloseableIterable.transform(entries, ScanTaskPlanner.this::requireDataEntry) | ||
| .iterator(); | ||
| } | ||
| } | ||
| private CloseableIterable<TrackedFile> open(InputFile manifest, ByteBuffer manifestDv) { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't open the file, it creates a reader. This should probably be called | ||
| return V4ManifestReader.builder(manifest, specsById, tableLocation) | ||
| .forScanPlanning() | ||
| .filter(dataFilter) | ||
| .caseSensitive(caseSensitive) | ||
| .scanMetrics(scanMetrics) | ||
| .manifestDv(manifestDv) | ||
| ||
| .build(); | ||
| } | ||
| private TrackedFile requireDataEntry(TrackedFile entry) { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't use this pattern of returning a validated object. | ||
| // the tree is at most two levels, so a leaf holds only DATA entries | ||
| Preconditions.checkArgument( | ||
MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check state instead? | ||
| entry.contentType() != FileContent.DATA_MANIFEST, | ||
| "Cannot expand a nested manifest in a leaf manifest: %s", | ||
| entry.location()); | ||
| if (entry.contentType() != FileContent.DATA) { | ||
| throw new UnsupportedOperationException( | ||
| "Cannot plan content type in leaf manifest: " + entry.contentType()); | ||
| } | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you trying to have a more specific error message for manifests found in leaf files? It doesn't make much sense to do this check twice. I'm also skeptical that this is the responsibility of the planner. Shouldn't this be done by a leaf manifest reader? Verifying that the files in a manifest are valid should be the reader's job. | ||
| return entry; | ||
| } | ||
| private FileScanTask createTask(TrackedFile trackedFile) { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should accept only MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please see response to below (line 220) Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you mean this: #17541 (comment) Let's get the types fixed. I don't think that argument affects passing | ||
| DataFile dataFile = TrackedFileAdapters.asDataFile(trackedFile, specsById); | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a heads-up from wiring the v4 read path through Spark's DSv2 scan, once a FileScanTask emitted by the v4 planner reaches Spark, its DataFile gets serialized to executors. The tracked-manifest read adapter that presents a v4 row as a DataFile needs to be Serializable for that to work. | ||
| TaskContext context = | ||
| taskContextsBySpec.computeIfAbsent(dataFile.specId(), this::newTaskContext); | ||
| DeleteFile[] deletes; | ||
| if (trackedFile.deletionVector() != null) { | ||
| deletes = new DeleteFile[] {TrackedFileAdapters.asDVDeleteFile(trackedFile, specsById)}; | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This conversion should already be done inside of the MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it should be inside
| ||
| } else { | ||
| deletes = 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 String rootManifestLocation; | ||
| private final Map<Integer, PartitionSpec> 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, | ||
| String rootManifestLocation, | ||
| Map<Integer, PartitionSpec> specsById, | ||
| String tableLocation) { | ||
| Preconditions.checkArgument(io != null, "Invalid file IO: null"); | ||
| Preconditions.checkArgument( | ||
| rootManifestLocation != null, "Invalid root manifest location: null"); | ||
| Preconditions.checkArgument(specsById != null, "Invalid specs by ID: null"); | ||
| Preconditions.checkArgument(tableLocation != null, "Invalid table location: null"); | ||
| this.io = io; | ||
| this.rootManifestLocation = rootManifestLocation; | ||
| 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) { | ||
anoopj marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| Preconditions.checkArgument(newExecutorService != null, "Invalid executor service: null"); | ||
| this.executorService = newExecutorService; | ||
| return this; | ||
| } | ||
| ScanTaskPlanner build() { | ||
| return new ScanTaskPlanner( | ||
| io, | ||
| rootManifestLocation, | ||
| specsById, | ||
| tableLocation, | ||
| dataFilter, | ||
| ignoreResiduals, | ||
| caseSensitive, | ||
| scanMetrics, | ||
| executorService); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -18,6 +18,7 @@ | ||
| */ | ||
| package org.apache.iceberg; | ||
| import java.nio.ByteBuffer; | ||
| import java.util.Arrays; | ||
| import java.util.Collection; | ||
| import java.util.Map; | ||
| @@ -185,6 +186,7 @@ static class Builder { | ||
| private Collection<String> columns = null; | ||
| private Schema requestedProjection = null; | ||
| private ScanMetrics scanMetrics = ScanMetrics.noop(); | ||
| private ByteBuffer manifestDv = null; | ||
| private Builder(InputFile file, Map<Integer, PartitionSpec> specsById, String tableLocation) { | ||
| Preconditions.checkArgument(tableLocation != null, "Invalid table location: null"); | ||
| @@ -258,7 +260,18 @@ Builder scanMetrics(ScanMetrics newScanMetrics) { | ||
| return this; | ||
| } | ||
| /** Sets the deletion vector that marks manifest entries deleted by position. */ | ||
| Builder manifestDv(ByteBuffer dv) { | ||
| this.manifestDv = dv; | ||
| return this; | ||
| } | ||
| V4ManifestReader build() { | ||
| if (manifestDv != null) { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the point of the changes in this file? I guess it's setting the stage for some follow-up change, but at this point I don't think it adds any value. MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Until we implement MDV, the rejection has to be done somewhere. The initial version of the PR had it done in the scan task planner: this is moving the rejection to the correct place (reader) | ||
| throw new UnsupportedOperationException( | ||
| "Cannot apply manifest deletion vector: " + file.location()); | ||
| } | ||
| Map<Integer, Pair<Evaluator, StructProjection>> partitionFilters = Maps.newHashMap(); | ||
| if (rowFilter != Expressions.alwaysTrue() && !unionPartitionType.fields().isEmpty()) { | ||
| for (PartitionSpec spec : specsById.values()) { | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for my benefit to see the bigger picture: Is this meant to replace the functionality in
SnapshotScan.doPlanFiles()and inManifestGroup.planFiles()and such? I'd like to understand how this fits into the flow that is executed when a user runstable.newScan().planFiles(). Is there going to be an if on the table version to branch which code path is run?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct. We will probably have a version specific branch in table scan layer. e.g.
DataTableScanThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I general comment about this class after taking a second look: I think this serves 2 different purposes merged into one.
TrackedFiles including the ones in the leaves.TrackedFileiterable we receive from the "root manifest reader"I'd bet that we want to read the manifest tree for multiple purposes not just to perform "scan task planning" so I'm wondering if we can split the root reader part into some other class that can be reused ?
Another general, maybe unrelated question is if want to still support
Snapshot'sallManifest(),dataManifests(),deleteManifests()functionality for V4.