rdblue commented on code in PR #17433:
URL: https://github.com/apache/iceberg/pull/17433#discussion_r3779960418
##########
core/src/main/java/org/apache/iceberg/V4ManifestReader.java:
##########
@@ -292,17 +338,70 @@ 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, requiredStatsType,
hasPartitionFilter);
}
if (requestedProjection != null) {
- return addRequiredColumns(requestedProjection, hasPartitionFilter);
+ return addRequiredColumns(
+ fullSchema, requestedProjection, requiredStatsType,
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(Types.StructType contentStatsType) {
+ 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.
+ *
+ * <p>Stats for every field are read unless the caller narrows them with
{@link
+ * #forScanPlanning()} or {@link #projectStats(Iterable)}, because copying
entries into a new
+ * manifest needs all of them. A {@link #filter(Expression) filter}
therefore never narrows the
+ * stats that are read; it only widens a set the caller has already
narrowed.
+ */
+ private Types.StructType contentStatsType(Types.StructType
requiredStatsType) {
+ if (scanPlanning || statsProjectionForFieldIds != null) {
+ return requiredStatsType;
+ }
+
+ return StatsUtil.statsReadSchema(
+ tableSchema, TypeUtil.indexById(tableSchema.asStruct()).keySet());
+ }
+
+ /** Returns the table field IDs whose stats are read regardless of the
projection. */
+ private Set<Integer> requiredStatsProjectionForFieldIds() {
+ Set<Integer> fieldIds = Sets.newHashSet();
+ if (statsProjectionForFieldIds != null) {
+ fieldIds.addAll(statsProjectionForFieldIds);
+ }
+
+ if (rowFilter != Expressions.alwaysTrue()) {
+ // stats for filter references are read so that the filter can be
evaluated against them
Review Comment:
I don't think this comment is helping. It took me a minute to realize what
it's trying to say. It should be something more like "project stats needed to
evaluate the row filter". Or just omit it, I think it's pretty clear form
context what's happening here. Comments should add value or be omitted.
--
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]
