stevenzwu commented on code in PR #17159:
URL: https://github.com/apache/iceberg/pull/17159#discussion_r3591103535
##########
core/src/main/java/org/apache/iceberg/MetricsUtil.java:
##########
@@ -516,8 +516,11 @@ static Map<Integer, Long> nanValueCounts(ContentStats
stats) {
Map<Integer, Long> result = Maps.newHashMap();
for (FieldStats<?> fs : stats.fieldStats()) {
- if (fs != null && fs.nanValueCount() != null) {
- 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) {
Review Comment:
**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:**
1. **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.
2. **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.
##########
core/src/main/java/org/apache/iceberg/MetricsUtil.java:
##########
@@ -531,8 +534,11 @@ static Map<Integer, ByteBuffer> lowerBounds(ContentStats
stats) {
Map<Integer, ByteBuffer> result = Maps.newHashMap();
for (FieldStats<?> fs : stats.fieldStats()) {
- if (fs != null && fs.lowerBound() != null && fs.type() != null) {
- result.put(fs.fieldId(), Conversions.toByteBuffer(fs.type(),
fs.lowerBound()));
+ 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()));
Review Comment:
**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`.
##########
core/src/test/java/org/apache/iceberg/TestTrackedFileStruct.java:
##########
@@ -211,6 +217,103 @@ void copy() {
assertThat(copy.recordCount()).isEqualTo(50L);
assertThat(copy.fileSizeInBytes()).isEqualTo(512L);
assertThat(copy.specId()).isEqualTo(1);
+ assertThat(copy.contentStats()).isSameAs(CONTENT_STATS_COPY);
+ assertThat(copy.sortOrderId()).isEqualTo(5);
+ assertThat(copy.deletionVector()).isSameAs(DELETION_VECTOR_COPY);
+ assertThat(copy.manifestInfo()).isSameAs(MANIFEST_INFO_COPY);
+ assertThat(copy.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2,
3}));
+ assertThat(copy.splitOffsets()).containsExactly(100L, 200L);
+ assertThat(copy.equalityIds()).containsExactly(1, 2, 3);
+ assertThat(copy.partition()).isSameAs(PARTITION_COPY);
+
+ // mutable fields are deep-copied, not shared with the original
+ assertThat(copy.keyMetadata()).isNotSameAs(file.keyMetadata());
+ }
+
+ @Test
+ void copyWithStats() {
+ ContentStats stats = Mockito.mock(ContentStats.class);
+ ContentStats statsCopy = Mockito.mock(ContentStats.class);
+ Mockito.when(stats.copy(ImmutableSet.of(1))).thenReturn(statsCopy);
+
+ TrackedFileStruct file =
+ new TrackedFileStruct(
+ TRACKING,
+ FileContent.DATA,
+ FORMAT_VERSION_V4,
+ "s3://bucket/data/00000-0-file.parquet",
+ FileFormat.PARQUET,
+ PARTITION,
+ 50L,
+ 512L,
+ 1,
+ stats,
+ 5,
+ DELETION_VECTOR,
+ MANIFEST_INFO,
+ ByteBuffer.wrap(new byte[] {1, 2, 3}),
+ ImmutableList.of(100L, 200L),
+ ImmutableList.of(1, 2, 3));
+
+ TrackedFile copy = file.copyWithStats(ImmutableSet.of(1));
+
+ assertThat(copy).isInstanceOf(TrackedFileStruct.class);
+ assertThat(copy.tracking()).isSameAs(TRACKING_COPY);
+ assertThat(copy.contentType()).isEqualTo(FileContent.DATA);
+ assertThat(copy.formatVersion()).isEqualTo(FORMAT_VERSION_V4);
+
assertThat(copy.location()).isEqualTo("s3://bucket/data/00000-0-file.parquet");
+ assertThat(copy.fileFormat()).isEqualTo(FileFormat.PARQUET);
+ assertThat(copy.recordCount()).isEqualTo(50L);
+ assertThat(copy.fileSizeInBytes()).isEqualTo(512L);
+ assertThat(copy.specId()).isEqualTo(1);
+ assertThat(copy.contentStats()).isSameAs(statsCopy);
+ assertThat(copy.sortOrderId()).isEqualTo(5);
+ assertThat(copy.deletionVector()).isSameAs(DELETION_VECTOR_COPY);
+ assertThat(copy.manifestInfo()).isSameAs(MANIFEST_INFO_COPY);
+ assertThat(copy.keyMetadata()).isEqualTo(ByteBuffer.wrap(new byte[] {1, 2,
3}));
+ assertThat(copy.splitOffsets()).containsExactly(100L, 200L);
+ assertThat(copy.equalityIds()).containsExactly(1, 2, 3);
+ assertThat(copy.partition()).isSameAs(PARTITION_COPY);
+
+ // mutable fields are deep-copied, not shared with the original
+ assertThat(copy.keyMetadata()).isNotSameAs(file.keyMetadata());
+ }
+
+ @Test
+ void copyWithoutStats() {
Review Comment:
Two problems:
1. **The test at line 283 doesn't call `copyWithoutStats()`.** Line 305
calls `copyWithStats(ImmutableSet.of(1))`. The `copy.contentStats() == null`
assertion on 316 only passes because `stats` is a Mockito mock and `.copy(Set)`
was never stubbed — not a real check of `copyWithoutStats` semantics.
2. **`TrackedFile.copyWithoutStats()` throws at runtime.** The default at
TrackedFile.java:180 calls `copyWithStats(Collections.emptySet())` →
`ContentStatsStruct.copy(emptySet)` → trips the `!fieldIds.isEmpty()`
precondition at ContentStatsStruct.java:105 → `IllegalArgumentException`. Any
file with non-null `contentStats` throws.
Verified with [this
gist](https://gist.github.com/stevenzwu/97a9de101e4500b7b7398702d10eb923).
Two fix options: loosen the `ContentStatsStruct.copy` precondition to accept
empty sets, or short-circuit `copyWithoutStats` in `TrackedFileStruct` to null
out `contentStats` directly. Later seems more reasonable.
##########
core/src/main/java/org/apache/iceberg/MetricsUtil.java:
##########
@@ -531,8 +534,11 @@ static Map<Integer, ByteBuffer> lowerBounds(ContentStats
stats) {
Map<Integer, ByteBuffer> result = Maps.newHashMap();
for (FieldStats<?> fs : stats.fieldStats()) {
- if (fs != null && fs.lowerBound() != null && fs.type() != null) {
- result.put(fs.fieldId(), Conversions.toByteBuffer(fs.type(),
fs.lowerBound()));
+ 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()));
Review Comment:
**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
##########
core/src/test/java/org/apache/iceberg/TestContentStatsStruct.java:
##########
@@ -0,0 +1,261 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.IOException;
+import java.util.Comparator;
+import java.util.List;
+import org.apache.iceberg.TestHelpers.RoundTripSerializer;
+import org.apache.iceberg.inmemory.InMemoryOutputFile;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.types.Comparators;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Named;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.FieldSource;
+import org.mockito.Mockito;
+
+public class TestContentStatsStruct {
Review Comment:
I asked AI to compare the coverage btw the deleted `TestContentStats` and
this new class `TestContentStatsStruct`. This flagged one is probably worth a
callout.
**Coverage regression: bound-type validation at set time is gone.**
Deleted `TestContentStats.setByPositionWithInvalidLowerAndUpperBound`
asserted that setting a wrong-typed bound threw `IllegalArgumentException` with
`"Invalid lower bound type, expected a subtype of ..."`. Neither
`FieldStatsStruct.setLowerBound` (FieldStatsStruct.java:108-110) nor
`ContentStatsStruct.setStats` (ContentStatsStruct.java:64-76) performs this
check now. Compile-time generics catch typed callers; raw-type /
`StructLike.set` / reflection paths bypass.
Test that would lock in the invariant if validation were restored (verified
locally: fails on the current PR):
```java
@Test
void testSetLowerBoundRejectsWrongType() {
Types.StructType idStatsSchema =
StatsUtil.statsReadSchema(
new Schema(required(1, "id", Types.IntegerType.get())),
ImmutableList.of(1))
.field("id").type().asStructType();
FieldStatsStruct<Object> stats = new FieldStatsStruct<>(idStatsSchema);
assertThatThrownBy(() -> stats.set(0, 5.0)) // lower_bound offset, Double
vs Integer
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("expected a subtype of java.lang.Integer:
java.lang.Double");
}
```
Without the check, wrong-typed bounds flow through silently and fail later
at `Conversions.toByteBuffer` with a less-actionable `ClassCastException`.
Intentional (readers guarantee types upstream), or a gap to restore at
`setLowerBound` / `setUpperBound`?
##########
core/src/test/java/org/apache/iceberg/TestStatsUtil.java:
##########
@@ -18,511 +18,449 @@
*/
package org.apache.iceberg;
-import static org.apache.iceberg.FieldStatistic.AVG_VALUE_SIZE_IN_BYTES;
-import static org.apache.iceberg.FieldStatistic.LOWER_BOUND;
-import static org.apache.iceberg.FieldStatistic.NAN_VALUE_COUNT;
-import static org.apache.iceberg.FieldStatistic.NULL_VALUE_COUNT;
-import static org.apache.iceberg.FieldStatistic.TIGHT_BOUNDS;
-import static org.apache.iceberg.FieldStatistic.UPPER_BOUND;
-import static org.apache.iceberg.FieldStatistic.VALUE_COUNT;
-import static org.apache.iceberg.types.Types.NestedField.optional;
-import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
import java.util.List;
-import java.util.concurrent.ThreadLocalRandom;
-import java.util.stream.IntStream;
+import org.apache.iceberg.types.Type;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.FieldSource;
public class TestStatsUtil {
- // the reserved field IDs from the reserved field ID space as defined in
- // https://iceberg.apache.org/spec/#reserved-field-ids
- private static final int RESERVED_FIELD_IDS_START = Integer.MAX_VALUE - 200;
+ @Test
+ public void testToBaseIdWithData() {
+ assertThat(StatsUtil.toBaseId(0)).isEqualTo(10_000);
+ assertThat(StatsUtil.toBaseId(1)).isEqualTo(10_200);
+ assertThat(StatsUtil.toBaseId(100)).isEqualTo(30_000);
+ assertThat(StatsUtil.toBaseId(999_949)).isEqualTo(199_999_800);
+ }
@Test
- public void statsIdsForTableColumns() {
- int offset = 0;
- for (int id = 0; id < StatsUtil.MAX_DATA_FIELD_ID; id++) {
- int statsFieldId = StatsUtil.statsFieldIdForField(id);
- int expected = StatsUtil.STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS +
offset;
- assertThat(statsFieldId).as("at pos %s", id).isEqualTo(expected);
- offset += StatsUtil.NUM_SUPPORTED_STATS_PER_COLUMN;
- assertThat(StatsUtil.fieldIdForStatsField(statsFieldId)).as("at pos %s",
id).isEqualTo(id);
- }
+ public void testToBaseIdWithMetadata() {
+
assertThat(StatsUtil.toBaseId(MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId()))
+ .isEqualTo(9_000);
+
assertThat(StatsUtil.toBaseId(MetadataColumns.ROW_ID.fieldId())).isEqualTo(9_200);
+ }
- // also verify hardcoded field IDs from docs
- int fieldId = 0;
- int statsFieldId = 10_000;
-
assertThat(StatsUtil.statsFieldIdForField(fieldId)).isEqualTo(statsFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(statsFieldId)).isEqualTo(fieldId);
-
- fieldId = 1;
- statsFieldId = 10_200;
-
assertThat(StatsUtil.statsFieldIdForField(fieldId)).isEqualTo(statsFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(statsFieldId)).isEqualTo(fieldId);
-
- fieldId = 2;
- statsFieldId = 10_400;
-
assertThat(StatsUtil.statsFieldIdForField(fieldId)).isEqualTo(statsFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(statsFieldId)).isEqualTo(fieldId);
-
- fieldId = 5;
- statsFieldId = 11_000;
-
assertThat(StatsUtil.statsFieldIdForField(fieldId)).isEqualTo(statsFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(statsFieldId)).isEqualTo(fieldId);
-
- fieldId = 100;
- statsFieldId = 30_000;
-
assertThat(StatsUtil.statsFieldIdForField(fieldId)).isEqualTo(statsFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(statsFieldId)).isEqualTo(fieldId);
-
- fieldId = StatsUtil.MAX_DATA_FIELD_ID;
- statsFieldId = StatsUtil.MAX_DATA_STATS_FIELD_ID;
-
assertThat(StatsUtil.statsFieldIdForField(fieldId)).isEqualTo(statsFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(statsFieldId)).isEqualTo(fieldId);
-
- fieldId = -1;
- statsFieldId = -1;
-
assertThat(StatsUtil.statsFieldIdForField(fieldId)).isEqualTo(statsFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(statsFieldId)).isEqualTo(fieldId);
+ @Test
+ public void testBaseIdUnassigned() {
+ assertThat(StatsUtil.toBaseId(999_950)).isEqualTo(-1);
+ assertThat(StatsUtil.toBaseId(-1)).isEqualTo(-1);
+
assertThat(StatsUtil.toBaseId(MetadataColumns.FILE_PATH.fieldId())).isEqualTo(-1);
}
@Test
- public void statsIdsOverflowForTableColumns() {
- // pick 100 random IDs that are > MAX_FIELD_ID and <
RESERVED_FIELD_IDS_START as going over
- // the entire ID range takes too long
- int invalidFieldId = -1;
- for (int i = 0; i < 100; i++) {
- int id =
- ThreadLocalRandom.current()
- .nextInt(StatsUtil.MAX_DATA_FIELD_ID + 1,
RESERVED_FIELD_IDS_START);
- assertThat(StatsUtil.statsFieldIdForField(id)).as("at pos %s",
id).isEqualTo(invalidFieldId);
- }
+ public void testToFieldIdWithData() {
+ assertThat(StatsUtil.toFieldId(10_000)).isEqualTo(0);
+ assertThat(StatsUtil.toFieldId(10_200)).isEqualTo(1);
+ assertThat(StatsUtil.toFieldId(30_000)).isEqualTo(100);
+ assertThat(StatsUtil.toFieldId(199_999_800)).isEqualTo(999_949);
+ }
- assertThat(StatsUtil.fieldIdForStatsField(-1)).isEqualTo(invalidFieldId);
- assertThat(StatsUtil.fieldIdForStatsField(0)).isEqualTo(invalidFieldId);
- assertThat(StatsUtil.fieldIdForStatsField(200)).isEqualTo(invalidFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(5_000)).isEqualTo(invalidFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(8_600)).isEqualTo(invalidFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(10_001)).isEqualTo(invalidFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(10_201)).isEqualTo(invalidFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(10_500)).isEqualTo(invalidFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(10_900)).isEqualTo(invalidFieldId);
-
- // stats field IDs at or above the exclusive upper bound are invalid
-
assertThat(StatsUtil.fieldIdForStatsField(StatsUtil.STATS_SPACE_FIELD_ID_END))
- .isEqualTo(invalidFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(StatsUtil.STATS_SPACE_FIELD_ID_END +
200))
- .isEqualTo(invalidFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(Integer.MAX_VALUE)).isEqualTo(invalidFieldId);
-
- // field ID just past MAX_DATA_FIELD_ID is invalid
- assertThat(StatsUtil.statsFieldIdForField(StatsUtil.MAX_DATA_FIELD_ID + 1))
- .isEqualTo(invalidFieldId);
+ @Test
+ public void testToFieldIdWithMetadata() {
+ assertThat(StatsUtil.toFieldId(9_000))
+ .isEqualTo(MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId());
+
assertThat(StatsUtil.toFieldId(9_200)).isEqualTo(MetadataColumns.ROW_ID.fieldId());
}
@Test
- public void statsIdsForMetadataColumns() {
- int fieldId = MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId();
- int statsFieldId = 9_000;
-
assertThat(StatsUtil.statsFieldIdForField(fieldId)).isEqualTo(statsFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(statsFieldId)).isEqualTo(fieldId);
-
- fieldId = MetadataColumns.ROW_ID.fieldId();
- statsFieldId = 9_200;
-
assertThat(StatsUtil.statsFieldIdForField(fieldId)).isEqualTo(statsFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(statsFieldId)).isEqualTo(fieldId);
-
- // reserved metadata fields with IDs below/above the reserved stats range
have no stats
- int invalidFieldId = -1;
-
assertThat(StatsUtil.fieldIdForStatsField(8_800)).isEqualTo(invalidFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(9_400)).isEqualTo(invalidFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(9_600)).isEqualTo(invalidFieldId);
-
assertThat(StatsUtil.fieldIdForStatsField(9_800)).isEqualTo(invalidFieldId);
- assertThat(IntStream.range(RESERVED_FIELD_IDS_START, Integer.MAX_VALUE))
- .filteredOn(id -> !StatsUtil.SUPPORTED_METADATA_FIELD_IDS.contains(id))
- .allSatisfy(
- id ->
- assertThat(StatsUtil.statsFieldIdForField(id))
- .as("at pos %s", id)
- .isEqualTo(invalidFieldId));
+ public void testToFieldIdOutsideRange() {
+ assertThatThrownBy(() -> StatsUtil.toFieldId(200_000_000))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid stats field ID: 200000000");
+ assertThatThrownBy(() -> StatsUtil.toFieldId(200_000_001))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid stats field ID: 200000001");
+ assertThatThrownBy(() -> StatsUtil.toFieldId(8_800))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid stats field ID: 8800");
+ assertThatThrownBy(() -> StatsUtil.toFieldId(8_801))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid stats field ID: 8801");
}
@Test
- public void contentStatsForSimpleSchema() {
- Types.NestedField intField = required(0, "i", Types.IntegerType.get());
- Types.NestedField floatField = required(2, "f", Types.FloatType.get());
- Types.NestedField stringField = required(4, "s", Types.StringType.get());
- Types.NestedField booleanField = required(6, "b", Types.BooleanType.get());
- Types.NestedField uuidField = required(StatsUtil.MAX_DATA_FIELD_ID, "u",
Types.UUIDType.get());
- Schema schema = new Schema(intField, floatField, stringField,
booleanField, uuidField);
- Schema expectedStatsSchema =
- new Schema(
- optional(
- 146,
- "content_stats",
- Types.StructType.of(
- optional(10000, "i",
FieldStatistic.fieldStatsFor(intField, 10000)),
- optional(10400, "f",
FieldStatistic.fieldStatsFor(floatField, 10400)),
- optional(10800, "s",
FieldStatistic.fieldStatsFor(stringField, 10800)),
- optional(11200, "b",
FieldStatistic.fieldStatsFor(booleanField, 11200)),
- optional(
- StatsUtil.MAX_DATA_STATS_FIELD_ID,
- "u",
- FieldStatistic.fieldStatsFor(
- uuidField, StatsUtil.MAX_DATA_STATS_FIELD_ID)))));
- Schema statsSchema = new Schema(StatsUtil.contentStatsFor(schema));
-
assertThat(statsSchema.asStruct()).isEqualTo(expectedStatsSchema.asStruct());
+ public void testToFieldIdReservedMetadataRange() {
+ // 9,000 to 10,000 (exclusive) is reserved for metadata column stats
+ assertThatThrownBy(() -> StatsUtil.toFieldId(9_400))
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage("Unsupported metadata stats field ID: 9400");
}
@Test
- public void contentStatsForComplexSchema() {
- Types.NestedField listElement = optional(3, "element",
Types.IntegerType.get());
- Types.NestedField structInt = optional(7, "int", Types.IntegerType.get());
- Types.NestedField structString = optional(8, "string",
Types.StringType.get());
- Types.NestedField mapKey = required(22, "key", Types.IntegerType.get());
- Types.NestedField mapValue = optional(24, "value", Types.StringType.get());
- Types.NestedField variantField = required(30, "variant",
Types.VariantType.get());
- Types.NestedField uuidField = required(StatsUtil.MAX_DATA_FIELD_ID, "u",
Types.UUIDType.get());
- Schema schema =
- new Schema(
- required(0, "i", Types.IntegerType.get()),
- required(2, "list", Types.ListType.ofOptional(3,
Types.IntegerType.get())),
- required(
- 6,
- "simple_struct",
- Types.StructType.of(
- optional(7, "int", Types.IntegerType.get()),
- optional(8, "string", Types.StringType.get()))),
- required(
- 20,
- "b",
- Types.MapType.ofOptional(22, 24, Types.IntegerType.get(),
Types.StringType.get())),
- variantField,
- uuidField);
- Schema expectedStatsSchema =
- new Schema(
- optional(
- 146,
- "content_stats",
- Types.StructType.of(
- optional(
- 10000,
- "i",
- FieldStatistic.fieldStatsFor(
- required(0, "i", Types.IntegerType.get()), 10000)),
- optional(
- 10600, "list.element",
FieldStatistic.fieldStatsFor(listElement, 10600)),
- optional(
- 11400, "simple_struct.int",
FieldStatistic.fieldStatsFor(structInt, 11400)),
- optional(
- 11600,
- "simple_struct.string",
- FieldStatistic.fieldStatsFor(structString, 11600)),
- optional(14400, "b.key",
FieldStatistic.fieldStatsFor(mapKey, 14400)),
- optional(14800, "b.value",
FieldStatistic.fieldStatsFor(mapValue, 14800)),
- optional(16000, "variant",
FieldStatistic.fieldStatsFor(variantField, 16000)),
- optional(
- StatsUtil.MAX_DATA_STATS_FIELD_ID,
- "u",
- FieldStatistic.fieldStatsFor(
- uuidField, StatsUtil.MAX_DATA_STATS_FIELD_ID)))));
- Schema statsSchema = new Schema(StatsUtil.contentStatsFor(schema));
-
assertThat(statsSchema.asStruct()).isEqualTo(expectedStatsSchema.asStruct());
+ public void testStatOffset() {
+ assertThat(StatsUtil.statOffset(10_000)).isEqualTo(0);
+ assertThat(StatsUtil.statOffset(10_201)).isEqualTo(1);
+ assertThat(StatsUtil.statOffset(10_211)).isEqualTo(11);
+ assertThat(StatsUtil.statOffset(10_399)).isEqualTo(199);
+ assertThat(StatsUtil.statOffset(10_400)).isEqualTo(0);
+ assertThat(StatsUtil.statOffset(199_999_999)).isEqualTo(199);
}
@Test
- public void contentStatsChildNamesAreUnique() {
- Schema schema =
- new Schema(
- required(1, "a", Types.StructType.of(required(2, "x",
Types.IntegerType.get()))),
- required(3, "b", Types.StructType.of(required(4, "x",
Types.IntegerType.get()))));
+ public void testStatOffsetOutsideRange() {
+ assertThatThrownBy(() -> StatsUtil.statOffset(200_000_000))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid stats field ID: 200000000");
+ assertThatThrownBy(() -> StatsUtil.statOffset(200_000_001))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid stats field ID: 200000001");
+ assertThatThrownBy(() -> StatsUtil.statOffset(8_800))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid stats field ID: 8800");
+ assertThatThrownBy(() -> StatsUtil.statOffset(8_801))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid stats field ID: 8801");
+ }
- Types.StructType contentStats =
StatsUtil.contentStatsFor(schema).type().asStructType();
+ private static final List<Type> FIXED_WIDTH_TYPES =
+ List.of(
+ Types.BooleanType.get(),
+ Types.IntegerType.get(),
+ Types.LongType.get(),
+ Types.DateType.get(),
+ Types.TimeType.get(),
+ Types.TimestampType.withoutZone(),
+ Types.TimestampType.withZone(),
+ Types.TimestampNanoType.withoutZone(),
+ Types.TimestampNanoType.withZone(),
+ Types.DecimalType.of(9, 2),
+ Types.UUIDType.get(),
+ Types.FixedType.ofLength(16));
+
+ @ParameterizedTest
+ @FieldSource("FIXED_WIDTH_TYPES")
+ public void testFixedWidthPrimitiveStruct(Type type) {
+ // fixed-width, non-floating-point types track bounds (including tight
bounds) but have no NaN
+ // count or average value size
+ Types.StructType expected =
+ Types.StructType.of(
+ Types.NestedField.optional(30_001, "lower_bound", type),
+ Types.NestedField.optional(30_002, "upper_bound", type),
+ Types.NestedField.optional(30_003, "tight_bounds",
Types.BooleanType.get()),
+ Types.NestedField.optional(30_004, "value_count",
Types.LongType.get()),
+ Types.NestedField.optional(30_005, "null_value_count",
Types.LongType.get()));
+
+ Types.StructType actual =
+ StatsUtil.fieldStatsStruct(true, type, 30_000,
MetricsModes.Full.get());
+
+ assertSameStructure(expected, actual);
+ }
-
assertThat(contentStats.fields().stream().map(Types.NestedField::name).toList())
- .doesNotHaveDuplicates()
- .containsExactly("a.x", "b.x");
+ private static final List<Type> VARIABLE_WIDTH_TYPES =
+ List.of(Types.StringType.get(), Types.BinaryType.get());
+
+ @ParameterizedTest
+ @FieldSource("VARIABLE_WIDTH_TYPES")
+ public void testVariableWidthPrimitiveStruct(Type type) {
+ // variable-width types track bounds (including tight bounds) and an
average value size, but no
+ // NaN count
+ Types.StructType expected =
+ Types.StructType.of(
+ Types.NestedField.optional(30_001, "lower_bound", type),
+ Types.NestedField.optional(30_002, "upper_bound", type),
+ Types.NestedField.optional(30_003, "tight_bounds",
Types.BooleanType.get()),
+ 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()));
+
+ Types.StructType actual =
+ StatsUtil.fieldStatsStruct(true, type, 30_000,
MetricsModes.Full.get());
+
+ assertSameStructure(expected, actual);
}
- @Test
- public void contentStatsSkipsFieldsOutsideStatsRange() {
- Types.NestedField validField = required(0, "i", Types.IntegerType.get());
- Types.NestedField outOfRangeField =
- required(StatsUtil.MAX_DATA_FIELD_ID + 1, "out_of_range",
Types.IntegerType.get());
- Schema schema = new Schema(validField, outOfRangeField);
- Schema expectedStatsSchema =
- new Schema(
- optional(
- 146,
- "content_stats",
- Types.StructType.of(
- optional(10000, "i",
FieldStatistic.fieldStatsFor(validField, 10000)))));
- Schema statsSchema = new Schema(StatsUtil.contentStatsFor(schema));
-
assertThat(statsSchema.asStruct()).isEqualTo(expectedStatsSchema.asStruct());
+ private static final List<Type> GEO_TYPES =
+ List.of(
+ Types.GeometryType.crs84(),
+ Types.GeometryType.of("srid:3857"),
+ Types.GeographyType.crs84(),
+ Types.GeographyType.of("srid:4269"));
+
+ @ParameterizedTest
+ @FieldSource("GEO_TYPES")
+ public void testGeoStruct(Type type) {
+ // geometry and geography use bounding-box structs for their bounds, do
not track tight bounds,
+ // and record an average value size
+ Types.StructType lowerBound =
+ Types.StructType.of(
+ Types.NestedField.required(30_010, "x", Types.DoubleType.get()),
+ Types.NestedField.required(30_011, "y", Types.DoubleType.get()),
+ Types.NestedField.optional(30_012, "z", Types.DoubleType.get()),
+ Types.NestedField.optional(30_013, "m", Types.DoubleType.get()));
+ Types.StructType upperBound =
+ Types.StructType.of(
+ Types.NestedField.required(30_014, "x", Types.DoubleType.get()),
+ Types.NestedField.required(30_015, "y", Types.DoubleType.get()),
+ Types.NestedField.optional(30_016, "z", Types.DoubleType.get()),
+ Types.NestedField.optional(30_017, "m", Types.DoubleType.get()));
+ Types.StructType expected =
+ Types.StructType.of(
+ Types.NestedField.optional(30_001, "lower_bound", lowerBound),
+ 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()));
+
+ Types.StructType actual =
+ StatsUtil.fieldStatsStruct(true, type, 30_000,
MetricsModes.Full.get());
+
+ assertSameStructure(expected, actual);
}
- @Test
- public void conditionalFieldInclusionForInteger() {
- assertThat(
- fieldStatsNames(
- FieldStatistic.fieldStatsFor(required(1, "x",
Types.IntegerType.get()), 10000)))
- .containsExactly(
- LOWER_BOUND.fieldName(),
- UPPER_BOUND.fieldName(),
- TIGHT_BOUNDS.fieldName(),
- VALUE_COUNT.fieldName())
- .doesNotContain(
- NULL_VALUE_COUNT.fieldName(),
- NAN_VALUE_COUNT.fieldName(),
- AVG_VALUE_SIZE_IN_BYTES.fieldName());
+ private static final List<Type> FLOATING_POINT_TYPES =
+ List.of(Types.FloatType.get(), Types.DoubleType.get());
+
+ @ParameterizedTest
+ @FieldSource("FLOATING_POINT_TYPES")
+ public void testFloatingPointStruct(Type type) {
+ // floating-point types track bounds (including tight bounds) and a NaN
count, but have no
+ // average value size
+ Types.StructType expected =
+ Types.StructType.of(
+ Types.NestedField.optional(30_001, "lower_bound", type),
+ Types.NestedField.optional(30_002, "upper_bound", type),
+ Types.NestedField.optional(30_003, "tight_bounds",
Types.BooleanType.get()),
+ 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_006, "nan_value_count",
Types.LongType.get()));
+
+ Types.StructType actual =
+ StatsUtil.fieldStatsStruct(true, type, 30_000,
MetricsModes.Full.get());
+
+ assertSameStructure(expected, actual);
+ }
- assertThat(
- fieldStatsNames(
- FieldStatistic.fieldStatsFor(optional(1, "x",
Types.IntegerType.get()), 10000)))
- .containsExactly(
- LOWER_BOUND.fieldName(),
- UPPER_BOUND.fieldName(),
- TIGHT_BOUNDS.fieldName(),
- VALUE_COUNT.fieldName(),
- NULL_VALUE_COUNT.fieldName())
- .doesNotContain(NAN_VALUE_COUNT.fieldName(),
AVG_VALUE_SIZE_IN_BYTES.fieldName());
+ private static final Types.StructType POINT =
+ Types.StructType.of(
+ Types.NestedField.required(1, "x", Types.FloatType.get()),
+ Types.NestedField.required(2, "y", Types.FloatType.get()));
+
+ private static final List<Type> NESTED_TYPES =
+ List.of(
+ Types.ListType.ofRequired(1, Types.IntegerType.get()),
+ Types.ListType.ofOptional(1, POINT),
+ Types.MapType.ofRequired(1, 2, Types.StringType.get(),
Types.StringType.get()),
+ Types.MapType.ofOptional(1, 2, Types.StringType.get(), POINT));
+
+ @ParameterizedTest
+ @FieldSource("NESTED_TYPES")
+ public void testNestedTypesHaveNoStats(Type type) {
+ // list and map types are not tracked and produce no stats struct
+ assertThat(StatsUtil.fieldStatsStruct(true, type, 30_000,
MetricsModes.Full.get())).isNull();
}
@Test
- public void conditionalFieldInclusionForFloatAndDouble() {
- assertThat(
- fieldStatsNames(
- FieldStatistic.fieldStatsFor(required(1, "x",
Types.FloatType.get()), 10000)))
- .containsExactly(
- LOWER_BOUND.fieldName(),
- UPPER_BOUND.fieldName(),
- TIGHT_BOUNDS.fieldName(),
- VALUE_COUNT.fieldName(),
- NAN_VALUE_COUNT.fieldName())
- .doesNotContain(NULL_VALUE_COUNT.fieldName(),
AVG_VALUE_SIZE_IN_BYTES.fieldName());
-
- assertThat(
- fieldStatsNames(
- FieldStatistic.fieldStatsFor(optional(1, "x",
Types.DoubleType.get()), 10000)))
- .containsExactly(
- LOWER_BOUND.fieldName(),
- UPPER_BOUND.fieldName(),
- TIGHT_BOUNDS.fieldName(),
- VALUE_COUNT.fieldName(),
- NULL_VALUE_COUNT.fieldName(),
- NAN_VALUE_COUNT.fieldName());
+ public void testRequiredField() {
+ // a required column does not produce a null_value_count field
+ Type type = Types.IntegerType.get();
+ Types.StructType expected =
+ Types.StructType.of(
+ Types.NestedField.optional(30_001, "lower_bound", type),
+ Types.NestedField.optional(30_002, "upper_bound", type),
+ Types.NestedField.optional(30_003, "tight_bounds",
Types.BooleanType.get()),
+ Types.NestedField.optional(30_004, "value_count",
Types.LongType.get()));
+
+ Types.StructType actual =
+ StatsUtil.fieldStatsStruct(false, type, 30_000,
MetricsModes.Full.get());
+
+ assertSameStructure(expected, actual);
}
@Test
- public void conditionalFieldInclusionForString() {
- assertThat(
- fieldStatsNames(
- FieldStatistic.fieldStatsFor(required(1, "x",
Types.StringType.get()), 10000)))
- .containsExactly(
- LOWER_BOUND.fieldName(),
- UPPER_BOUND.fieldName(),
- TIGHT_BOUNDS.fieldName(),
- VALUE_COUNT.fieldName(),
- AVG_VALUE_SIZE_IN_BYTES.fieldName())
- .doesNotContain(NULL_VALUE_COUNT.fieldName(),
NAN_VALUE_COUNT.fieldName());
-
+ public void testStringNoneMode() {
+ // none mode produces no stats struct
assertThat(
- fieldStatsNames(
- FieldStatistic.fieldStatsFor(optional(1, "x",
Types.StringType.get()), 10000)))
- .containsExactly(
- LOWER_BOUND.fieldName(),
- UPPER_BOUND.fieldName(),
- TIGHT_BOUNDS.fieldName(),
- VALUE_COUNT.fieldName(),
- NULL_VALUE_COUNT.fieldName(),
- AVG_VALUE_SIZE_IN_BYTES.fieldName());
+ StatsUtil.fieldStatsStruct(
+ true, Types.StringType.get(), 30_000, MetricsModes.None.get()))
+ .isNull();
}
@Test
- public void conditionalFieldInclusionForBinary() {
- assertThat(
- fieldStatsNames(
- FieldStatistic.fieldStatsFor(optional(1, "x",
Types.BinaryType.get()), 10000)))
- .containsExactly(
- LOWER_BOUND.fieldName(),
- UPPER_BOUND.fieldName(),
- TIGHT_BOUNDS.fieldName(),
- VALUE_COUNT.fieldName(),
- NULL_VALUE_COUNT.fieldName(),
- AVG_VALUE_SIZE_IN_BYTES.fieldName())
- .doesNotContain(NAN_VALUE_COUNT.fieldName());
+ public void testStringCountsMode() {
+ // counts mode drops the bounds (and tight_bounds) but keeps counts and
the value size
+ Types.StructType expected =
+ Types.StructType.of(
+ 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()));
+
+ Types.StructType actual =
+ StatsUtil.fieldStatsStruct(true, Types.StringType.get(), 30_000,
MetricsModes.Counts.get());
+
+ assertSameStructure(expected, actual);
+ }
- assertThat(
- fieldStatsNames(
- FieldStatistic.fieldStatsFor(required(1, "x",
Types.BinaryType.get()), 10000)))
- .containsExactly(
- LOWER_BOUND.fieldName(),
- UPPER_BOUND.fieldName(),
- TIGHT_BOUNDS.fieldName(),
- VALUE_COUNT.fieldName(),
- AVG_VALUE_SIZE_IN_BYTES.fieldName())
- .doesNotContain(NULL_VALUE_COUNT.fieldName(),
NAN_VALUE_COUNT.fieldName());
+ private static final List<MetricsModes.MetricsMode> BOUNDS_MODES =
+ List.of(MetricsModes.Truncate.withLength(16), MetricsModes.Full.get());
+
+ @ParameterizedTest
+ @FieldSource("BOUNDS_MODES")
+ public void testStringBoundsModes(MetricsModes.MetricsMode mode) {
+ // truncate and full both retain the full bounds struct
+ Type string = Types.StringType.get();
+ Types.StructType expected =
+ Types.StructType.of(
+ Types.NestedField.optional(30_001, "lower_bound", string),
+ Types.NestedField.optional(30_002, "upper_bound", string),
+ Types.NestedField.optional(30_003, "tight_bounds",
Types.BooleanType.get()),
+ 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()));
+
+ Types.StructType actual = StatsUtil.fieldStatsStruct(true, string, 30_000,
mode);
+
+ assertSameStructure(expected, actual);
}
@Test
- public void conditionalFieldInclusionForGeometry() {
- Types.StructType requiredStats =
- FieldStatistic.fieldStatsFor(required(1, "x",
Types.GeometryType.crs84()), 10000);
- assertThat(fieldStatsNames(requiredStats))
- .containsExactly(LOWER_BOUND.fieldName(), UPPER_BOUND.fieldName(),
VALUE_COUNT.fieldName())
- .doesNotContain(
- TIGHT_BOUNDS.fieldName(),
- NULL_VALUE_COUNT.fieldName(),
- NAN_VALUE_COUNT.fieldName(),
- AVG_VALUE_SIZE_IN_BYTES.fieldName());
- assertGeoBoundStructs(requiredStats, 10000);
-
- Types.StructType optionalStats =
- FieldStatistic.fieldStatsFor(optional(1, "x",
Types.GeometryType.crs84()), 10000);
- assertThat(fieldStatsNames(optionalStats))
- .containsExactly(
- LOWER_BOUND.fieldName(),
- UPPER_BOUND.fieldName(),
- VALUE_COUNT.fieldName(),
- NULL_VALUE_COUNT.fieldName())
- .doesNotContain(
- TIGHT_BOUNDS.fieldName(),
- NAN_VALUE_COUNT.fieldName(),
- AVG_VALUE_SIZE_IN_BYTES.fieldName());
- assertGeoBoundStructs(optionalStats, 10000);
+ public void testContentStatsReadSchema() {
Review Comment:
**Coverage gap:** `statsWriteSchema` has no tests. `statsReadSchema` has
four dedicated tests (this one + `testContentStatsReadSchemaSubset` /
`NestedStruct` / `OmitsListAndMap`), but the new write-side entry point — which
branches on per-column `metricsConfig.columnMode(id)` — is not exercised in a
schema-generation test.
The branching behavior worth covering:
- Default mode = Full for all columns → parallel to
`testContentStatsReadSchema`
- Per-column override (e.g., one column Counts, one Full) → the
mode-per-column dispatch
- One column set to None → skipped in the output struct
- Sorted columns → default upgraded to Truncate (see
`sortedColumnDefaultMode` at MetricsConfig.java:326)
Without these, the per-column `columnMode(id)` path (StatsUtil.java:141) has
zero regression coverage.
--
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]
