stevenzwu commented on code in PR #17433:
URL: https://github.com/apache/iceberg/pull/17433#discussion_r3760795350
##########
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:
@rdblue clarified the expected behavior, which makes sense to me.
There are two read modes
1. caller explicitly project column stats (for filter, join etc.). This is
already covered by the PR.
2. caller just want to read all existing column stats (for manifest update
carryover). Currently, we construct the content stats from all table schema
fields. Ryan was suggesting that we should just use the current MetricsConfig
to construct the read schema for content_stats. We only want to carry over
stats based on the latest metrics config.
We probably should rename the two APIs from `StatsUtil` to clarify their
purpose.
- `statsWriteSchema` -> `currentStatsSchema`
- `statsReadSchema` -> `statsProjectionSchema` // we might be able to leave
this unchanged too.
--
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]
