stevenzwu commented on code in PR #17433:
URL: https://github.com/apache/iceberg/pull/17433#discussion_r3732620942
##########
core/src/main/java/org/apache/iceberg/V4ManifestReader.java:
##########
@@ -292,17 +333,65 @@ private Schema readSchema(boolean hasPartitionFilter) {
if (columns != null) {
Schema selected =
caseSensitive ? fullSchema.select(columns) :
fullSchema.caseInsensitiveSelect(columns);
- return addRequiredColumns(selected, hasPartitionFilter);
+ return addRequiredColumns(fullSchema, selected, requiredFieldIds,
hasPartitionFilter);
}
if (requestedProjection != null) {
- return addRequiredColumns(requestedProjection, hasPartitionFilter);
+ return addRequiredColumns(
+ fullSchema, requestedProjection, requiredFieldIds,
hasPartitionFilter);
}
return fullSchema;
}
- private Schema addRequiredColumns(Schema projection, boolean
hasPartitionFilter) {
+ /** Returns the schema of everything this reader may read, including
content stats. */
+ private Schema fullSchema(Set<Integer> requiredStatsProjectionFieldIds) {
+ Types.StructType contentStatsType =
contentStatsType(requiredStatsProjectionFieldIds);
+ Schema base = TrackedFile.schema(unionPartitionType, contentStatsType);
+ if (contentStatsType.fields().isEmpty()) {
+ // schema uses the unknown type for empty stats, which cannot be
paired with the stats
+ // struct in the manifest, so drop the field instead of reading it as
unknown
+ base = TypeUtil.selectNot(base,
ImmutableSet.of(TrackedFile.CONTENT_STATS_ID));
+ }
+
+ // the read schema carries row_position (via BASE_TYPE) so the reader
can fill manifestPos
+ return TypeUtil.replaceFieldTypes(
+ base, ImmutableMap.of(TrackedFile.TRACKING.fieldId(),
TrackingStruct.BASE_TYPE));
+ }
+
+ /** Returns the stats type to read, which is empty when no stats are
needed. */
+ private Types.StructType contentStatsType(Set<Integer>
requiredStatsProjectionForFieldIds) {
+ if (scanPlanning || statsProjectionForFieldIds != null) {
+ // scan planning and projectStats(fieldIds) both narrow the set of
stats that are read
+ return StatsUtil.statsReadSchema(tableSchema,
requiredStatsProjectionForFieldIds);
+ }
+
+ return StatsUtil.statsReadSchema(
+ tableSchema, TypeUtil.indexById(tableSchema.asStruct()).keySet());
Review Comment:
This walks the full table schema for every `build()` — `TypeUtil.indexById`
once, then `statsReadSchema` walks again (plus `indexParents` and per-field
`isScalar` climbs to the root). Fine per manifest, but this is on the default
path (no `projectStats`, no `forScanPlanning`) taken for every manifest read
that copies entries forward, so the cost multiplies across a scan's fan-out on
wide tables.
Compounding this: manifests only store stats for a capped prefix of columns
(default ~100 via `MetricsConfig`), so on a table with e.g. 5,000 columns the
default "read all stats" builds a stats schema with ~5,000 slots and registers
5,000 `FieldStatsStruct` custom types — but ~4,900 of them resolve to null at
decode time because the manifest never stored them. We're paying construction
cost for stats we know aren't there.
But I don't have a good solution. Neither option below is clean:
- Using current `MetricsConfig.metricsFieldIds()` at read time is per-table,
not per-manifest. If the cap narrowed since the manifest was written, we
silently drop stats the manifest actually holds — no correctness impact
(`InclusiveMetricsEvaluator` treats absent stats as "may match"), but pruning
gets coarser on copy-forward and scans open more files at query time. If it
widened, we still over-ask for the extra columns and get the same
null-resolution waste. Not a sound signal either way.
- The only truthful source is the manifest's own `content_stats` schema. But
peeking at that before configuring the projection means either an extra file
open per manifest (drop below `InternalData` to `Avro.read`/`Parquet.read` for
a header/footer peek, then reopen via `InternalData` with the intersection), or
extending `InternalData.ReadBuilder` with a `fileSchema()` accessor so the
projection can be picked after the header is read. Both cost something.
Flagging this to see if we can explore good alternatives — not blocking this
PR.
And orthogonally, I am also wondering if we should caching the full stats
read schema keyed off the `Schema` (like `Schema.lazyIdToField`)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
