szehon-ho commented on code in PR #17451:
URL: https://github.com/apache/iceberg/pull/17451#discussion_r3834894268
##########
api/src/main/java/org/apache/iceberg/ContentFile.java:
##########
@@ -99,6 +99,14 @@ default String location() {
/** Returns if collected, map from column ID to value upper bounds, null
otherwise. */
Map<Integer, ByteBuffer> upperBounds();
+ /**
+ * Returns if collected, map from column ID to its average non-null value
size in bytes, null
+ * otherwise.
Review Comment:
```suggestion
* Returns if collected, map from column ID to its average value size in
memory (uncompressed)
* in bytes over non-null values, null otherwise.
```
Matches how the spec defines `avg_value_size_in_bytes`. Without the "in
memory (uncompressed)" qualifier this reads as the encoded size, which happens
to coincide for geospatial WKB but won't once a compressible type like `string`
tracks it.
##########
api/src/main/java/org/apache/iceberg/Metrics.java:
##########
@@ -204,6 +244,15 @@ public Map<Integer, ByteBuffer> upperBounds() {
return upperBounds;
}
+ /**
+ * Get the average non-null value size in bytes for all fields where it was
collected.
Review Comment:
```suggestion
* Get the average value size in memory (uncompressed) in bytes over
non-null values, for all
* fields where it was collected.
```
Same as the `ContentFile` javadoc — the spec defines this as the in-memory
uncompressed size.
##########
core/src/test/java/org/apache/iceberg/StatsTestUtil.java:
##########
@@ -207,6 +220,8 @@ static FieldStats<Object> mockFieldStats(
Mockito.when(stats.nanValueCount()).thenReturn(nanCount);
}
+ Mockito.when(stats.avgValueSizeInBytes()).thenReturn(avgValueSize);
Review Comment:
```suggestion
if (avgValueSize != null) {
Mockito.when(stats.avgValueSizeInBytes()).thenReturn(avgValueSize);
}
```
Matches the guarded stubbing for `nanCount` just above, which leaves the
mock's default in place when the stat is absent.
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetMetrics.java:
##########
@@ -172,6 +177,7 @@ static Metrics metrics(
nanValueCounts,
lowerBounds,
upperBounds,
+ avgValueSizes,
Review Comment:
```suggestion
avgValueSizes.isEmpty() ? null : avgValueSizes,
```
`avgValueSizes` is created eagerly, so every Parquet file without a
geospatial column carries an empty map rather than null.
`ContentFile.avgValueSizes()` documents "null otherwise", and
`ContentStatsBackedMap.avgValueSizes()` returns null through `viewOrNull` when
no column tracks the stat, so a v4 consumer that null-checks gets a different
answer depending on whether the file came from the writer or from a manifest.
##########
core/src/main/java/org/apache/iceberg/MetricsUtil.java:
##########
@@ -56,13 +57,15 @@ public static Metrics copyWithoutFieldCounts(Metrics
metrics, Set<Integer> exclu
copyWithoutKeys(metrics.nanValueCounts(), excludedFieldIds),
metrics.lowerBounds(),
metrics.upperBounds(),
+ copyWithoutKeys(metrics.avgValueSizes(), excludedFieldIds),
Review Comment:
Worth adding a `TestMetricsUtil` case covering this and the matching line in
`copyWithoutFieldCountsAndBounds`. Nothing asserts the new drop today, and
`PositionDeleteWriter`'s excluded columns are `file_path` and `pos`, so no avg
size is ever present there yet — the line stays unreachable until
`StringWriter` starts emitting `ValueSizeFieldMetrics`.
--
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]
