Core: Refactor ContentStats and FieldStats by rdblue · Pull Request #17159 · apache/iceberg

rdblue · 2026-07-10T21:46:24Z

This updates the content stats API and implementations used for columnar stats in v4.

API changes:

Removes StructLike used for serialization from the interfaces
Adds copy methods to support copying TrackedFile with stats
Aligns interface conventions (use Iterable, expose full struct type as type, etc.)

StatsUtil changes:

Reduce complexity of ID methods, reduce number of constants
Remove FieldStatistic enum; not all offsets were represented as FieldStatistic and it also embedded util methods. All utils are now in StatsUtil.
Avoid double switch statement to resolve position to enum and enum to field in FieldStatsStruct get and set
Simplified field stats struct generation
Replaced content stats schema visitor with a loop
Added stats schema generation for read path based on field IDs
Added stats schema generation for write path based on MetricsConfig

ContentStats implementation changes:

Rename to ContentStatsStruct to align with other v4 types
Implement StructLike methods without copying data to allow object reuse, do not rebuild field stats
Use setCustomType instead of copying via builder in set
Keep field stats in a map rather than syncing between list and map
Remove unnecessary builder
Fix raw types suppressions

FieldStats implementation changes:

Rename to FieldStatsStruct to align with other v4 types
Remove SupportsIndexProjection that is bypassed
Implement get and set projection directly based on offset, do not rely on mapping passed to constructor
Use internal get/set to avoid duplicated javaClass.cast
Remove equals and hashCode
Remove unnecessary builder

Test changes:

Separated StatsUtil test values from production constants
Aligned implementation tests with other v4 object tests (test accessors, get, set, projection, serialization, and copy)
Updated tests that depend on stats classes

rdblue · 2026-07-10T21:47:41Z

+  Types.StructType type();
+
+  /** Returns a copy of these {@link ContentStats}, deep-copying the contained field stats. */
+  ContentStats copy();


Needed to implement copy in TrackedFile correctly. We also need to follow up with methods to project by field ID.

rdblue · 2026-07-10T21:49:32Z

+   * <p>Note: This type may be a projection of the stats stored in manifest files.
   */
-  Type type();
+  Types.StructType type();


This changed to the actual schema. The lower and upper bounds types do not always match (for geo types) so this is the best way to expose types. It is also more consistent with existing conventions.

rdblue · 2026-07-10T21:49:58Z


  /** The total NaN value count */
-  Long nanValueCount();
+  long nanValueCount();


These changes are conservative. We may want to widen later, but I want to make a conscious choice.

Curious what's driving this, from a spec perspective is there an open question about if these stats should be required? My understanding of the parquet spec is that value counts are required but null/NaN counts are optional

I think these accessors should reflect the API that we want for building on top of stats. So the decision should be made when we introduce classes that consume these.

I think that primitives are more locked down. They will throw a NullPointerException if called when there is no value, which is good for catching calls that should not be made. For example, IS NaN and IS NOT NaN predicates cannot be bound to fields that are not floating points (throws ValidationException) so accessing the NaN count should not happen if the field is missing from the stats struct.

On the other hand, these values may just be missing for a specific data file because they weren't collected. In this case, we may want to signal that the value is missing by returning the underlying null. This is why I left the avg_value_size_in_bytes column as an Integer.

If there is a case where we know we always have a value, then it makes sense to use the primitive. But it may also be better to have a method to check whether the value is known and still use a primitive when we may not have a value. I'm undecided at the moment and I'm waiting for actual uses. I'm also currently writing the evaluator and, while it isn't done, I think we may want to add more methods and keep the types more restrictive.

consolidate the discussion on how to handle no stats scenario. there are two options

nullable boxed type of Long

primitive long + ValidationException + hasXxx

The existing APIs consistently use option 1, where null means "unknown/not collected," and callers null-check directly. If we decided to go with option 2, we should include the guardrails of ValidationException and hasXxx.

rdblue · 2026-07-10T21:50:30Z

+
+  private Object getOffset(int offset) {
+    return switch (offset) {
+      case StatsUtil.LOWER_BOUND_OFFSET -> lowerBound();


Uses offsets to avoid a switch by position immediately followed by a switch by enum symbol.

rdblue · 2026-07-10T21:52:12Z


  private static final Types.StructType CONTENT_STATS_TYPE =
-      StatsUtil.contentStatsFor(TABLE_SCHEMA).type().asStructType();
+      StatsUtil.statsReadSchema(TABLE_SCHEMA, ImmutableList.of(1, 2));


The read schema method is easiest to call for testing since it creates full stats structs for each field ID. The write schema method will omit lower and upper bounds for MetricsMode.Counts.

rdblue · 2026-07-10T21:53:16Z

+          10L,
+          3L,
+          null);
+  private static final ContentStatsStruct CONTENT_STATS =


I left this in to continue testing the MetricsUtil conversion, but I suspect that those conversions will be done lazily.

Those are the only place where fieldId is needed so I think we can avoid needing to recover field ID from base ID if we rewrite them.

rdblue · 2026-07-10T21:53:39Z

    assertThat(dataFile.equalityFieldIds()).isNull();
    assertThat(dataFile.columnSizes()).isNull();
-    assertThat(dataFile.valueCounts()).containsOnly(Map.entry(1, 100L), Map.entry(2, 200L));
+    assertThat(dataFile.valueCounts()).containsOnly(Map.entry(1, 100L), Map.entry(2, 100L));


Updated to align with what stats would actually look like.

amogh-jahagirdar · 2026-07-14T15:09:24Z


  /** The total NaN value count */
-  Long nanValueCount();
+  long nanValueCount();


Curious what's driving this, from a spec perspective is there an open question about if these stats should be required? My understanding of the parquet spec is that value counts are required but null/NaN counts are optional

amogh-jahagirdar · 2026-07-14T15:12:46Z

+    return tightBounds;
+  }
+
+  @Override
+  public long valueCount() {
+    return valueCount;


Based on the comment below the change to make this API a long was a conscious one but given the additional change to MetricsUtil that's made wouldn't this NPE in cases where value / null / nan counts are null?

I added a comment about MetricsUtil conversion, but it was in tests so it wasn't obvious. I don't think that we actually want to convert between the two representations and that it was premature to add that logic.

Conversion is expensive. It allocates lots of buffers and copies data into them, but we don't really know what is going to be accessed. I think we should have a simple Map implementation that translates when values are accessed, or just return null for the old APIs.

I think we're seeing another place where you get in trouble when production code is introduced just for testing.

amogh-jahagirdar · 2026-07-14T15:21:17Z

+  /** Container struct type containing tracked field-level stats structs. */
+  Types.StructType type();
+
+  /** Returns a copy, deep-copying all field stats. */


nit: Returns a deep copy of all field stats?

amogh-jahagirdar · 2026-07-14T15:30:32Z

+
+  @Override
+  public <T> T get(int pos, Class<T> javaClass) {
+    return javaClass.cast(idToFieldStats.get(posToId[pos]));


BaseContentStats get used to check pos against the struct size, and it looks like the old code returned null in case the pos was "out of bounds" of the struct. It looks like that was done for Avro because on the write path Avro calls get() for every field in the writer schema. I don't think we're fully plumbed through which is why we don't see any issues but are we handling that case differently in this PR, or we're just deferring that? If I recall correctly we wanted to use this in memory representation regardless of format version (because we theoretically could adapt the existing stats structure to this)

Yes, that behavior deviated from what StructLike instances are intended to do. If the indexes don't match then the reader is using a different schema than the class and we want that to fail. Part of the problem with the old implementation was not implementing the StructLike interface or using the SupportsIndexProjection base class correctly.

This implementation should bring the struct classes in line with what SupportsIndexProjection does.

stevenzwu

some initial comments

stevenzwu · 2026-07-15T22:14:46Z

-        result.put(fs.fieldId(), fs.nanValueCount());
+      if (fs != null) {
+        Type boundType = fs.type().fieldType("lower_bound");
+        if (boundType.typeId() == Type.TypeID.FLOAT || boundType.typeId() == Type.TypeID.DOUBLE) {


Problem:

boundType.typeId() NPEs when the stats struct has no lower_bound field. This happens for float/double columns in Counts mode — StatsUtil.fieldStatsStruct only adds lower_bound when mode.hasBounds() is true (L262-273), but nan_value_count is added for any floating-point field regardless of mode.

because bound fields are conditionally omitted from the stats struct, the field's type isn't available to consumers when bounds aren't collected. MetricsUtil has no way to know whether a column is float/double if lower_bound is missing.

Options:

Tactical fix — use a different presence signal. fs.type().field("nan_value_count") != null maps directly to "this field can have NaN counts" and doesn't depend on lower_bound being present. Local, targeted.

Keep bound fields in the schema even when values aren't collected. Change fieldStatsStruct so Counts mode still emits lower_bound / upper_bound NestedFields (with null values). Preserves the type/values distinction, keeps schema shape stable across modes, and consumers can always derive the bound type. Null column probably has negligible cost for Parquet manifest.

Option 2 is the architecturally cleanest. Option 1 is the smallest diff.

Yes, this is throw-away code that will likely not be kept.

stevenzwu · 2026-07-15T22:14:47Z

+      if (fs != null) {
+        Type boundType = fs.type().fieldType("lower_bound");
+        if (fs.lowerBound() != null && boundType != null) {
+          result.put(fs.fieldId(), Conversions.toByteBuffer(boundType, fs.lowerBound()));


Summary: FieldStats.type() changed from "field's data type" to "stats struct type," which affects only geo — for a geo column, lower_bound's schema type is a struct (bounding-box) rather than a scalar matching the field type, and Conversions.toByteBuffer has no case STRUCT, so it throws instead of dispatching to case GEOMETRY.

Layout. For a geo column, lower_bound / upper_bound are bounding-box structs, not scalar Geometry values (geoLowerBound / geoUpperBound at StatsUtil.java:207-237):

FieldStatsStruct for a GEOMETRY column: ├── lower_bound: STRUCT ← name is "lower_bound", type is a struct │ ├── x: double (required) │ ├── y: double (required) │ ├── z: double (optional) │ └── m: double (optional) ├── upper_bound: STRUCT ← same shape ├── tight_bounds: boolean ├── value_count: long └── null_value_count: long

FieldStats.type() before/after. Before this PR: returned the field's data type (GeometryType for geo). After (FieldStats.java:32): returns the stats struct type; the bound's type is now retrieved via fs.type().fieldType("lower_bound").

Runtime failure. For geo, fs.type().fieldType("lower_bound") returns the geo_lower StructType. Conversions.toByteBuffer (Conversions.java:95-146) switches on typeId() — STRUCT has no matching case, so default throws UnsupportedOperationException: Cannot serialize type: STRUCT. The case GEOMETRY / case GEOGRAPHY branch handling bounds via ((GeospatialBound) value).toByteBuffer() is unreachable.

Same at line 558 for upperBounds.

Coverage gap. TestFieldStatsStruct.testGeoSerialization exercises Java / Kryo / InternalData round-trip but not this MetricsUtil path. TestTrackedFileAdapters calls MetricsUtil.lowerBounds via TrackedFileAdapters.asDataFile, but its schema is (id: int, score: float) — no geo. Geo × MetricsUtil isn't tested anywhere.

Should we add TestMetricsUtil? Every other v4 stats class in this PR has its own test file (TestFieldStatsStruct, TestContentStatsStruct, TestStatsUtil) — MetricsUtil is the exception and currently has no direct test. A new core/src/test/java/org/apache/iceberg/TestMetricsUtil.java would be the natural home for:

testLowerBoundsGeo — geometry column, assert ByteBuffer output matches GeospatialBound.toByteBuffer()

testUpperBoundsGeography — same for geography

testNanValueCountsWithCountsMode — float column in Counts mode, assert no NPE (see the sibling thread on L521)

testValueCountsProjection — FieldStatsStruct constructed via the projection constructor, verify unset counts are handled without NPE (see the sibling thread on L490)

testLowerBoundsScalarPrimitives — smoke test across int / long / double / string / binary / decimal / uuid

MetricsUtil conversions are placeholders. This was already a problem. It was just hidden before.

anoopj · 2026-07-16T01:06:09Z

+    Map<Integer, String> idToStatsName = TypeUtil.indexStatsNames(tableSchema.asStruct());
+    List<Types.NestedField> fieldStructs = Lists.newArrayList();
+
+    for (int id : metricsConfig.metricsFieldIds()) {


The field order is non-determinisitc because we are iterating over a hash map. Should we sort by IDs? There is no correctness issue here.

I thought about this and wanted to preserve the schema order. At the time, it seemed like a good idea to use the order from the input (in this case metricsFieldIds) rather than sorting by field ID because sorting would erase the schema order (this is why the read path accepts Iterable). However, you're right that metricsFieldIds is unordered.

I think what we want is to preserve the schema order, but I'd like to do it in a follow up since it would probably make more changes to MetricsConfig and I tried to separate those changes out (into #17022).

stevenzwu

I am approving this.

but I wanted to point out that this discussion seems still open: #17159 (comment)

nastra · 2026-07-17T13:45:00Z

+      long nanValueCount,
+      Integer avgValueSize) {
+    this(struct);
+    setLowerBound(lowerBound);


we used to check that upper/lower bound are of the right type and would throw Invalid lower bound type, expected a subtype of xx. Are we not doing that anymore?

That's correct. We don't want additional type checks. The readers and writers are responsible for ensuring types match. Those are generated from the schema and won't operate on invalid values.

nastra · 2026-07-17T13:47:37Z

+    return javaClass.cast(getOffset(posToOffset[pos]));
+  }
+
+  private void setOffset(int offset, Object value) {


should this be called setFromOffset because setOffset is misleading and indicates that we're setting the offset while we actually set the value

nastra · 2026-07-17T13:55:27Z

+    switch (offset) {
+      case StatsUtil.LOWER_BOUND_OFFSET -> setLowerBound(value);
+      case StatsUtil.UPPER_BOUND_OFFSET -> setUpperBound(value);
+      case StatsUtil.TIGHT_BOUNDS_OFFSET -> this.tightBounds = (Boolean) value;


wouldn't this fail with a NPE when a null is passed?

I guess the same applies for the other fields

I think the only time that tight bounds will be null is when the column is not present. Otherwise, we should always have a value for tight bounds because we can always assume that bounds are not tight if it is unknown.

That said, the column is currently optional. I think we should decide whether to make that required or to accept a null here and translate it to false since the bounds are assumed to not be tight when this is unknown.

nastra · 2026-07-17T14:34:44Z

+            Types.NestedField.optional(30_002, "upper_bound", upperBound),
+            Types.NestedField.optional(30_004, "value_count", Types.LongType.get()),
+            Types.NestedField.optional(30_005, "null_value_count", Types.LongType.get()),
+            Types.NestedField.optional(30_007, "avg_value_size_in_bytes", Types.IntegerType.get()));


for Geo the spec says that avg_value_size_in_bytes should be omitted

Geo types are variable width so I think we should update the spec.

nastra · 2026-07-17T14:49:44Z

+      String fieldName = idToStatsName.get(id);
+      Types.NestedField field = tableSchema.findField(id);
+      Preconditions.checkArgument(
+          field != null, "Cannot build content stats schema: missing field ID %s", id);


maybe mention that the field ID is missing from the table schema

I think that's what this says.

nastra · 2026-07-17T15:35:04Z

+    return (FieldStats<T>) idToFieldStats.get(id);
+  }
+
+  public <T> void setStats(int id, FieldStats<T> fieldStats) {


Suggested change

public <T> void setStats(int id, FieldStats<T> fieldStats) {

public <T> void setStats(int fieldId, FieldStats<T> fieldStats) {

nastra · 2026-07-17T15:36:01Z

+
+  @Override
+  @SuppressWarnings("unchecked")
+  public <T> FieldStats<T> statsFor(int id) {


Suggested change

public <T> FieldStats<T> statsFor(int id) {

public <T> FieldStats<T> statsFor(int fieldId) {

rdblue · 2026-07-17T19:31:29Z

Thanks for the reviews! I'm going to merge this to unblock other changes and integration with the readers. We can still follow up on a few things, like whether tightBounds is required or optional.

…e_count MetricsUtil.nullValueCounts(ContentStats) was added in apache#16100, when the FieldStats count accessors returned boxed Long, so a column without a null_value_count simply produced a null map entry. apache#17159 changed those accessors to primitive long. In v4 content stats, null_value_count is only stored for optional (nullable) columns: StatsUtil.fieldStatsStruct omits the field for required columns, so a deserialized FieldStatsStruct for a required column leaves it null. nullValueCounts now throws a NullPointerException unboxing that null. Add StatsUtil.tracksStat(fieldStatsType, fieldId, statOffset) and guard nullValueCounts with it, skipping columns whose stats struct does not track null_value_count (reported as unknown) instead of throwing, as nanValueCounts already filters by whether the metric applies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

github-actions Bot added the core label Jul 10, 2026

rdblue commented Jul 10, 2026

View reviewed changes

rdblue force-pushed the fix-content-stats branch from b35b148 to f996392 Compare July 10, 2026 21:56

rdblue commented Jul 10, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TrackedFileStruct.java Outdated

rdblue added this to V4: metadata tree Jul 13, 2026

rdblue mentioned this pull request Jul 13, 2026

Core: Use mocks in TrackedFile tests #17133

Merged

rdblue force-pushed the fix-content-stats branch from 297cd9b to 33128e1 Compare July 13, 2026 22:22

amogh-jahagirdar self-requested a review July 14, 2026 15:04

amogh-jahagirdar reviewed Jul 14, 2026

View reviewed changes

rdblue mentioned this pull request Jul 14, 2026

API: Extract superclass from InclusiveMetricsEvaluator #17201

Merged

stevenzwu reviewed Jul 15, 2026

View reviewed changes

stevenzwu reviewed Jul 16, 2026

View reviewed changes

anoopj reviewed Jul 16, 2026

View reviewed changes

RussellSpitzer added the Iceberg V4 Iceberg Table Format Version 4 label Jul 16, 2026

rdblue force-pushed the fix-content-stats branch 2 times, most recently from 4da0316 to d16aa52 Compare July 16, 2026 22:11

Core: Refactor ContentStats and FieldStats.

0fa25ad

rdblue force-pushed the fix-content-stats branch from d16aa52 to 0fa25ad Compare July 16, 2026 22:59

Apply spotless.

6bcc3c6

rdblue mentioned this pull request Jul 16, 2026

API, Core: Make variant classes serializable #17260

Merged

stevenzwu moved this to In review in V4: metadata tree Jul 17, 2026

stevenzwu approved these changes Jul 17, 2026

View reviewed changes

nastra reviewed Jul 17, 2026

View reviewed changes

amogh-jahagirdar approved these changes Jul 17, 2026

View reviewed changes

rdblue merged commit 100d062 into apache:main Jul 17, 2026
36 of 38 checks passed

github-project-automation Bot moved this from In review to Done in V4: metadata tree Jul 17, 2026

nssalian added this to the Iceberg 1.12.0 milestone Jul 20, 2026

stevenzwu mentioned this pull request Jul 20, 2026

Core: guard MetricsUtil.nullValueCounts for columns without null_value_count #17306

Closed

nastra mentioned this pull request Jul 21, 2026

Spec: Track avg_value_size_in_bytes for Geo types #17313

Merged

	public <T> void setStats(int id, FieldStats<T> fieldStats) {
	public <T> void setStats(int fieldId, FieldStats<T> fieldStats) {

	public <T> FieldStats<T> statsFor(int id) {
	public <T> FieldStats<T> statsFor(int fieldId) {

Uh oh!

Conversation

rdblue commented Jul 10, 2026

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!

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!

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!

stevenzwu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenzwu 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!