stevenzwu commented on code in PR #18310:
URL: https://github.com/apache/iceberg/pull/18310#discussion_r4150403784
##########
api/src/main/java/org/apache/iceberg/ContentFile.java:
##########
@@ -198,7 +198,7 @@ default Long firstRowId() {
* to copy data without stats when collecting files.
*
* @return a copy of this data file, without lower bounds, upper bounds,
value counts, null value
- * counts, nan value counts, or average value sizes
+ * counts, nan value counts, or total bytes
Review Comment:
I am wondering if we should stop enumerating every metric. just say `a copy
of this data file without column stats`?
Same for two other Javadoc comments in this file.
##########
.palantir/revapi.yml:
##########
@@ -674,7 +674,7 @@ acceptedBreaks:
\ be implemented outside the project. Promoting them to public reads
as an\
\ existing type gaining methods, not as a new type appearing."
- code: "java.method.addedToInterface"
- new: "method java.lang.Integer
org.apache.iceberg.FieldStats<T>::avgValueSizeInBytes()"
+ new: "method java.lang.Long
org.apache.iceberg.FieldStats<T>::totalBytes()"
Review Comment:
This swap covers `FieldStats.totalBytes()`, but CI is still failing on the
1.12.0 removals of `ContentFile.avgValueSizes()` and `Metrics.avgValueSizes()`
(`java.method.removed`, source + binary). We probably need to add
`java.method.removed` entries for those two.
##########
api/src/main/java/org/apache/iceberg/ContentFile.java:
##########
@@ -100,13 +100,13 @@ default String location() {
Map<Integer, ByteBuffer> upperBounds();
/**
- * Returns if collected, map from column ID to its average value size in
memory (uncompressed) in
- * bytes over non-null values, null otherwise.
+ * Returns if collected, map from column ID to the total uncompressed size
in memory in bytes of
Review Comment:
I would move the `if collected` next to `, null otherwise`. like
```
Returns map from column ID to the total uncompressed size in memory in bytes
of
non-null values if collected, null otherwise.
```
##########
core/src/main/java/org/apache/iceberg/GeometryFieldMetrics.java:
##########
@@ -69,15 +69,14 @@ public void addValue(ByteBuffer wkb) {
public GeometryFieldMetrics build() {
// build() returns null bounds when a dimension has no value or a value
could not be bounded,
- // which leaves the field with counts but no bounds. The average size is
still reported: it
- // does not depend on whether a box could be formed.
+ // which leaves the field with counts but no bounds. The total is still
reported and does not
Review Comment:
+1. maybe "the total bytes" here.
##########
api/src/main/java/org/apache/iceberg/Metrics.java:
##########
@@ -150,8 +150,8 @@ public Metrics(
* @param nanValueCounts a map of field id to the number of NaN values, or
null if unknown
* @param lowerBounds a map of field id to the lower bound of the column, or
null if unknown
* @param upperBounds a map of field id to the upper bound of the column, or
null if unknown
- * @param avgValueSizes a map of field id to the average size in bytes of
the column's non-null
- * values, or null if unknown
+ * @param totalBytes a map of field id to the total uncompressed size in
bytes of the column's
+ * non-null values, or null if unknown
Review Comment:
nit: `, or null if not collected` to be consistent with another Javadoc
earlier?
##########
core/src/main/java/org/apache/iceberg/MetricsUtil.java:
##########
@@ -57,14 +56,14 @@ public static Metrics copyWithoutFieldCounts(Metrics
metrics, Set<Integer> exclu
copyWithoutKeys(metrics.nanValueCounts(), excludedFieldIds),
metrics.lowerBounds(),
metrics.upperBounds(),
- copyWithoutKeys(metrics.avgValueSizes(), excludedFieldIds),
+ copyWithoutKeys(metrics.totalBytes(), excludedFieldIds),
metrics.originalTypes());
}
/**
- * Copies a metrics object without counts, average value sizes, and bounds
for given fields.
+ * Copies a metrics object without counts, total bytes, and bounds for given
fields.
*
- * @param excludedFieldIds field IDs for which the counts, average value
sizes, and bounds must be
+ * @param excludedFieldIds field IDs for which the counts, total bytes, and
bounds must be dropped
* dropped
Review Comment:
left a duplicate `dropped` word here.
##########
core/src/test/java/org/apache/iceberg/TestFieldStatsStruct.java:
##########
@@ -357,14 +357,15 @@ public void
variantSerialization(RoundTripSerializer<FieldStatsStruct<?>> serial
int size = metadata.dictionarySize() + lowerObject.sizeInBytes();
Review Comment:
nit: just define `long size` to do one type cast.
--
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]
