Core, Parquet: Carry avg value sizes for v4 content stats by huan233usc · Pull Request #17451 · apache/iceberg

huan233usc · 2026-07-31T14:49:16Z

Follow-up to #17333.

#17333 collects the average serialized WKB size in FieldMetrics, but ParquetMetrics discarded it while assembling Metrics, so the value could never reach a DataFile or the v4 content_stats adapters. This PR closes that gap, carrying per-field average non-null value sizes through Metrics, ContentFile, the file builders, copies, and filtering, and exposing them through the legacy ContentFile view that v4 manifest readers use.

The v1-v3 manifest schemas are unchanged, so v3 manifests do not persist this optional metric. The v4 write-direction wrapper in #16936 can consume ContentFile.avgValueSizes() after rebasing.

ContentFile.avgValueSizes() is a default method returning null. Adding an abstract method to ContentFile would break the public API and ABI (RevAPI reports java.method.addedToInterface), and implementations that carry no column stats, such as the deletion-vector adapter, correctly inherit null. Metrics also keeps its unpinned serialVersionUID: no type holds a Metrics field, so it is only ever serialized and deserialized within a single version, and no cross-version shim is needed.

Tests:

./gradlew :iceberg-api:test --tests org.apache.iceberg.TestMetricsSerialization
./gradlew :iceberg-core:test --tests org.apache.iceberg.TestContentStatsBackedMap --tests org.apache.iceberg.TestTrackedFileAdapters
./gradlew :iceberg-data:test --tests org.apache.iceberg.parquet.TestParquetMetrics.testMetricsForGeospatialTypes
./gradlew :iceberg-parquet:test --tests org.apache.iceberg.parquet.TestParquetDataWriter.testGeospatialRoundTrip
./gradlew :iceberg-api:revapi :iceberg-core:revapi :iceberg-parquet:revapi

Propagate average non-null value sizes from Parquet metrics through Metrics and ContentFile so v4 content stats adapters can preserve them. The v1-v3 manifest schemas are unchanged, so v3 manifests do not persist this optional metric. No cross-version Java-serialization shim is added: no type holds a Metrics field, so Metrics is only ever serialized and deserialized within a single version.

szehon-ho

The change is well-scoped and mechanically consistent — every Metrics producer, file builder, copy path, and delegating wrapper is updated, and TrackedDVDeleteFile correctly inherits the null default by extending TrackedFileAdapter rather than TrackedContentFile. One thing worth fixing before merge (the empty-map contract in ParquetMetrics), plus a few small notes inline.

Two broader points, neither blocking:

Is ContentFile.avgValueSizes() needed in this PR? api/ has the strongest stability guarantees in the project and nothing in the tree reads the accessor yet — V4Metadata.DataFileWrapper still projects the legacy data_file struct with no content_stats field, so the only consumer is #16936. If that wrapper can read the stat from Metrics, or from a ContentStats held by GenericDataFile, the interface addition is avoidable. If it genuinely needs the accessor, worth saying so, since the method is permanent once released.

Separately, Metrics is now up to nine positional parameters with eight maps, and this PR has to pass null /* avgValueSizes */ and null /* originalTypes */ at three call sites. Fine to defer, but whenever the next stat lands here it would be a good moment to switch to a builder.

One doc suggestion beyond the inline notes: it may be worth stating that the value is only observable before the file is written. It survives copy() and Java serialization but is dropped by any v1-v3 manifest round trip, since it has no position in BaseFile.internalGet/internalSet. That is intentional and covered in the PR description, but a caller reading only the interface has no way to know.

szehon-ho · 2026-08-22T02:52:23Z

        nanValueCounts,
        lowerBounds,
        upperBounds,
+        avgValueSizes,


Suggested change

avgValueSizes,

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.

szehon-ho · 2026-08-22T02:52:23Z

+   * Returns if collected, map from column ID to its average non-null value size in bytes, null
+   * otherwise.


Suggested change

* Returns if collected, map from column ID to its average non-null value size in bytes, null

* otherwise.

* 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.

szehon-ho · 2026-08-22T02:52:23Z

  }

+  /**
+   * Get the average non-null value size in bytes for all fields where it was collected.


Suggested change

* Get the average non-null value size in bytes for all fields where it was collected.

* 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.

szehon-ho · 2026-08-22T02:52:23Z

        copyWithoutKeys(metrics.nanValueCounts(), excludedFieldIds),
        metrics.lowerBounds(),
        metrics.upperBounds(),
+        copyWithoutKeys(metrics.avgValueSizes(), excludedFieldIds),


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.

- ParquetMetrics: return null instead of an empty avgValueSizes map when no column tracks the stat, so a writer-produced file matches the null a ContentStatsBackedMap returns from a manifest. - ContentFile/Metrics javadoc: describe the value as the in-memory (uncompressed) size over non-null values, and note it is observable only before the file is written -- it survives copy() and Java serialization but a v1-v3 manifest round trip drops it. - Add TestMetricsUtil covering that copyWithoutFieldCounts and copyWithoutFieldCountsAndBounds drop avg value sizes for excluded fields, and return null once every tracked field is excluded.

Co-authored-by: Szehon Ho <szehon.apache@gmail.com>

Guarding the stub broke TestTrackedFileAdapters. Unlike the counts, avgValueSizeInBytes has no has*() gate, so ContentStatsBackedMap uses avgValueSizeInBytes() != null as the presence check. A Mockito mock defaults this method to 0, not null, so an absent stat left unstubbed reads back as present with 0. Stub it unconditionally so a null value models an untracked column, and note why with a comment.

szehon-ho

LGTM. All prior feedback is addressed. One non-blocking javadoc nit inline.

Co-authored-by: Szehon Ho <szehon.apache@gmail.com>

szehon-ho · 2026-08-27T18:29:05Z

+   * Returns if collected, map from column ID to its average value size in memory (uncompressed) in
+   * bytes over non-null values, null otherwise.
+   *
+   * <p>This statistic is not persisted in manifests prior to v4, so it is generally only present


Please apply the Spotless wrapping here. The latest build-checks job fails :iceberg-api:spotlessJavaCheck on this Javadoc, so required CI remains red.

Applied — wrapped the Javadoc so :iceberg-api:spotlessJavaCheck is clean.

szehon-ho · 2026-08-27T18:29:05Z

      this.nanValueCounts = copyMap(toCopy.nanValueCounts, requestedColumnIds);
      this.lowerBounds = copyByteBufferMap(toCopy.lowerBounds, requestedColumnIds);
      this.upperBounds = copyByteBufferMap(toCopy.upperBounds, requestedColumnIds);
+      this.avgValueSizes = copyMap(toCopy.avgValueSizes, requestedColumnIds);


Please normalize this to null when filtering removes every average-size entry. copyMap returns a non-null empty SerializableMap, so copyWithStats with a nonmatching field ID differs from the accessor contract and from the tracked-file adapter. Could we also add a nonmatching-column test?

Done. copyWithStats now normalizes a fully-filtered avgValueSizes map to null, matching the accessor contract and the tracked-file adapter. Added a nonmatching-column unit test plus the same assertion on the Parquet geo round-trip.

szehon-ho · 2026-08-27T18:29:57Z

        lowerBounds == null ? null : Collections.unmodifiableMap(lowerBounds),
-        upperBounds == null ? null : Collections.unmodifiableMap(upperBounds));
+        upperBounds == null ? null : Collections.unmodifiableMap(upperBounds),
+        file.avgValueSizes(),


Please remove PATH_ID from avgValueSizes in both path-rewrite helpers and normalize an empty result to null. The action rewrites the position-delete file_path values, so preserving the old average produces stale statistics whenever the source and target prefixes have different byte lengths. Removing only PATH_ID preserves any unrelated statistics.

let's fix the size in a follow up pr for rewrite_table_paths

Sounds good — I'll leave PATH_ID / rewrite-table-paths avg sizes for a follow-up.

copyMap leaves an empty SerializableMap when no requested column matches, which disagrees with the null-otherwise contract. Also wrap the ContentFile javadoc for Spotless.

github-actions Bot added API spark parquet core Specification Issues that may introduce spec changes. labels Jul 31, 2026

huan233usc marked this pull request as draft July 31, 2026 14:54

huan233usc mentioned this pull request Jul 31, 2026

Parquet: Track avg_value_size_in_bytes for Geo types #17333

Merged

huan233usc force-pushed the geo-avg-value-size-manifest branch from 018c802 to 4bdbfaf Compare July 31, 2026 17:45

huan233usc changed the title ~~Core, Parquet: Carry avg value sizes for v4 content stats~~ [WIP]Core, Parquet: Carry avg value sizes for v4 content stats Jul 31, 2026

uros-b mentioned this pull request Aug 11, 2026

API: Add Javadoc for the Metrics constructor parameters #17608

Merged

huan233usc force-pushed the geo-avg-value-size-manifest branch from 4bdbfaf to 0b172a2 Compare August 19, 2026 18:32

huan233usc changed the title ~~[WIP]Core, Parquet: Carry avg value sizes for v4 content stats~~ Core, Parquet: Carry avg value sizes for v4 content stats Aug 19, 2026

huan233usc force-pushed the geo-avg-value-size-manifest branch 2 times, most recently from adf31b5 to 8e3e2a1 Compare August 20, 2026 02:09

huan233usc force-pushed the geo-avg-value-size-manifest branch from 8e3e2a1 to f139886 Compare August 20, 2026 04:10

huan233usc marked this pull request as ready for review August 20, 2026 04:12

szehon-ho reviewed Aug 22, 2026

View reviewed changes

Xin Huang and others added 3 commits August 26, 2026 10:31

Update core/src/test/java/org/apache/iceberg/StatsTestUtil.java

74a31af

Co-authored-by: Szehon Ho <szehon.apache@gmail.com>

huan233usc requested a review from szehon-ho August 26, 2026 18:10

szehon-ho approved these changes Aug 27, 2026

View reviewed changes

Comment thread api/src/main/java/org/apache/iceberg/ContentFile.java Outdated

Update api/src/main/java/org/apache/iceberg/ContentFile.java

da11c41

Co-authored-by: Szehon Ho <szehon.apache@gmail.com>

szehon-ho reviewed Aug 27, 2026

View reviewed changes

Core: Normalize empty avgValueSizes after copyWithStats

8d88e43

copyMap leaves an empty SerializableMap when no requested column matches, which disagrees with the null-otherwise contract. Also wrap the ContentFile javadoc for Spotless.

	avgValueSizes,
	avgValueSizes.isEmpty() ? null : avgValueSizes,

		* Returns if collected, map from column ID to its average non-null value size in bytes, null
		* otherwise.

	* Get the average non-null value size in bytes for all fields where it was collected.
	* Get the average value size in memory (uncompressed) in bytes over non-null values, for all
	* fields where it was collected.

Uh oh!

Conversation

huan233usc commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szehon-ho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szehon-ho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

huan233usc commented Jul 31, 2026 •

edited

Loading