Core: present v4 ContentStats as legacy stat maps via ContentStatsBackedMap by stevenzwu · Pull Request #17322 · apache/iceberg

stevenzwu · 2026-07-21T18:33:16Z

What

Add ContentStatsBackedMap, a lazy read-only Map<Integer, V> view over a ContentStats that projects a single metric (value/null/NaN counts, or lower/upper bounds) from a column's FieldStats on demand. TrackedFileAdapters now presents the five legacy ContentFile stat maps through ContentStatsBackedMap.forKind(...), replacing the eager MetricsUtil converters that materialized a fresh map per metric for every manifest entry.

forKind restores the eager converters' null-when-empty contract — it returns null when no column tracks the metric — via an allocation-free, short-circuiting scan. Point reads (get/containsKey) allocate no map, boxed value, or bound buffer; the entry set is materialized (and cached) only when the whole map is iterated.
FieldStats gains hasNullCount() / hasNaNCount() so the presence of a primitive-long count is explicit. FieldStatsStruct takes the two counts as boxed Long, so null uniformly signals an untracked count across every construction path, and the count getters checkState rather than unbox a null.
Removes the now-unused MetricsUtil.valueCounts/nullValueCounts/nanValueCounts/lowerBounds/upperBounds(ContentStats) converters.

Why

The eager converters allocated a LinkedHashMap (plus boxed counts and decoded bound buffers) per metric for every manifest entry read. The lazy view avoids that on the hot point-read path and preserves the legacy null-when-empty semantics that scan planning relies on.

Benchmarks

The design and the null-when-empty tradeoff are backed by JMH microbenchmarks (lazy wrap vs eager convert, and the cost of restoring null-when-empty): V4 Read-Direction Bridge Benchmark.

🤖 Generated with Claude Code

stevenzwu · 2026-07-21T18:48:57Z

          26L,
          0L,
-          0L,
+          null,


Intentional: data is optional (tracks a null count, kept 0L above) but not floating-point, so it has no NaN count. With hasNullCount()/hasNaNCount() now derived from a non-null boxed Long, an untracked count must be null — matching what StatsUtil.fieldStatsStruct declares. ID_STATS (a required long) is null for both.

-1 for changing the tests in this file.

These fields are set correctly for the types so these are all non-functional changes that are not needed. If the type is a string, then a 0 NaN count is valid.

stevenzwu · 2026-07-21T18:48:57Z


    FieldStats<Integer> fieldStats =
-        new FieldStatsStruct<>(UNKNOWN_FIELD_STATS_STRUCT, 0, 10, false, 8, 3, 0, null);
+        new FieldStatsStruct<>(UNKNOWN_FIELD_STATS_STRUCT, 0, 10, false, 8, null, null, null);


Intentional: UNKNOWN_FIELD_STATS_STRUCT is a required Integer, so it declares neither count; both args are null. Previously 3, 0 — harmless under the old type-based tracksStat, but with the value-based check a non-null value would make hasNullCount()/hasNaNCount() report a metric the type doesn't track. This test never reads the counts, so null just keeps the fixture consistent.

stevenzwu · 2026-07-21T19:05:27Z

      long valueCount,
-      long nullValueCount,
-      long nanValueCount,
+      Long nullValueCount,


Boxed Long (not primitive long) for the two counts so a caller can pass null for a metric the field's type does not track; hasNullCount()/hasNaNCount() then report presence as != null. valueCount stays primitive since every column has one.

I disagree with this change. The cases where null count can be null do not go through this path. This constructor should require non-null counts for both nan value count and null value count (though not avgValueSize) because both should be known when generating stats. I don't know of a case where we write a file and have stats, but don't know the NaN count.

There are cases where the null and NaN counts can be null.

nan_value_count is only applicable for float and double fields. other types won't have Nan count.

null_value_count is omitted in the type for required fields.

But maybe you are suggesting we should construct the FieldStatsStruct this way?

FieldStatsStruct<String> stats = new FieldStatsStruct<>(STRING_STATS); stats.fromFieldMetrics(new FieldMetrics<>(100, 28, 2, "a", "z"));

The existing code already has trouble construct the FieldStatsStruct properly. Because the constructor can't pass in null, 0L is passed in for the nanValueCount for an integer field. E.g., this code from TestTrackedFileAdapters.

private static final FieldStats<Integer> ID_STATS = new FieldStatsStruct<>( CONTENT_STATS_TYPE.fieldType("id").asStructType(), 1, 1000, true, 100L, 5L, 0L, null); ^^^

huaxingao · 2026-07-21T21:25:43Z

+  }
+
+  /** Returns a lazy view of the metric across columns, or {@code null} if no column tracks it. */
+  static <V> Map<Integer, V> forKind(Kind kind, ContentStats stats) {


Nit: the argument order here is (kind, stats), but the constructor above is (stats, kind) and the static isEmpty below is also (stats, kind). Could we make forKind match so the order is consistent across all three?

We could also remove the forKind method to keep Kind private and instead use factory methods: lowerBounds, upperBounds, valueCounts, etc.

huaxingao · 2026-07-21T21:32:52Z

+  @Override
+  public V get(Object key) {
+    if (!(key instanceof Integer)) {
+      throw new ClassCastException("Key must be an Integer field id: " + key);


This seems to be a behavior change from the HashMaps this replaces, which returned null from get and false from containsKey for a missing or wrong-typed key. Is this change intentional? If so, a short comment would help.

Seems easier to return null and be consistent with more widely used Map implementations.

huaxingao · 2026-07-21T21:54:32Z


-  /** The total null value count */
+  /** Whether a null value count is tracked for this field. */
+  boolean hasNullCount();


Small naming nit: could we align these with the existing getters? They are nullValueCount() and nanValueCount(), so hasNullValueCount() and hasNanValueCount() would match,

I agree with this.

stevenzwu · 2026-07-22T21:56:40Z

+
+  /** Returns a lazy view of the metric across columns, or {@code null} if no column tracks it. */
+  static <V> Map<Integer, V> forKind(Kind kind, ContentStats stats) {
+    return isEmpty(stats, kind) ? null : new ContentStatsBackedMap<>(stats, kind);


isEmpty check adds some overhead as it needs to iterates through the FieldStats collection in the ContentStats. But I think it is worth to have this for two reason.

The ContentFile interface states null map if not collected.

/** Returns if collected, map from column ID to its null value count, null otherwise. */ Map<Integer, Long> nullValueCounts();

isEmpty short-circuit and return early if it detects a FieldStats has stats for the kind. In most production scenarios, I imagine short-circuit should kick in.

I think this is a good idea.

rdblue · 2026-07-22T22:58:03Z

+  private final Kind kind;
+  private Map<Integer, V> materialized;
+
+  ContentStatsBackedMap(ContentStats stats, Kind kind) {


Can this be private?

rdblue · 2026-07-22T23:01:54Z

+  }
+
+  @SuppressWarnings("unchecked")
+  private static <V> V getStatValue(FieldStats<?> fieldStats, Kind kind) {


Style: method names (even private ones) should generally avoid "get".

rdblue · 2026-07-22T23:07:47Z

+  private static ByteBuffer bound(FieldStats<?> fieldStats, Object bound, int boundOffset) {
+    Types.NestedField boundField =
+        fieldStats.type().field(StatsUtil.toBaseId(fieldStats.fieldId()) + boundOffset);
+    if (bound == null || boundField == null) {


bound == null is checked in toByteBuffer so there's no need for it here. Also, it is easier to look up the bound field by name rather than ID:

// you can introduce a constant for "lower_bound" if you want Type boundType = fieldStats.type().fieldType("lower_bound");

I think it is correct to check for the field since it may be missing for MetricsMode.Counts.

Also, it is easier to look up the bound field by name rather than ID:

I also thought about it. it is also probably slightly better to query by name lower_bound, as the FieldStatsStruct constructor internally already triggered lazyFieldsByName() mapping.

this.boundType = struct.fieldType("lower_bound");

Earlier, I used id because it is the canonical identifier defined in StatsUtil, while field names are not exposed constants in StatsUtil. I don't want to use hardcoded literals here. But since field name is also part of the spec, we can add the stat field name constants to StatsUtil just like the stat field offset.

Either way, we should be consistent in the wrapper and the FieldStatsStruct constructor.

Yeah, we can introduce a constant for it if you like. I just wanted to reduce the number of constants if they weren't needed.

rdblue · 2026-07-22T23:09:25Z

+  @Override
+  public Set<Entry<Integer, V>> entrySet() {
+    if (materialized == null) {
+      Map<Integer, V> entries = Maps.newLinkedHashMap();


Why use a map rather than a set of entries?

Oh, I get it. This caches the result for faster lookups.

actually, it is not used for lookups get call. it is only cached for repeated entrySet() call. your question on why not set is valid.

rdblue · 2026-07-22T23:15:32Z

+
+  // Whether the field's stats struct declares the metric at the given offset.
+  private static boolean tracksStat(FieldStats<?> fieldStats, int statOffset) {
+    return fieldStats.type().field(StatsUtil.toBaseId(fieldStats.fieldId()) + statOffset) != null;


I don't think that this class should be performing ID calculations. Instead, this should check based on field name in the struct. This is safe because the field name is generated by this library. We generate the read schema with predictable field names.

rdblue · 2026-07-22T23:24:40Z

+
+  // Whether getStatValue would return a non-null value, without allocating a boxed count or
+  // decoding a bound. Must mirror getStatValue's null-ness.
+  private static boolean contributes(FieldStats<?> fieldStats, Kind kind) {


The results are inconsistent between map Kinds. This handles bounds by checking whether the field is present in the schema, but it handles counts by assuming the field is present and just checking for null. The same check would be valid for bounds: if the schema doesn't have a bound then it will always be null because there is no way to set it during a read. And it would be more accurate to test isEmpty using the simpler null check because lower_bound could be present but null.

I would rename this to be a bit more clear, like isKnown.

rdblue · 2026-07-22T23:32:55Z

  /** The total value count, including null and NaN */
  long valueCount();

-  /** The total null value count */


I think we also need a few more methods:

hasValueCount: the value count may not be projected or may not have been written

missingNullValueCount, missingNaNValueCount: I think it is good to have negations to make code easier to read, like Map.nonEmpty

hasAvgValueSizeInBytes: this field may be null for the same reason as the others, so we should treat it the same way (this was an oversight on my part)

I will add hasValueCount. But let's add others when they are actually used/needed. Introducing them now is a bit weird since there are no callers.

missingNullValueCount

missingNaNValueCount

hasAvgValueSizeInBytes

…kedMap Add ContentStatsBackedMap, a lazy read-only Map<Integer, V> view over a ContentStats that projects one metric per column on demand, replacing the eager MetricsUtil converters that materialized a map per metric. TrackedFileAdapters exposes the five legacy ContentFile stat maps through ContentStatsBackedMap.forKind, which restores the converters' null-when-empty contract with an allocation-free short-circuit scan. Add FieldStats.hasNullCount()/hasNaNCount() so the presence of a primitive-long count is explicit, and take the two counts as boxed Long in FieldStatsStruct's constructor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

- expose per-column factory methods (valueCounts/nullValueCounts/ nanValueCounts/lowerBounds/upperBounds) and make Kind and the constructor private; drop forKind - get() returns null for a non-Integer key instead of throwing, matching common Map implementations - rename contributes -> isKnown and test bounds with a simple null check (a bound absent from the schema is always null); drop the field-id math - look up the bound field type by name via StatsUtil.LOWER_BOUND_NAME/ UPPER_BOUND_NAME - FieldStats: rename hasNullCount/hasNaNCount -> hasNullValueCount/ hasNanValueCount; add hasValueCount and guard valueCount() - cache entrySet() as a Set; rename getStatValue -> statValue Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Revert the null and NaN value counts on the FieldStatsStruct constructor from boxed Long back to primitive long, matching the existing signature. The boxed parameters existed only so tests could construct columns with absent counts, which does not warrant widening production code. Build those absent-count states in tests instead: ContentStatsBackedMap and TrackedFileAdapters read stats only through the FieldStats interface, so they mock FieldStats; the FieldStatsStruct and ContentStatsStruct tests exercise the real struct and serialize it, so they populate a FieldStatsStruct through the setter path, leaving untracked counts unset. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

rdblue · 2026-07-23T21:40:16Z

+
  @Override
  public long valueCount() {
+    Preconditions.checkState(hasValueCount(), "Field %s does not track a value count", fieldId);


I think this and similar checks should be removed. We don't need extra checks in tight loops that are only going to slow down scan planning. If this is called when the value count is missing then it is a bug in the caller.

rdblue · 2026-07-23T21:43:17Z

+
+    // Presents the wrapped content stats as a legacy per-column stats map without eagerly
+    // materializing it.
+    private <V> Map<Integer, V> statsMap(Function<ContentStats, Map<Integer, V>> statsView) {


I don't understand the value of this function. All it does is check that contentStats is non-null. Rather than a complicated way to save an inline null check, the stats functions should just return null for null content stats. You can do that in a single place by updating isEmpty to check for a null content stats.

rdblue · 2026-07-23T21:48:24Z

+  }
+
+  @Test
+  public void testGetDoesNotUnboxUntrackedNullCount() {


This duplicates the null value count test above, just without the third entry for dbl. I think you can remove it.

rdblue · 2026-07-23T21:49:16Z

+  }
+
+  @Test
+  public void testLowerAndUpperBounds() {


I think this should be a separate test like there are for the other map types. I'd also like to see each value in the map tested.

rdblue · 2026-07-23T21:51:00Z

+  }
+
+  @Test
+  public void testFactoryReturnsPopulatedViewWhenColumnsTrackMetric() {


These maps are already tested. We know that field 1 is not in either map and that the counts are correct, so I think this test can be removed as a duplicate.

rdblue · 2026-07-23T21:52:34Z

+  }
+
+  @Test
+  public void testFactoryReturnsNullWhenNoColumnTracksMetric() {


I would also like to see a similar test to this one that returns a non-null map for counts. The case is for null stats structs. The schema may include them but they are missing.

rdblue · 2026-07-23T21:54:46Z

+  @Test
+  public void testPopulatedViewIsNotEmpty() {
+    // isEmpty() answers from a scan without materializing entrySet
+    assertThat(ContentStatsBackedMap.valueCounts(ONLY_REQUIRED_STATS)).isNotEmpty();


I think this is okay, but this relies on the same isEmpty implementation as the cases that the map is null and all cases where the map is non-null. As a result, the only thing actually being tested here is that isEmpty() is backed by isEmpty(stats, kind). I'm not sure I'd include this but it's fine if you think it's important.

Move the FieldStatsStruct setter-path factory into a shared StatsTestUtil so the FieldStats, ContentStats, ContentStatsBackedMap, and TrackedFileAdapters tests build stats through one helper instead of per-file copies and inline FieldStats mocks. Drop two comments that only restated the code: the StatsUtil bound-name constants and the ContentStatsBackedMap isKnown helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…Struct Remove the value/null/NaN count getter guards in FieldStatsStruct: reading a missing count is a caller bug and the checkState calls only slow scan-planning loops. Callers that must tolerate absence check has*ValueCount() first. Handle null content stats in one place: ContentStatsBackedMap.isEmpty (and therefore the stat-map factories) returns null for null stats, so TrackedFileAdapters calls the factories directly and its statsMap null-check wrapper is removed. Restructure TestContentStatsBackedMap: remove three tests that duplicated existing coverage, split the bounds test into separate lower/upper tests that assert every entry, and add a test that a count map stays non-null when some columns' stats structs are missing. Drop the obsolete "reading absent count throws" assertions in TestFieldStatsStruct. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

gaborkaszab

Went through the PR for my own education. Left some comments in the meantime

gaborkaszab · 2026-07-24T05:54:08Z

+    UPPER_BOUND
+  }
+
+  /** Per-column value counts, or {@code null} if no column tracks the value count. */


nit: null instead of {@code null}

gaborkaszab · 2026-07-24T06:00:27Z

+  }
+
+  @Override
+  public boolean isEmpty() {


If I'm not mistaken, in the constructing methods we return null if the map were empty. Following this, in case we have constructed a map object it's not empty. Can we simply return false here?

gaborkaszab · 2026-07-24T06:03:47Z

+    return viewOrNull(stats, Kind.UPPER_BOUND);
+  }
+
+  private static <V> Map<Integer, V> viewOrNull(ContentStats stats, Kind kind) {


nit: this is private, shouldn't this be located next to the other private static methods?

gaborkaszab · 2026-07-24T06:15:42Z

+
+  @SuppressWarnings("unchecked")
+  private static <V> V statValue(FieldStats<?> fieldStats, Kind kind) {
+    switch (kind) {


nit: new switch style here and above?

gaborkaszab · 2026-07-24T07:22:45Z

+      return true;
+    }
+
+    for (FieldStats<?> fieldStats : stats.fieldStats()) {


nit: maybe with stream?

return stats.fieldStats().stream() .filter(Objects::nonNull) .noneMatch(fieldStats -> isKnown(fieldStats, kind));

isEmpty() is on the scan-planning's hot code path. A stream adds a Stream + lambda allocation per call, and ContentStats.fieldStats() returns an Iterable, so it would also need a Streams.stream(...) wrapper. The imperative loop short-circuits on the first contributing field and allocates nothing, so I'd prefer to keep it here.

gaborkaszab · 2026-07-24T07:50:02Z

+  }
+
+  @Override
+  public Set<Entry<Integer, V>> entrySet() {


Just for my understanding: I'm trying to understand the reason for introducing the cached materialized result. Do we expect entrySet to be called multiple times on the same ContentStatsBackedMap object? Apart from that e.g. ContentStatsBackedMap.valueCounts(stats) seems identical to MetricsUtil.valueCounts(stats) to me, unless I miss something.
Asking the question from a different angle, can this entrySet function be "pass-through" without caching similarly to get?

entrySet() is the only method that has to materialize (it's a full projection), so its result is cached: AbstractMap routes size(), forEach(), toString(), and equals() through entrySet(), and without the cache any caller that does more than a single pass would rebuild the LinkedHashSet every time. get()/containsKey() stay pass-through and allocate nothing.

It isn't equivalent to the old MetricsUtil.valueCounts(stats), which eagerly built and returned a full map on every call. Here the map is a lazy view: a caller that only does get() or a null check never materializes a set, and an empty metric returns null from an allocation-free scan rather than an empty map. That laziness, plus returning null instead of an empty map, is the reason for the class.

gaborkaszab · 2026-07-24T07:54:37Z

+    return materialized;
+  }
+
+  /** Returns whether no column contributes an entry for the metric, including for null stats. */


nit: Returns whether no column contributes an entry for the metric
This sentence seems weird. Maybe this:
Returns whether no fields have the given metric.

, including for null stats: I think this part of the comment is not needed.

- isEmpty() returns false directly, since a factory returns null for an empty view so a constructed instance always has an entry; add a test that isEmpty() agrees with entrySet() - convert isKnown and statValue to expression switches - group viewOrNull with the other private static helpers - javadoc: use plain null over {@code null} and reword the isEmpty helper doc Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

rdblue · 2026-07-24T19:07:21Z

+   * {@code hasNullValueCount()}/{@code hasNanValueCount()} report false. This keeps "absent count"
+   * states out of the production constructor.
+   */
+  static <T> FieldStatsStruct<T> fieldStats(


I don't think that we should be abusing FieldStatsStruct in this way to test a class that is based on the FieldStats interface and not on the object. This should just return a mock or a test implementation, not a FieldStatsStruct.

Per review feedback, apache#17322 should not modify TestFieldStatsStruct or TestContentStatsStruct -- fixing structs in those tests is unrelated to the content-stats maps and belongs in a separate change. Revert both to their apache/main form. The ContentStatsBackedMap and TrackedFileAdapters tests now mock FieldStats to construct the columns they need, instead of a set-by-position helper; a single shared StatsTestUtil.mockFieldStats(...) backs both. The FieldStats and FieldStatsStruct presence methods that ContentStatsBackedMap relies on stay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

rdblue · 2026-07-24T20:17:54Z

-          3L,
-          null);
+  private static final FieldStats<?> ID_STATS =
+      StatsTestUtil.mockFieldStats(


Is this necessary? Isn't this equivalent to the existing code?

Not equivalent

old: 100L, 5L, 0L

new: 100L, 5L, null

it's needed. id is an integer, and the previous fixture set nan=0 on it. MetricsUtil.nanValueCounts hid that by filtering on column type (FLOAT/DOUBLE only); ContentStatsBackedMap decides tracking by presence (hasNanValueCount()), so a forced 0 makes id show up in nanValueCounts() -- which a real integer column never does, since its stats struct has no nan_value_count field. The value constructor can't express "no nan count" (the nan arg is a primitive long), so the fixture mocks FieldStats instead.

Sounds reasonable. Thanks for the context!

rdblue · 2026-07-24T20:19:29Z

  }

+  @Test
+  void testMetricTrackedByNoColumnReturnsNull() {


I don't think additional tests are needed here. Behavior of the new maps is thoroughly checked by TestContentStatsBackedMap, which is a better place for the tests. The existing tests demonstrate that content stats are adapted to the maps, but the behavior does not need to be thoroughly validated.

TestTrackedFileAdapters should only check that the stat maps are produced; whether a map is null when no column tracks a metric is the map's contract, covered by TestContentStatsBackedMap. Remove testMetricTrackedByNoColumnReturnsNull and the dataFileWithStats helper it used. The ID_STATS/SCORE_STATS fixtures stay mocked so the integer id column carries no nan_value_count, which the FieldStatsStruct value constructor cannot express. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…eldStats apache/main (via apache#17322) added presence methods to FieldStats. Implement hasValueCount / hasNullValueCount / hasNanValueCount in MapBackedFieldStats, mirroring FieldStatsStruct. Also drop the redundant final class modifiers and make MapBackedFieldStats a non-static inner class, so it reads the enclosing stats view's maps directly instead of carrying an explicit parent reference. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Production: - Implement the new FieldStats presence methods (hasValueCount / hasNullValueCount / hasNanValueCount) in MapBackedFieldStats. - Drop the -1 sentinel from the count getters; valueCount / nullValueCount / nanValueCount unbox directly and callers must check has*Count() first (matches FieldStatsStruct after apache#17322). - Drop redundant final modifiers and make MapBackedFieldStats a non-static inner class, reading the enclosing view's maps directly. Tests: - Assert upperBound type in testBoundDecodingPerType. - Add testSetNotSupported for the outer set(); split testFieldStatsCopyAndSetNotSupported into two focused tests. - Sharpen testContentStructLikeGetReturnsChildrenOrNull to verify the null slot maps to source field 5, not just that some position is null. - Split the StructLike surface out of testCountsOnlyColumnOmitsBounds into two symmetric tests (testDefaultStructLike / testCountsOnlyStructLike) using Comparators.forType with TestHelpers.Row expectations. - Extend testCountsAndDefaults to also cover the absent-count throw path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

dramaticlly · 2026-07-27T21:32:23Z

    Type boundType = isGeoType(type) ? geoLowerBound(baseId) : type;
-    return optional(baseId + LOWER_BOUND_OFFSET, "lower_bound", boundType);


Not direct relate to your change, but I think geoLowerBound(baseId) actually returns StructType, but the Conversions.toByteBuffer called within ContentStatsBackedMap.bound() today might not handle the struct type for geometry properly.

iceberg/api/src/main/java/org/apache/iceberg/types/Conversions.java

Lines 95 to 145 in c8a4b98

switch (typeId) {

case BOOLEAN:

return ByteBuffer.allocate(1).put(0, (Boolean) value ? (byte) 0x01 : (byte) 0x00);

case INTEGER:

case DATE:

return ByteBuffer.allocate(4).order(ByteOrder.LITTLE_ENDIAN).putInt(0, (int) value);

case LONG:

case TIME:

case TIMESTAMP:

case TIMESTAMP_NANO:

return ByteBuffer.allocate(8).order(ByteOrder.LITTLE_ENDIAN).putLong(0, (long) value);

case FLOAT:

return ByteBuffer.allocate(4).order(ByteOrder.LITTLE_ENDIAN).putFloat(0, (float) value);

case DOUBLE:

return ByteBuffer.allocate(8).order(ByteOrder.LITTLE_ENDIAN).putDouble(0, (double) value);

case STRING:

CharBuffer buffer = CharBuffer.wrap((CharSequence) value);

try {

return ENCODER.get().encode(buffer);

} catch (CharacterCodingException e) {

throw new RuntimeIOException(e, "Failed to encode value as UTF-8: %s", value);

}

case UUID:

return UUIDUtil.convertToByteBuffer((UUID) value);

case FIXED:

case BINARY:

return (ByteBuffer) value;

case DECIMAL:

return ByteBuffer.wrap(((BigDecimal) value).unscaledValue().toByteArray());

case VARIANT:

// Produce a concatenated buffer of metadata and value

Variant variant = (Variant) value;

VariantMetadata variantMetadata = variant.metadata();

VariantValue variantValue = variant.value();

ByteBuffer variantBuffer =

ByteBuffer.allocate(variantMetadata.sizeInBytes() + variantValue.sizeInBytes())

.order(ByteOrder.LITTLE_ENDIAN);

variantMetadata.writeTo(variantBuffer, 0);

variantValue.writeTo(variantBuffer, variantMetadata.sizeInBytes());

return variantBuffer;

case GEOMETRY:

case GEOGRAPHY:

// Geometry and geography lower/upper bounds are single points encoded as an

// x:y:z:m concatenation of 8-byte little-endian IEEE 754 doubles. See the

// Bound Serialization section of the Iceberg spec.

return ((GeospatialBound) value).toByteBuffer();

case UNKNOWN:

// underlying type not known

return null;

default:

throw new UnsupportedOperationException("Cannot serialize type: " + typeId);

.

I am not sure the status of upper/lower bound of geometry/geography type in main, but it seems we can run into UnsupportedOperationException.

Confirmed ,this does throw UnsupportedOperationException for geometry and geography bounds.
Repro and proposed fix in #17493.

…eldStats apache/main (via apache#17322) added presence methods to FieldStats. Implement hasValueCount / hasNullValueCount / hasNanValueCount in MapBackedFieldStats, mirroring FieldStatsStruct. Also drop the redundant final class modifiers and make MapBackedFieldStats a non-static inner class, so it reads the enclosing stats view's maps directly instead of carrying an explicit parent reference. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Production: - Implement the new FieldStats presence methods (hasValueCount / hasNullValueCount / hasNanValueCount) in MapBackedFieldStats. - Drop the -1 sentinel from the count getters; valueCount / nullValueCount / nanValueCount unbox directly and callers must check has*Count() first (matches FieldStatsStruct after apache#17322). - Drop redundant final modifiers and make MapBackedFieldStats a non-static inner class, reading the enclosing view's maps directly. Tests: - Assert upperBound type in testBoundDecodingPerType. - Add testSetNotSupported for the outer set(); split testFieldStatsCopyAndSetNotSupported into two focused tests. - Sharpen testContentStructLikeGetReturnsChildrenOrNull to verify the null slot maps to source field 5, not just that some position is null. - Split the StructLike surface out of testCountsOnlyColumnOmitsBounds into two symmetric tests (testDefaultStructLike / testCountsOnlyStructLike) using Comparators.forType with TestHelpers.Row expectations. - Extend testCountsAndDefaults to also cover the absent-count throw path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

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

stevenzwu added this to V4: metadata tree Jul 21, 2026

stevenzwu force-pushed the content-stats-backed-map branch 2 times, most recently from 00fa85c to 02c40c9 Compare July 21, 2026 18:59

stevenzwu commented Jul 21, 2026

View reviewed changes

huaxingao reviewed Jul 21, 2026

View reviewed changes

stevenzwu commented Jul 22, 2026

View reviewed changes

rdblue reviewed Jul 22, 2026

View reviewed changes

stevenzwu and others added 2 commits July 23, 2026 11:24

stevenzwu force-pushed the content-stats-backed-map branch from 3fec893 to 132377a Compare July 23, 2026 18:32

rdblue reviewed Jul 23, 2026

View reviewed changes

stevenzwu and others added 2 commits July 23, 2026 15:28

gaborkaszab reviewed Jul 24, 2026

View reviewed changes

stevenzwu mentioned this pull request Jul 24, 2026

Docs: Recommend switch expressions in AGENTS.md code style #17357

Merged

rdblue reviewed Jul 24, 2026

View reviewed changes

rdblue approved these changes Jul 24, 2026

View reviewed changes

stevenzwu merged commit ea5c1cd into apache:main Jul 24, 2026
36 checks passed

github-project-automation Bot moved this to Done in V4: metadata tree Jul 24, 2026

stevenzwu deleted the content-stats-backed-map branch July 24, 2026 21:42

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

dramaticlly reviewed Jul 27, 2026

View reviewed changes

vishnuprakaz mentioned this pull request Aug 3, 2026

Core: Fix geo bounds in v4 legacy stat maps #17493

Open

 L,
 L,
-L,
+                        null,

		Type boundType = isGeoType(type) ? geoLowerBound(baseId) : type;
		return optional(baseId + LOWER_BOUND_OFFSET, "lower_bound", boundType);

	switch (typeId) {
	case BOOLEAN:
	return ByteBuffer.allocate(1).put(0, (Boolean) value ? (byte) 0x01 : (byte) 0x00);
	case INTEGER:
	case DATE:
	return ByteBuffer.allocate(4).order(ByteOrder.LITTLE_ENDIAN).putInt(0, (int) value);
	case LONG:
	case TIME:
	case TIMESTAMP:
	case TIMESTAMP_NANO:
	return ByteBuffer.allocate(8).order(ByteOrder.LITTLE_ENDIAN).putLong(0, (long) value);
	case FLOAT:
	return ByteBuffer.allocate(4).order(ByteOrder.LITTLE_ENDIAN).putFloat(0, (float) value);
	case DOUBLE:
	return ByteBuffer.allocate(8).order(ByteOrder.LITTLE_ENDIAN).putDouble(0, (double) value);
	case STRING:
	CharBuffer buffer = CharBuffer.wrap((CharSequence) value);
	try {
	return ENCODER.get().encode(buffer);
	} catch (CharacterCodingException e) {
	throw new RuntimeIOException(e, "Failed to encode value as UTF-8: %s", value);
	}
	case UUID:
	return UUIDUtil.convertToByteBuffer((UUID) value);
	case FIXED:
	case BINARY:
	return (ByteBuffer) value;
	case DECIMAL:
	return ByteBuffer.wrap(((BigDecimal) value).unscaledValue().toByteArray());
	case VARIANT:
	// Produce a concatenated buffer of metadata and value
	Variant variant = (Variant) value;
	VariantMetadata variantMetadata = variant.metadata();
	VariantValue variantValue = variant.value();
	ByteBuffer variantBuffer =
	ByteBuffer.allocate(variantMetadata.sizeInBytes() + variantValue.sizeInBytes())
	.order(ByteOrder.LITTLE_ENDIAN);
	variantMetadata.writeTo(variantBuffer, 0);
	variantValue.writeTo(variantBuffer, variantMetadata.sizeInBytes());
	return variantBuffer;
	case GEOMETRY:
	case GEOGRAPHY:
	// Geometry and geography lower/upper bounds are single points encoded as an
	// x:y:z:m concatenation of 8-byte little-endian IEEE 754 doubles. See the
	// Bound Serialization section of the Iceberg spec.
	return ((GeospatialBound) value).toByteBuffer();
	case UNKNOWN:
	// underlying type not known
	return null;
	default:
	throw new UnsupportedOperationException("Cannot serialize type: " + typeId);

Uh oh!

Conversation

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

Uh oh!

What

Why

Benchmarks

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 Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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 commented Jul 21, 2026 •

edited

Loading

stevenzwu Jul 23, 2026 •

edited

Loading

rdblue Jul 22, 2026 •

edited

Loading

stevenzwu Jul 23, 2026 •

edited

Loading