stevenzwu commented on code in PR #17433:
URL: https://github.com/apache/iceberg/pull/17433#discussion_r3731312422
##########
core/src/main/java/org/apache/iceberg/V4ManifestReader.java:
##########
@@ -252,6 +272,25 @@ Builder project(Schema newProjection) {
return this;
}
+ /**
+ * Reads content stats for the given table field IDs instead of for every
field. Stats for
+ * fields referenced by the {@link #filter(Expression) filter} are always
read.
+ */
Review Comment:
nit: worth spelling out the null-vs-empty distinction contract. Something
like: *Call with no arguments or an empty iterable to opt out of stats reads
entirely; a `null` argument is rejected.* This applies symmetrically to the
`Iterable<Integer>` overload below.
##########
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 so to explore if we can have 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`)?
##########
core/src/main/java/org/apache/iceberg/util/StructLikeUtil.java:
##########
@@ -28,7 +29,7 @@ public static StructLike copy(StructLike struct) {
return StructCopy.copy(struct);
}
- private static class StructCopy implements StructLike {
+ private static class StructCopy implements StructLike, Serializable {
Review Comment:
is this needed for the geo bounding box stats (struct) round trip
serialization?
##########
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) {
Review Comment:
Is the asymmetry intended? A caller doing join/aggregate pushdown has to
explicitly narrow via `projectStats(...)`, but stats for row-filter-referenced
columns come along for free — without any opt-in from the caller — because the
default (non-scan-planning, no `projectStats`) reads full stats and the filter
only forces its refs to be included on top of a narrower projection.
The check itself (`statsProjectionForFieldIds != null`) is right —
`requiredStatsProjectionForFieldIds` may be non-empty due to the row filter,
but we only want to narrow when the caller explicitly asks via
`projectStats(fieldIds)`.
--
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]
