Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vishnuprakaz wants to merge 5 commits into
base: main
Choose a base branch
from
+161 −1
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4bc33e8
Core: Fix geo bounds in v4 legacy stat maps
vishnuprakaz Aug 3, 2026
d0c91c8
Round-trip XYZ and XYM geo bounds through Conversions in tests
vishnuprakaz Aug 3, 2026
dfaaac0
Check geo bound shape explicitly instead of any struct
vishnuprakaz Aug 5, 2026
c169efb
Test isGeoBoundType directly in TestStatsUtil
vishnuprakaz Aug 5, 2026
ac69057
Revert explicit geo bound type check
vishnuprakaz Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion core/src/main/java/org/apache/iceberg/ContentStatsBackedMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.AbstractMap;
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.geospatial.GeospatialBound;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.types.Conversions;
import org.apache.iceberg.types.Type;
Expand Down Expand Up @@ -165,7 +166,39 @@ private static <V> V statValue(FieldStats<?> fieldStats, Kind kind) {

private static ByteBuffer bound(FieldStats<?> fieldStats, Object bound, String boundFieldName) {
Type boundType = fieldStats.type().fieldType(boundFieldName);
if (boundType == null) {
return null;
}

if (boundType.isStructType()) {

@szehon-ho szehon-ho Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: rather than treating any struct bound as geo via isStructType(), can we check that this is a geometry/geography bound explicitly? That works today only because StatsUtil uses struct bounds solely for geo types, and an explicit check would avoid silently taking this path if another type ever gains struct bounds.

@vishnuprakaz vishnuprakaz Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @szehon-ho, Added StatsUtil.isGeoBoundType which checks the bounding-box shape explicitly with a direct test in TestStatsUtil.
So, a non-geo struct bound now falls through to Conversions.toByteBuffer and fails loudly instead of silently encoding as geo.

// geo bounds are bounding-box structs; convert to the spec's single-point encoding
return geoBound((StructLike) bound);
}

// toByteBuffer returns null for a null bound
return boundType == null ? null : Conversions.toByteBuffer(boundType, bound);
return Conversions.toByteBuffer(boundType, bound);
}

/** Encodes a geo bounding-box struct (x, y, z, m) as the spec's single-point bound. */
private static ByteBuffer geoBound(StructLike bound) {
if (bound == null) {
return null;
}

// field order is fixed by the stats schema: x, y, z, m
double coordX = bound.get(0, Double.class);
double coordY = bound.get(1, Double.class);
Double coordZ = bound.get(2, Double.class);
Double coordM = bound.get(3, Double.class);

if (coordZ != null && coordM != null) {
return GeospatialBound.createXYZM(coordX, coordY, coordZ, coordM).toByteBuffer();
} else if (coordZ != null) {
return GeospatialBound.createXYZ(coordX, coordY, coordZ).toByteBuffer();
} else if (coordM != null) {
return GeospatialBound.createXYM(coordX, coordY, coordM).toByteBuffer();
} else {
return GeospatialBound.createXY(coordX, coordY).toByteBuffer();
}
}
}
127 changes: 127 additions & 0 deletions core/src/test/java/org/apache/iceberg/TestContentStatsBackedMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.nio.ByteBuffer;
import java.util.List;
import java.util.Map;
import org.apache.iceberg.geospatial.GeospatialBound;
import org.apache.iceberg.types.Conversions;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -99,6 +100,132 @@ public void testUpperBounds() {
Map.entry(3, Conversions.toByteBuffer(Types.DoubleType.get(), 9.0)));
}

@Test
public void testGeoBoundsUseSinglePointEncoding() {
Schema geoSchema =
new Schema(
required(1, "id", Types.LongType.get()),
optional(10, "geom", Types.GeometryType.crs84()),
optional(11, "geog", Types.GeographyType.crs84()));
Types.StructType statsType = StatsUtil.statsReadSchema(geoSchema, List.of(1, 10, 11));

ContentStatsStruct stats = new ContentStatsStruct(statsType);
stats.setStats(
1,
StatsTestUtil.mockFieldStats(
statsType.field("id").type().asStructType(), 1, 1L, 5L, 26L, null, null));
stats.setStats(
10,
StatsTestUtil.mockFieldStats(
statsType.field("geom").type().asStructType(),
10,
TestHelpers.Row.of(1.0, 2.0, null, null),
TestHelpers.Row.of(5.0, 6.0, null, null),
26L,
2L,
null));
stats.setStats(
11,
StatsTestUtil.mockFieldStats(
statsType.field("geog").type().asStructType(),
11,
TestHelpers.Row.of(-1.0, -2.0, 3.0, 4.0),
TestHelpers.Row.of(7.0, 8.0, 9.0, 10.0),
26L,
0L,
null));

// geometry and geography bounds are stored as bounding-box structs (x, y, z, m) but must be
// presented in the legacy maps using the spec's single-point encoding
Map<Integer, ByteBuffer> lower = ContentStatsBackedMap.lowerBounds(stats);
assertThat(lower)
.containsOnly(
Map.entry(1, Conversions.toByteBuffer(Types.LongType.get(), 1L)),
Map.entry(10, GeospatialBound.createXY(1.0, 2.0).toByteBuffer()),
Map.entry(11, GeospatialBound.createXYZM(-1.0, -2.0, 3.0, 4.0).toByteBuffer()));

Map<Integer, ByteBuffer> upper = ContentStatsBackedMap.upperBounds(stats);
assertThat(upper)
.containsOnly(
Map.entry(1, Conversions.toByteBuffer(Types.LongType.get(), 5L)),
Map.entry(10, GeospatialBound.createXY(5.0, 6.0).toByteBuffer()),
Map.entry(11, GeospatialBound.createXYZM(7.0, 8.0, 9.0, 10.0).toByteBuffer()));

// the encoding must round-trip through the geo conversion used by legacy readers

@uros-b uros-b Aug 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Conversions.fromByteBuffer round-trip assertion covers only the XY (geometry lower) and XYZM (geography upper) variants; the XYZ (24-byte) and XYM (32-byte, NaN z-slot) encodings are asserted only at raw ByteBuffer byte-equality via .isEqualTo(GeospatialBound.createXYZ(...).toByteBuffer()), not round-tripped back through Conversions.fromByteBuffer. GeospatialBound.toByteBuffer/fromByteBuffer are separately tested so the risk is low, but XYM is the encoding most likely to harbor a NaN-slot/endianness edge case and is the exact integration point the fix depends on; adding fromByteBuffer round-trips for XYZ and XYM would close the integration coverage gap. This goes for testGeoBoundsUseSinglePointEncoding, testGeoBoundWithZOnlyAndMOnly, etc.

@uros-b uros-b Aug 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also cc @szehon-ho

GeospatialBound geomLower =
Conversions.fromByteBuffer(Types.GeometryType.crs84(), lower.get(10));
assertThat(geomLower).isEqualTo(GeospatialBound.createXY(1.0, 2.0));
GeospatialBound geogUpper =
Conversions.fromByteBuffer(Types.GeographyType.crs84(), upper.get(11));
assertThat(geogUpper).isEqualTo(GeospatialBound.createXYZM(7.0, 8.0, 9.0, 10.0));
}

@Test
public void testGeoBoundWithZOnlyAndMOnly() {
Schema geoSchema = new Schema(optional(10, "geom", Types.GeometryType.crs84()));
Types.StructType statsType = StatsUtil.statsReadSchema(geoSchema, List.of(10));

ContentStatsStruct stats = new ContentStatsStruct(statsType);
stats.setStats(
10,
StatsTestUtil.mockFieldStats(
statsType.field("geom").type().asStructType(),
10,
TestHelpers.Row.of(1.0, 2.0, 3.0, null),
TestHelpers.Row.of(5.0, 6.0, null, 7.0),
26L,
2L,
null));

Map<Integer, ByteBuffer> lower = ContentStatsBackedMap.lowerBounds(stats);
assertThat(lower.get(10)).isEqualTo(GeospatialBound.createXYZ(1.0, 2.0, 3.0).toByteBuffer());

Map<Integer, ByteBuffer> upper = ContentStatsBackedMap.upperBounds(stats);
assertThat(upper.get(10)).isEqualTo(GeospatialBound.createXYM(5.0, 6.0, 7.0).toByteBuffer());

// the encoding must round-trip through the geo conversion used by legacy readers
GeospatialBound geomLower =
Conversions.fromByteBuffer(Types.GeometryType.crs84(), lower.get(10));
assertThat(geomLower).isEqualTo(GeospatialBound.createXYZ(1.0, 2.0, 3.0));
GeospatialBound geomUpper =
Conversions.fromByteBuffer(Types.GeometryType.crs84(), upper.get(10));
assertThat(geomUpper).isEqualTo(GeospatialBound.createXYM(5.0, 6.0, 7.0));
}

@Test
public void testGeoFieldWithoutLowerBoundIsAbsentFromView() {
Schema geoSchema =
new Schema(
required(1, "id", Types.LongType.get()),
optional(10, "geom", Types.GeometryType.crs84()));
Types.StructType statsType = StatsUtil.statsReadSchema(geoSchema, List.of(1, 10));

ContentStatsStruct stats = new ContentStatsStruct(statsType);
stats.setStats(
1,
StatsTestUtil.mockFieldStats(
statsType.field("id").type().asStructType(), 1, 1L, 5L, 26L, null, null));
stats.setStats(
10,
StatsTestUtil.mockFieldStats(
statsType.field("geom").type().asStructType(),
10,
null,
TestHelpers.Row.of(5.0, 6.0, null, null),
26L,
2L,
null));

Map<Integer, ByteBuffer> lower = ContentStatsBackedMap.lowerBounds(stats);
assertThat(lower.get(10)).isNull();
assertThat(lower.containsKey(10)).isFalse();
assertThat(lower)
.containsOnly(Map.entry(1, Conversions.toByteBuffer(Types.LongType.get(), 1L)));

Map<Integer, ByteBuffer> upper = ContentStatsBackedMap.upperBounds(stats);
assertThat(upper.get(10)).isEqualTo(GeospatialBound.createXY(5.0, 6.0).toByteBuffer());
}

@Test
public void testGetReturnsNullForMissingKey() {
Map<Integer, Long> map = ContentStatsBackedMap.valueCounts(POPULATED_STATS);
Expand Down
Loading