uros-b commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3743403240
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetValueWriters.java:
##########
@@ -60,4 +68,117 @@ void geospatialValueSizeMetricsExcludeNulls() {
assertThat(metrics.nullValueCount()).isEqualTo(1);
assertThat(metrics.avgValueSizeInBytes()).isEqualTo(31);
}
+
Review Comment:
All four new tests assert only at the writer.metrics() boundary via a mocked
ColumnWriteStore; none pins the corrected count at the user-visible end of the
claim, that it reaches DataFile.nullValueCounts(). The seam is load-bearing:
ParquetMetrics.primitive() (lines 242–248) prefers writer metrics and only
falls through to the footer when absent, and ParquetMetrics.metrics() line 146
filters on nullValueCount() >= 0 before the value lands in the manifest. A
future refactor of either could silently drop this fix with all four
writer-level tests still green. There is an existing harness for exactly this
(TestMetrics/TestParquetMetrics with assertCounts(fieldId, valueCount,
nullValueCount, metrics)), but the existing NESTED_SCHEMA uses required(2,
"nestedStructCol", ...), so a new test case with an optional struct containing
a double leaf (or float, or geo) is needed. One added case in that harness
closes the gap.
--
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]
