rdblue commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786436321
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueWriters.java:
##########
@@ -494,9 +515,31 @@ public Stream<FieldMetrics<?>> metrics() {
}
}
- // skipping updating null stats for non-primitive types since we don't
use them today, to
- // avoid unnecessary work
- return writer.metrics();
+ // A null value here is also null for every descendant column, but those
columns are written
+ // directly and never see it, so their writers cannot count it. Add it
to the metrics of
+ // optional descendants; null counts are not tracked for required fields.
+ return writer
+ .metrics()
+ .map(
+ metrics ->
+ optionalFieldIds.contains(metrics.id()) ?
addNullValues(metrics) : metrics);
Review Comment:
I don't think this is correct. I think we want to update the null count
regardless of whether the inner field is required or optional. The only case
where we could omit the null count is when the field and all of its parents are
required because it won't be accessed.
That means that the leaf's required/optional status doesn't matter. Any
metrics that pass through this level should be updated.
--
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]
