stevenzwu commented on code in PR #17433:
URL: https://github.com/apache/iceberg/pull/17433#discussion_r3737268186
##########
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:
Idea worth exploring: version `MetricsConfig` in `TableMetadata` alongside
schemas and partition specs, and stamp each manifest file (root or leaf) with
the `metrics-config-id` in effect when the manifest was written. Readers
resolve the id and recover the exact write-time stats field IDs, and build the
read schema off the intersection with `tableSchema`. Correct across config
drift; no extra file peek at read time.
Trade-offs: spec change (`metrics-configs` list +
`current-metrics-config-id` on `TableMetadata`, `MetricsConfigParser`,
`metrics-config-id` on manifest file entries); `MetricsConfig` shifts from a
runtime property derivation to a first-class immutable artifact;
retention/dedup matters since property tweaks churn more than schema/spec
changes. With `metrics-config-id`, `schema-id`, `sort-id`, reader should be
able to faithfully reconstruct the writer `MetricsConfig`. This also requires
move the metrics config from table properties to properly versioned struct in
table metadata.
Alternatives — field-IDs list in each manifest's header, or on the
manifest-list entry — either force a pre-read I/O per manifest or bloat the
manifest list on wide tables, so the versioned direction seems the cleanest
long-term shape.
--
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]
