-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Core: Add v4 manifest reader #16958
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?
Core: Add v4 manifest reader #16958
Changes from all commits
08c7f59
ef3f3a1
f4f1f0c
ed345b8
719bc9a
48aa494
eb5b8dd
c61fd2c
88b31f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -238,9 +238,18 @@ public static StructType groupingKeyType(Schema schema, Collection<PartitionSpec | |
| * @return the constructed unified partition type | ||
| */ | ||
| public static StructType partitionType(Table table) { | ||
| Collection<PartitionSpec> specs = table.specs().values(); | ||
| return buildPartitionProjectionType( | ||
| "table partition", specs, allActiveFieldIds(table.schema(), specs)); | ||
| return partitionType(table.schema(), table.specs().values()); | ||
| } | ||
|
|
||
| /** | ||
| * Builds a unified partition type from a schema and its specs, unioning every partition field | ||
| * whose source column is present in the schema. | ||
| * | ||
| * @param schema the schema used to determine which partition fields are active | ||
| * @param specs the partition specs to unify | ||
| */ | ||
| static StructType partitionType(Schema schema, Collection<PartitionSpec> specs) { | ||
| return buildPartitionProjectionType("table partition", specs, allActiveFieldIds(schema, specs)); | ||
|
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 using That method will filter out partition fields when the source is no longer part of the table schema. In some cases (bucket joins), that is fine because the data is still partitioned by a subset of its partition fields. However, for a partition type that can hold any partition tuple, it doesn't work. Equality deletes are matched using partition tuple equality. For example, say I have With the use of This needs to union all partition fields so that we don't drop tuple values that define equality.
Member
Author
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. Thanks for pointing this out - I didn't think about this scenario. We can fix it by dropping the filter on active fields, but there might be another problem: partition specs don't store the type, and we derive it based on the type of the source column. So if the source column doesn't exist in the schema anymore, we can't derive the partition type. I think this might be working in v3 because of the way the reader is implemented: the reader gets the schema from the Avro schema, and uses it instead of the schema from the metadata. Maybe the Parquet reader can also work that way (can verify), but is this by design? Does the other Iceberg implementations actually work in this scenario in v3 if this detail is not in the spec?
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 a good point to think about, but I think it will not affect what we do in practice. It's possible to remove old partition specs, but in practice this hardly ever happens. In addition, the only cases where we can't determine the output type of the transform are identity and truncate transforms. Truncation isn't very common and identity columns are unlikely to be removed from the table. Because there are good reasons to think this case is unlikely, I think we should move forward assuming that the partition specs will be there. If we want to be more careful, we can require a metadata check to remove old specs to avoid breaking anything. Also, we can always go back and get the type from existing metadata files at read time if we can't determine the type. I've also thought about storing the output type of transforms when we add column ranges for partition output values, like we do for UDF result types. We may just need to solve this by storing the output type.
Member
Author
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. Fixed. I agree the output type is only underivable for identity and truncate, but BTW +1 to storing the output type in metadata. v4 seems like the right time to add it since we're already revising the spec. It seems like a good overall improvement. Happy to write that up. |
||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,282 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.iceberg; | ||
|
|
||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
| import org.apache.iceberg.expressions.Evaluator; | ||
| import org.apache.iceberg.expressions.Expression; | ||
| import org.apache.iceberg.expressions.Expressions; | ||
| import org.apache.iceberg.expressions.Projections; | ||
| import org.apache.iceberg.io.CloseableGroup; | ||
| import org.apache.iceberg.io.CloseableIterable; | ||
| import org.apache.iceberg.io.CloseableIterator; | ||
| import org.apache.iceberg.io.InputFile; | ||
| import org.apache.iceberg.metrics.ScanMetrics; | ||
| import org.apache.iceberg.relocated.com.google.common.base.Preconditions; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.Lists; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.Maps; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.Sets; | ||
| import org.apache.iceberg.types.Types; | ||
| import org.apache.iceberg.util.StructProjection; | ||
|
|
||
| /** Reader that reads a v4+ manifest file as {@link TrackedFile}s. */ | ||
| class V4ManifestReader extends CloseableGroup implements CloseableIterable<TrackedFile> { | ||
|
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 name
Member
Author
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.
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 class implements and returns a
Member
Author
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. That is actually a good point. Leaving it open to hear from others.
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.
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 a good point, but I don't have a name suggestion that is better right now. Let's keep it in mind. |
||
| // Tracking fields read on the scan path. Omits the change-tracking fields (dv_snapshot_id, | ||
| // deleted_positions, replaced_positions) that a scan does not need. row_position backs | ||
| // Tracking.manifestPos. | ||
| private static final Types.StructType SCAN_TRACKING = | ||
| Types.StructType.of( | ||
| Tracking.STATUS, | ||
| Tracking.SNAPSHOT_ID, | ||
| Tracking.SEQUENCE_NUMBER, | ||
| Tracking.FILE_SEQUENCE_NUMBER, | ||
| Tracking.FIRST_ROW_ID, | ||
| MetadataColumns.ROW_POSITION); | ||
|
|
||
| private final InputFile file; | ||
| private final Types.StructType partitionType; | ||
| private final Schema fileProjection; | ||
| private final ScanMetrics scanMetrics; | ||
|
|
||
| // partition pruning state, keyed by spec ID; empty when no filtering is required | ||
|
anoopj marked this conversation as resolved.
|
||
| private final Map<Integer, Evaluator> partitionEvaluators; | ||
| private final Map<Integer, StructProjection> partitionProjections; | ||
|
|
||
| private V4ManifestReader( | ||
| InputFile file, | ||
| Types.StructType partitionType, | ||
| Map<Integer, Evaluator> partitionEvaluators, | ||
| Map<Integer, StructProjection> partitionProjections, | ||
| Schema fileProjection, | ||
| ScanMetrics scanMetrics) { | ||
| this.file = file; | ||
| this.partitionType = partitionType; | ||
| this.partitionEvaluators = partitionEvaluators; | ||
| this.partitionProjections = partitionProjections; | ||
| this.fileProjection = fileProjection; | ||
| this.scanMetrics = scanMetrics; | ||
| } | ||
|
|
||
| static Builder builder( | ||
| InputFile file, Schema tableSchema, Map<Integer, PartitionSpec> specsById) { | ||
| return new Builder(file, tableSchema, specsById); | ||
| } | ||
|
|
||
| /** Returns all tracked files in this manifest, regardless of status. */ | ||
| CloseableIterable<TrackedFile> allFiles() { | ||
| return files(false /* all files */); | ||
| } | ||
|
|
||
| /** Returns tracked files whose tracking {@link Tracking#isLive() is live}. */ | ||
| CloseableIterable<TrackedFile> liveFiles() { | ||
|
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. For the original
Member
Author
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. Done. I was following the original |
||
| return files(true /* only live files */); | ||
| } | ||
|
|
||
| /** Returns live tracked files. Makes defensive copies before returning. */ | ||
| @Override | ||
| public CloseableIterator<TrackedFile> iterator() { | ||
| return CloseableIterable.transform(liveFiles(), TrackedFile::copy).iterator(); | ||
| } | ||
|
|
||
| private CloseableIterable<TrackedFile> files(boolean onlyLive) { | ||
| CloseableIterable<TrackedFile> entries = CloseableIterable.transform(open(), this::prepare); | ||
| if (!partitionEvaluators.isEmpty()) { | ||
| entries = CloseableIterable.filter(entries, this::matchesPartition); | ||
| } | ||
|
|
||
| if (onlyLive) { | ||
| entries = CloseableIterable.filter(entries, entry -> entry.tracking().isLive()); | ||
| } | ||
|
|
||
| return entries; | ||
| } | ||
|
|
||
| private boolean matchesPartition(TrackedFile trackedFile) { | ||
|
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 thought we want to drop tuple-based pruning entirely and push predicates against partition-transform-expression columns in I am planning to bring up the discussion in Monday's sync.
Member
Author
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. As a followup of today's discussion, we will keep the partition tuple for planning/pruning for now and we can move to content stats based pruning later. This implementation in this PR is purely a pruning optimization, so swapping/removing it has no correctness implications. |
||
| FileContent content = trackedFile.contentType(); | ||
| if (content == FileContent.DATA_MANIFEST || content == FileContent.DELETE_MANIFEST) { | ||
| // manifest references are expanded later and are not pruned by the partition filter | ||
| return true; | ||
| } | ||
|
|
||
| Integer specId = trackedFile.specId(); | ||
| Evaluator evaluator = specId != null ? partitionEvaluators.get(specId) : null; | ||
| StructProjection projection = specId != null ? partitionProjections.get(specId) : null; | ||
| Preconditions.checkState( | ||
|
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 this partition filtering:
Member
Author
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 a table goes from unpartitioned, to partitioned, new files also can be written in the unpartitioned space - this is a supported use case. So an unpartitioned file should not throw. Instead, it should not get filtered out by partition filtering, so that filters should apply to rows instead. e.g. table is partitioned by a
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. My point was that specId = null is a valid state, but we throw an exception here because of that because both the evaluator and the projection will be null.
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. The specID is a required field, so it should not be null. Even unpartitioned data has a spec (the unpartitioned spec). Also, any spec that exists within the table metadata is valid and can be written to directly (it isn't limited to older data written and then spec being evolved).
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.
Hence the evaluation and preconditions check seem to only apply to data file entries in a leaf manifest 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.
I get the point, we return early for leaf manifests so here we only have data/delete entries where we expect Since the field is optional, shouldn't we have an extra Preconditions check to verify that?
Member
Author
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 the spec needs to be a bit clear on this. I have a slight preference to make it optional for data/delete files as well, for truly unpartitioned cases. I will leave this thread open till we resolve this. |
||
| evaluator != null && projection != null, | ||
| "Cannot apply partition filter: spec ID %s is not one of the known specs %s in manifest %s", | ||
| specId, | ||
| partitionEvaluators.keySet(), | ||
| file.location()); | ||
|
|
||
| boolean matches = evaluator.eval(projection.wrap(trackedFile.partition())); | ||
| if (!matches) { | ||
|
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. Can we use the Maybe constructing a predicate for partition filter and use
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. Taking a second look, for this probably we should know the contentType beforehand, but it depends on the TrackedFile entry.
Member
Author
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, your second comment is correct. A v4 manifest can mix data files, delete files, and manifest references in one file. So the counter depends on each entry's contentType and can only be selected inside the predicate. The current code is quite simple to undersand.
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 hope #17118 can help here where filter can now provide onKeep/onSkip callback and counters can be configured directly in the callback with more flexibility for the heterogeneous entry of TrackedFile |
||
| if (content == FileContent.DATA) { | ||
|
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 about manifests? |
||
| scanMetrics.skippedDataFiles().increment(); | ||
| } else { | ||
| scanMetrics.skippedDeleteFiles().increment(); | ||
| } | ||
| } | ||
|
|
||
| return matches; | ||
| } | ||
|
|
||
| private CloseableIterable<TrackedFile> open() { | ||
|
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. firstRowId is not supported in this version?
Member
Author
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, row lineage isn't wired up in this reader yet. |
||
| FileFormat format = FileFormat.fromFileName(file.location()); | ||
| Preconditions.checkArgument( | ||
| format != null, "Cannot determine format of manifest: %s", file.location()); | ||
|
|
||
| CloseableIterable<TrackedFile> reader = | ||
| InternalData.read(format, file) | ||
| .project(readSchema()) | ||
|
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're missing CONTENT_TYPE, which always needs to be projected due to the
Member
Author
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. That makes sense. Added content_type as always projected. |
||
| .setRootType(TrackedFileStruct.class) | ||
| .setCustomType(TrackedFile.TRACKING.fieldId(), TrackingStruct.class) | ||
|
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. nit: maybe a TODO that ContentStats is not covered now?
Member
Author
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. Generally we don't add many TODOs in the code (I got past feedback on this).
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. Yeah, not for things that are currently being worked on. The TODO should not catch anything. Tests should catch when something is missing. |
||
| .setCustomType(TrackedFile.DELETION_VECTOR.fieldId(), DeletionVectorStruct.class) | ||
| .setCustomType(TrackedFile.MANIFEST_INFO.fieldId(), ManifestInfoStruct.class) | ||
| .setCustomType(TrackedFile.PARTITION_ID, PartitionData.class) | ||
| .reuseContainers() | ||
| .build(); | ||
| addCloseable(reader); | ||
| return reader; | ||
| } | ||
|
|
||
| private TrackedFile prepare(TrackedFile trackedFile) { | ||
| Tracking tracking = trackedFile.tracking(); | ||
| // manifestLocation is not stored in the manifest; the reader fills it from the file location. | ||
| // manifestPos is filled from ROW_POSITION while reading the tracking struct. | ||
| if (tracking instanceof TrackingStruct) { | ||
| ((TrackingStruct) tracking).setManifestLocation(file.location()); | ||
|
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 populate
Member
Author
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. They look symmetric but the underlying mechanism differs. ROW_POSITION is synthesized by the reader itself. FILE_PATH us only populated when the caller injects it. I'd lean toward keeping the manual set for now
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 guess the difficulty here is that for InternalData API we don't have a way to pass the constants map to the underlying reader, right? I wonder what others say. Probably is an overkill at this point.
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.
|
||
| } | ||
|
|
||
| return trackedFile; | ||
| } | ||
|
|
||
| private Schema readSchema() { | ||
|
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 would expect the builder to handle schema projection. Why do this here instead of in the builder? |
||
| Types.StructType fullType = | ||
| TrackedFile.schemaWithContentStats(partitionType, Types.StructType.of()); | ||
| boolean unpartitioned = partitionType.fields().isEmpty(); | ||
|
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. Shouldn't an empty partition type already be handled by the schema generation in the previous statement? |
||
|
|
||
| Set<Integer> projectedIds = null; | ||
| if (fileProjection != null) { | ||
| projectedIds = | ||
| fileProjection.asStruct().fields().stream() | ||
| .map(Types.NestedField::fieldId) | ||
| .collect(Collectors.toCollection(Sets::newHashSet)); | ||
|
|
||
| // Always project tracking and content type. status drives live-file filtering, and content | ||
| // type distinguishes data, delete, and manifest-reference entries. | ||
| projectedIds.add(TrackedFile.TRACKING.fieldId()); | ||
|
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 there anything else needed from tracking? Not exactly sure how much projection gives us, but can't we project TRACKING.STATUS.fieldId() if nothing else is needed?
Member
Author
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 only rely on status now, and projecting only that.
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 are wrong here with adding only the
Member
Author
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, please see my comment above. |
||
| projectedIds.add(TrackedFile.CONTENT_TYPE.fieldId()); | ||
|
|
||
| // Force-project the remaining fields the partition filter reads, regardless of caller | ||
| // projection. | ||
| if (!partitionEvaluators.isEmpty()) { | ||
|
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.
Same reasoning as
Member
Author
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. Good catch, fixed such that we will be force-projecting partition (alongside spec_id) when a filter is active. Also added testPartitionFilterForceProjectsFilterFields() as a test |
||
| projectedIds.add(TrackedFile.SPEC_ID.fieldId()); | ||
| projectedIds.add(TrackedFile.PARTITION_ID); | ||
| } | ||
| } | ||
|
|
||
| List<Types.NestedField> fields = Lists.newArrayList(); | ||
| for (Types.NestedField field : fullType.fields()) { | ||
|
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. Why is this performing its own projection rather than using
Member
Author
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. Didn't know about |
||
| if (projectedIds != null && !projectedIds.contains(field.fieldId())) { | ||
| continue; | ||
| } | ||
|
|
||
| if (field.fieldId() == TrackedFile.TRACKING.fieldId()) { | ||
| fields.add( | ||
| Types.NestedField.required( | ||
| TrackedFile.TRACKING.fieldId(), | ||
| TrackedFile.TRACKING.name(), | ||
| SCAN_TRACKING, | ||
| TrackedFile.TRACKING.doc())); | ||
| } else if (field.fieldId() == TrackedFile.CONTENT_STATS_ID) { | ||
|
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 branch is because temporarily we don't read stats. I'm wondering what it would take to avoid this branch and read the stats even if we don't use them.
Member
Author
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 intent here is actually to avoid reading stats we don't use.
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. Thanks for the explanation! Projecting stats makes total sense. I think most of my comment on this area are for a single reason: for me this readSchema() function seems a bit more complicated than what it is for. What I have in mind is like this:
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 not convinced this is how we'll be projecting stats from the read path. Ideally, we wouldn't project the whole stats field (and that's just the containing field id). What we should have is the specific field ids for the individual status required to evaluate the filter.
Member
Author
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 do agree: we will be projecting only the field IDs we need. This is just a placeholder for now, as the interfaces for content stats are being revamped. |
||
| // content_stats are not projected yet | ||
| } else if (field.fieldId() == TrackedFile.PARTITION_ID && unpartitioned) { | ||
|
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 can still read the partition even if unpartitioned. As we discussed lately, it will be null. I don't think this branch of the
Member
Author
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 branch is intentional: when the table is fully unpartitioned there's no partition tuple, so we just don't project the field. Happy to switch to that if we want null per #17000, but the explicit omit is clearer here. |
||
| // unpartitioned manifests omit the partition field | ||
| } else { | ||
| fields.add(field); | ||
| } | ||
| } | ||
|
|
||
| return new Schema(fields); | ||
| } | ||
|
|
||
| static class Builder { | ||
| private final InputFile file; | ||
| private final Types.StructType partitionType; | ||
| private final Map<Integer, PartitionSpec> specsById; | ||
| private Expression rowFilter = Expressions.alwaysTrue(); | ||
| private boolean caseSensitive = true; | ||
| private Schema fileProjection = null; | ||
| private ScanMetrics scanMetrics = ScanMetrics.noop(); | ||
|
|
||
| private Builder(InputFile file, Schema tableSchema, Map<Integer, PartitionSpec> specsById) { | ||
| this.file = file; | ||
| this.partitionType = Partitioning.partitionType(tableSchema, specsById.values()); | ||
| this.specsById = specsById; | ||
| } | ||
|
|
||
| /** Sets a row filter; files that cannot match the expression are skipped. */ | ||
| Builder filterRows(Expression expr) { | ||
| Preconditions.checkArgument(expr != null, "Invalid row filter: null"); | ||
| this.rowFilter = expr; | ||
| return this; | ||
| } | ||
|
|
||
| Builder caseSensitive(boolean isCaseSensitive) { | ||
| this.caseSensitive = isCaseSensitive; | ||
| return this; | ||
| } | ||
|
|
||
| Builder project(Schema newFileProjection) { | ||
| this.fileProjection = newFileProjection; | ||
|
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. nit: precondition for != null?
Member
Author
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 intentional. Null projection means read all columns, and it's a supported case. This is the current semantics of the existing manifest reader as well. |
||
| return this; | ||
| } | ||
|
|
||
| Builder scanMetrics(ScanMetrics newScanMetrics) { | ||
| Preconditions.checkArgument(newScanMetrics != null, "Invalid scan metrics: null"); | ||
| this.scanMetrics = newScanMetrics; | ||
| return this; | ||
| } | ||
|
|
||
| V4ManifestReader build() { | ||
| Map<Integer, Evaluator> partitionEvaluators = Maps.newHashMap(); | ||
| Map<Integer, StructProjection> partitionProjections = Maps.newHashMap(); | ||
| if (rowFilter != Expressions.alwaysTrue() && !partitionType.fields().isEmpty()) { | ||
| for (PartitionSpec spec : specsById.values()) { | ||
| Expression partFilter = Projections.inclusive(spec, caseSensitive).project(rowFilter); | ||
| partitionEvaluators.put( | ||
| spec.specId(), new Evaluator(spec.partitionType(), partFilter, caseSensitive)); | ||
| partitionProjections.put( | ||
| spec.specId(), StructProjection.create(partitionType, spec.partitionType())); | ||
| } | ||
| } | ||
|
|
||
| return new V4ManifestReader( | ||
| file, | ||
| partitionType, | ||
| partitionEvaluators, | ||
| partitionProjections, | ||
| fileProjection, | ||
| scanMetrics); | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.