Core: Add geometry bounds collector

[huan233usc]

Summary

Geometry bounds metrics need to inspect WKB coordinates without adding a geometry-library dependency to Iceberg. This adds a package-private collector in core that accumulates the lower and upper bounds of geometry values from their WKB encoding, while leaving each input ByteBuffer position and limit unchanged.

• Walk the seven OGC WKB geometry types, including nested collections and mixed-endian children
• Accumulate X and Y independently, skipping NaN values per coordinate dimension
• Read past Z and M ordinates, keeping the XY bounds rather than rejecting the value
• Reject malformed input: unknown types, invalid collection children, invalid counts, truncation, excessive nesting, and trailing data

This PR only adds the reusable bounds collector. Geometry field metrics and Parquet/Spark writer integration remain in follow-up PRs.

Scope: geometry only

The collector applies to geometry columns, whose edge interpolation is "always linear/planar" (format/spec.md:284) and whose calculations "are always Cartesian" (format/spec.md:301). A box containing every vertex therefore contains the whole geometry, so scanning vertices is both correct and minimal.

It is deliberately not used for geography, which needs a different algorithm: edges are geodesics (format/spec.md:318) that can reach beyond their endpoints in latitude, longitude is periodic, and format/spec.md:772 both permits xmin > xmax for a tighter antimeridian-crossing box and restricts bounds to the canonical [-180..180]/[-90..90] ranges. That belongs in a separate collector, and the numeric semantics are worth agreeing on before implementing — happy to take that discussion to an issue.

Why an Iceberg-side WKB scan

Parquet 1.17.1 computes a geospatial bounding box itself and exposes it in the footer via ColumnChunkMetaData.getGeospatialStatistics(), so for the Parquet path this parser is a second pass over each value. It is still preferred, for three reasons:

1. NaN semantics differ from the spec. format/spec.md:774 requires NaN to be skipped per dimension, so POINT (1 NaN) contributes to X while leaving Y unset. Parquet's BoundingBox.isXValid()/isYValid() instead invalidate a whole dimension once a bound is NaN, which produces different bounds for the same input.
2. It reaches the non-Parquet paths. Avro (Core: Read and write geometry and geography values in Avro #17119) has no footer statistics to read, and the same collector serves ORC later.
3. It does not need JTS. Parquet's builder parses WKB through org.locationtech.jts.io.WKBReader; Iceberg has no JTS dependency in api or core.

Reading the Parquet footer instead remains a reasonable optimization for the Parquet writer specifically, and is worth revisiting in #17161 once the semantics above are reconciled. Flagging overlap with #12347.

Test Plan

• Cover representative bounds cases for points, line strings, polygons, multi-geometries, empty values, and nested geometry collections
• Cover both WKB byte orders, a big-endian child inside a little-endian parent, and a buffer with a non-zero array offset
• Cover bounds accumulated across values with independently missing coordinates, and that no box is produced when a single dimension is missing
• Cover that Z, M, and ZM values keep their XY bounds, including a Z geometry nested inside a collection and mixed dimensions across values
• Cover rejection of unknown type codes, a type code with the EWKB SRID flag, invalid byte order, excessive nesting, truncation, and trailing data
• Run the targeted core test class, formatting checks, style checks, and compatibility checks

Verification Commands

./gradlew :iceberg-core:test --tests org.apache.iceberg.TestGeometryBoundsCollector
./gradlew :iceberg-core:spotlessCheck :iceberg-api:spotlessCheck
./gradlew :iceberg-core:checkstyleMain :iceberg-core:checkstyleTest
./gradlew :iceberg-core:revapi :iceberg-api:revapi

---

AI Disclosure

• Model: GPT-5
• Platform/Tool: Codex
• Human Oversight: partially reviewed
• Prompt Summary: Split a reusable geometry bounds collector and focused tests from the geometry Parquet metrics work into a foundational PR.

[szehon-ho]

Thanks for splitting this out — the WKB walk is carefully bounded and the NaN/empty semantics match format/spec.md:772 exactly, including the per-dimension skip and suppressing the box when X or Y is missing.

The main thing I'd like to resolve is that rejecting XYZ/XYM/XYZM fails the write, since those are spec-legal geometries. Beyond that: a placement question on the new public surface in api/, a documentation request on the polygon shell assumption, and some minor notes. Details inline.

One broader question that isn't inline: Parquet 1.17.1 already computes a geospatial bounding box itself for columns annotated GEOMETRY/GEOGRAPHY (ColumnWriterBase threads GeospatialStatistics through writePage, and ColumnChunkMetaData.getGeospatialStatistics() exposes it in the footer, which is where ParquetMetrics already reads stats from). Since TypeToMessageType emits those annotations today, the Parquet path in #17161 would be parsing every WKB value a second time. An Iceberg-side parser is still clearly justified for the Avro path (#17119), so this isn't an argument against the class — but could the PR description say why the Iceberg-side scan is preferred over the footer for Parquet? Worth coordinating with #12347 too, which covers overlapping ground.

Two questions that didn't fit inline:

Should invalid-but-parseable WKB fail the write at all? Rejecting a LINESTRING child inside a MULTIPOINT is defensible, but together with the Z/M rejection there's a pattern of aborting writes over input that other producers accept.

How will geography bounds work? format/spec.md:770 restricts geography bounds to [-180..180]/[-90..90] and permits xmin > xmax for a tighter antimeridian-crossing box. A plain XY min/max is valid but not minimal, and will fall outside the canonical ranges if the input longitudes do. Separate collector, or a mode on this one?

[szehon-ho]

Suggest not failing on Z/M. Either read past the extra ordinates and keep the XY bounds — the numDimensions(dimensionGroup, typeCode) handling the earlier version of this code had in #17161 — or, if you'd rather keep the parser strictly 2D, mark the column's bounds unavailable for the file instead of throwing. The first is better since you still get usable bounds.

Z/M geometries are legal Iceberg data: Appendix G specifies the ISO WKB serializations "supporting XY, XYZ, XYM, XYZM", and the bounds section makes Z and M bounds optional rather than constraining input dimensionality. So POINT Z (1 2 3) is valid, but typeCode / 1000 == 0 rejects it.

As written the rejection fails the write rather than dropping bounds. In #17161, GeometryWriter.write() calls metricsBuilder.addValue(buffer) as its first statement with no try/catch, and ParquetWriter.add(T) calls model.write(0, value) unguarded, so the IllegalArgumentException propagates out of FileAppender.add() and aborts the task — a Spark INSERT into a table with 3D geometry would fail the job.

One caveat if you take the second route: it has to invalidate the whole column, not just skip the offending value. Skipping the value would leave bounds accumulated from the other values, which no longer contain every object in the file — format/spec.md:768 requires the box to contain all objects — and under-covering bounds get the file pruned for queries that should match it. Omitting bounds entirely is always safe, since metrics are optional. Parquet's own GeospatialStatistics does exactly this with abort()/isValid(), which may be a useful model.

Minor, on the same statement: this precondition runs before the type is known to be valid, so a garbage code such as 0xFFFFFFFF — for example EWKB with the SRID flag set — is also reported as "only 2D geometries are supported", which points a user at the wrong problem. Including the offending typeCode in the message, or validating the base geometry type first, would help.

[huan233usc]

Took the first option — numDimensions() now derives the ordinate count from the type code's dimension group and readCoordinate skips past Z and M, so XYZ/XYM/XYZM values contribute their XY extent instead of failing or discarding the box.

Thanks for pointing at #17161: that handling was lost when this PR was split out, not deliberately dropped. The aborted state the previous revision used is gone with it, which also restores the strict trailing-data check it had to relax.

[szehon-ho]

Suggest promoting this from an inline comment into the class javadoc, since it's a contract callers need to know rather than a local implementation note.

Something like: the box is derived from each polygon's exterior ring only, which assumes OGC-valid polygons whose interior rings lie within the shell. That's the same envelope JTS computes for a Polygon, so Iceberg matching it is reasonable — but Iceberg never validates geometry validity, and for a polygon with a hole extending past the shell the resulting box would not contain the geometry, which means the file gets pruned for queries that should match rather than erroring. Stating it in the javadoc makes that a known, documented assumption instead of something a future reader has to rediscover.

Reading all rings is the alternative if you'd rather not depend on validity — the cost is one pass over bytes already in cache — but I'm fine with the shell-only behavior if it's documented.

[huan233usc]

Promoted to the class javadoc.

[szehon-ho]

Suggest making this package-private in core alongside GeometryFieldMetrics, unless there's a planned api-side caller.

It works mechanically: in #17161 neither ParquetValueWriters.GeometryWriter nor Spark's SparkParquetWriters.GeometryWriter references the parser — both only construct a GeometryFieldMetrics.Builder and call addValue/build. Avro would go through the same public builder. So the parser can stay an implementation detail and never enter the tracked surface, which keeps it free to change when Z/M and geography land. The whole FieldMetrics family already lives in core, and VariantUtil in api is the close precedent for this shape: package-private defensive ByteBuffer parsing reachable only through public types in its package.

To be clear, moving to core alone wouldn't help much — core is in REVAPI_PROJECTS too (build.gradle:112); it's a weaker stability tier per AGENTS.md, not an escape from tracking. The package-private part is what does the work.

The counter-argument for keeping it here is real: BoundingBox and GeospatialBound are in this package, and a package-private class in api would be unusable from core, so those two options are mutually exclusive. Nothing in api needs WKB parsing today — the only BoundingBox users are GeospatialBound and GeospatialPredicateEvaluators, which compares boxes. But if the geospatial literal work in #14101 or the ST_INTERSECTS pushdown in #17175 will need to turn a WKB constant into a box inside api, then this placement is already right. Which is it?

[huan233usc]

Neither needs a WKB walk in api.

#14101's BoundingBoxLiteral deserializes through BoundingBox.fromByteBuffer, which reads the serialized box form — two fixed-width corner points — not a WKB geometry; every other construction site in that PR passes two GeospatialBounds directly. #17175 compares a constant box against bounds already read from manifests and touches BoundingBox.java by a single line.

So the parser is now package-private in core, next to where GeometryFieldMetrics will live, and stays off the tracked surface.

[szehon-ho]

Suggest dropping the save/restore until a caller needs it.

xSaved/ySaved plus the catch-restore-rethrow at lines 71-75 exist so a caller can keep accumulating after an invalid value, but the planned caller in #17161 lets the exception abort the write, so the restored state is never read. Per "keep the first version of a PR minimal" this could come with the caller that actually swallows the exception. If it stays, the javadoc on add should state the guarantee explicitly, since it's the sort of thing a later edit would silently break.

[huan233usc]

Dropped. With Z/M no longer aborting, the only remaining failures are malformed input, which the planned caller lets propagate.

[szehon-ho]

Suggest dropping the test prefixes from the method names.

AGENTS.md asks for this on new tests and the project is actively removing them — main currently has "Core: Drop test prefix from metadata table test methods" at HEAD. The guidance also asks for package-private test classes and methods, though the existing api tests are overwhelmingly public, so that part is a migration in progress rather than a deviation from local practice; up to you.

[huan233usc]

Prefixes were already dropped; the class and its methods are package-private now too.

[szehon-ho]

These cases will need to change if the Z/M handling changes per the comment on GeometryBoundsCollector.java:105 — flagging so they aren't missed.

[huan233usc]

Updated: extraDimensionsAreIgnored asserts the XY box for POINT Z, POINT M, POINT ZM and LINESTRING Z, plus extraDimensionsNestedInCollectionAreIgnored for a 3D child inside a collection and boundsAcrossValuesWithDifferentDimensions across values of differing dimensionality.

[szehon-ho]

Suggest adding a case that passes a slice() at a non-zero position, backed by a larger array with a non-zero array offset.

Every case today uses ByteBuffer.wrap(...) at position 0. The implementation is correct for offset buffers — it never touches array()/arrayOffset() and all reads are relative — so this is a guard against future edits rather than a fix. Nice to have alongside the existing position/limit assertions.

Coverage otherwise looks good. I decoded the hex by hand and the MULTIPOINT((1 2),EMPTY,EMPTY,(3 4)) case does exercise a big-endian child inside a little-endian parent, so mixed-endian nesting is covered even though the test plan only claims top-level byte orders.

[huan233usc]

Added as boundsFromBufferWithOffset: a slice() at position 11 of a larger array, asserting both the box and that the caller's position is untouched.

[huan233usc]

Thanks — replies to two of the questions that weren't inline.

Parquet already computes a bounding box. Its footer box doesn't follow the Iceberg spec. format/spec.md:772 skips NaN per coordinate, so a value with a valid ordinate in one dimension and NaN in the other still contributes the valid one. Parquet's BoundingBox.update(Geometry) takes the JTS envelope, and JTS expandToInclude has no NaN guard, so one NaN vertex makes the envelope's Y range NaN; the updateBounds guard then drops that dimension entirely:

MULTIPOINT((1 NaN),(5 2))
  Parquet footer  ->  x=[1,5]  y=[Infinity,-Infinity]   (Y lost)
  spec / this PR  ->  x=[1,5]  y=[2,2]

Iceberg can't recover the dropped dimension from the footer value afterwards. The Avro path in #17119 has no footer statistics to read at all, so a collector is needed regardless. Will coordinate with #12347.

Should invalid-but-parseable WKB fail the write? The line I'd draw is spec legality, not parseability. Z/M is legal per format/spec.md:2138 ("ISO supporting XY, XYZ, XYM, XYZM"), so it now contributes bounds rather than failing. A LINESTRING child inside a MULTIPOINT violates OGC — same class as truncation, trailing bytes and a bad byte-order flag, all already rejected here. Accepting malformed WKB silently would produce bounds that are quietly wrong and prune files that should match, which seems worse than failing the write.