szehon-ho commented on code in PR #17509: URL: https://github.com/apache/iceberg/pull/17509#discussion_r3725028430
########## api/src/main/java/org/apache/iceberg/geospatial/GeometryBoundsCollector.java: ########## @@ -0,0 +1,230 @@ +/* + * 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.geospatial; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; + +/** + * Accumulates geometry bounds from values encoded as Well-Known Binary (WKB). + * + * <p>The seven OGC geometry types are supported: point, line string, polygon, multi point, multi + * line string, multi polygon, and geometry collection. WKB values carrying Z or M dimensions are + * rejected. + * + * <p>Coordinates are tracked independently for the X and Y dimensions. {@code NaN} values do not + * contribute to a dimension, and no bounds are produced unless both dimensions are present. + */ +public final class GeometryBoundsCollector { + + private static final int TYPE_POINT = 1; + private static final int TYPE_LINE_STRING = 2; + private static final int TYPE_POLYGON = 3; + private static final int TYPE_MULTI_POINT = 4; + private static final int TYPE_MULTI_LINE_STRING = 5; + private static final int TYPE_MULTI_POLYGON = 6; + private static final int TYPE_GEOMETRY_COLLECTION = 7; + private static final int ANY_GEOMETRY = 0; + + private static final int MAX_DEPTH = 100; + + private final DimensionBounds xBounds = new DimensionBounds(); + private final DimensionBounds yBounds = new DimensionBounds(); + + // reusable copies of the accumulated bounds, used to undo a partially parsed value + private final DimensionBounds xSaved = new DimensionBounds(); + private final DimensionBounds ySaved = new DimensionBounds(); + + /** + * Adds the coordinates from one WKB geometry to these bounds. + * + * <p>The input is read through a duplicate, so its position and limit are left unchanged. + * + * @param wkb a buffer containing exactly one WKB geometry + * @throws IllegalArgumentException if the WKB is invalid or unsupported + */ + public void add(ByteBuffer wkb) { + Preconditions.checkArgument(wkb != null, "Invalid WKB buffer: null"); + xSaved.copyFrom(xBounds); + ySaved.copyFrom(yBounds); + ByteBuffer buffer = wkb.duplicate(); + try { + parseGeometry(buffer, 0, ANY_GEOMETRY); + Preconditions.checkArgument(!buffer.hasRemaining(), "Invalid WKB: trailing data"); + } catch (RuntimeException e) { + xBounds.copyFrom(xSaved); + yBounds.copyFrom(ySaved); + throw e; + } + } + + /** Returns the accumulated bounding box, or {@code null} if either X or Y has no value. */ + public BoundingBox boundingBox() { + if (!xBounds.hasValue() || !yBounds.hasValue()) { + return null; + } + + GeospatialBound min = GeospatialBound.createXY(xBounds.lower(), yBounds.lower()); + GeospatialBound max = GeospatialBound.createXY(xBounds.upper(), yBounds.upper()); + return new BoundingBox(min, max); + } + + private void parseGeometry(ByteBuffer buffer, int depth, int expectedType) { + Preconditions.checkArgument(depth <= MAX_DEPTH, "Invalid WKB: nesting too deep"); + checkRemaining(buffer, 5); + + byte order = buffer.get(); + if (order == 0) { + buffer.order(ByteOrder.BIG_ENDIAN); + } else if (order == 1) { + buffer.order(ByteOrder.LITTLE_ENDIAN); + } else { + throw new IllegalArgumentException("Invalid WKB byte order: " + order); + } + + long typeCode = buffer.getInt() & 0xFFFFFFFFL; + int geometryType = (int) (typeCode % 1000); + Preconditions.checkArgument( + typeCode / 1000 == 0, "Unsupported WKB: only 2D geometries are supported"); Review Comment: 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. ########## api/src/main/java/org/apache/iceberg/geospatial/GeometryBoundsCollector.java: ########## @@ -0,0 +1,230 @@ +/* + * 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.geospatial; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; + +/** + * Accumulates geometry bounds from values encoded as Well-Known Binary (WKB). + * + * <p>The seven OGC geometry types are supported: point, line string, polygon, multi point, multi + * line string, multi polygon, and geometry collection. WKB values carrying Z or M dimensions are + * rejected. + * + * <p>Coordinates are tracked independently for the X and Y dimensions. {@code NaN} values do not + * contribute to a dimension, and no bounds are produced unless both dimensions are present. + */ +public final class GeometryBoundsCollector { + + private static final int TYPE_POINT = 1; + private static final int TYPE_LINE_STRING = 2; + private static final int TYPE_POLYGON = 3; + private static final int TYPE_MULTI_POINT = 4; + private static final int TYPE_MULTI_LINE_STRING = 5; + private static final int TYPE_MULTI_POLYGON = 6; + private static final int TYPE_GEOMETRY_COLLECTION = 7; + private static final int ANY_GEOMETRY = 0; + + private static final int MAX_DEPTH = 100; + + private final DimensionBounds xBounds = new DimensionBounds(); + private final DimensionBounds yBounds = new DimensionBounds(); + + // reusable copies of the accumulated bounds, used to undo a partially parsed value + private final DimensionBounds xSaved = new DimensionBounds(); + private final DimensionBounds ySaved = new DimensionBounds(); Review Comment: 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. ########## api/src/main/java/org/apache/iceberg/geospatial/GeometryBoundsCollector.java: ########## @@ -0,0 +1,230 @@ +/* + * 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.geospatial; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; + +/** + * Accumulates geometry bounds from values encoded as Well-Known Binary (WKB). + * + * <p>The seven OGC geometry types are supported: point, line string, polygon, multi point, multi + * line string, multi polygon, and geometry collection. WKB values carrying Z or M dimensions are + * rejected. + * + * <p>Coordinates are tracked independently for the X and Y dimensions. {@code NaN} values do not + * contribute to a dimension, and no bounds are produced unless both dimensions are present. + */ +public final class GeometryBoundsCollector { Review Comment: 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? ########## api/src/main/java/org/apache/iceberg/geospatial/GeometryBoundsCollector.java: ########## @@ -0,0 +1,230 @@ +/* + * 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.geospatial; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; + +/** + * Accumulates geometry bounds from values encoded as Well-Known Binary (WKB). + * + * <p>The seven OGC geometry types are supported: point, line string, polygon, multi point, multi + * line string, multi polygon, and geometry collection. WKB values carrying Z or M dimensions are + * rejected. + * + * <p>Coordinates are tracked independently for the X and Y dimensions. {@code NaN} values do not + * contribute to a dimension, and no bounds are produced unless both dimensions are present. + */ +public final class GeometryBoundsCollector { + + private static final int TYPE_POINT = 1; + private static final int TYPE_LINE_STRING = 2; + private static final int TYPE_POLYGON = 3; + private static final int TYPE_MULTI_POINT = 4; + private static final int TYPE_MULTI_LINE_STRING = 5; + private static final int TYPE_MULTI_POLYGON = 6; + private static final int TYPE_GEOMETRY_COLLECTION = 7; + private static final int ANY_GEOMETRY = 0; + + private static final int MAX_DEPTH = 100; + + private final DimensionBounds xBounds = new DimensionBounds(); + private final DimensionBounds yBounds = new DimensionBounds(); + + // reusable copies of the accumulated bounds, used to undo a partially parsed value + private final DimensionBounds xSaved = new DimensionBounds(); + private final DimensionBounds ySaved = new DimensionBounds(); + + /** + * Adds the coordinates from one WKB geometry to these bounds. + * + * <p>The input is read through a duplicate, so its position and limit are left unchanged. + * + * @param wkb a buffer containing exactly one WKB geometry + * @throws IllegalArgumentException if the WKB is invalid or unsupported + */ + public void add(ByteBuffer wkb) { + Preconditions.checkArgument(wkb != null, "Invalid WKB buffer: null"); + xSaved.copyFrom(xBounds); + ySaved.copyFrom(yBounds); + ByteBuffer buffer = wkb.duplicate(); + try { + parseGeometry(buffer, 0, ANY_GEOMETRY); + Preconditions.checkArgument(!buffer.hasRemaining(), "Invalid WKB: trailing data"); + } catch (RuntimeException e) { + xBounds.copyFrom(xSaved); + yBounds.copyFrom(ySaved); + throw e; + } + } + + /** Returns the accumulated bounding box, or {@code null} if either X or Y has no value. */ + public BoundingBox boundingBox() { + if (!xBounds.hasValue() || !yBounds.hasValue()) { + return null; + } + + GeospatialBound min = GeospatialBound.createXY(xBounds.lower(), yBounds.lower()); + GeospatialBound max = GeospatialBound.createXY(xBounds.upper(), yBounds.upper()); + return new BoundingBox(min, max); + } + + private void parseGeometry(ByteBuffer buffer, int depth, int expectedType) { + Preconditions.checkArgument(depth <= MAX_DEPTH, "Invalid WKB: nesting too deep"); + checkRemaining(buffer, 5); + + byte order = buffer.get(); + if (order == 0) { + buffer.order(ByteOrder.BIG_ENDIAN); + } else if (order == 1) { + buffer.order(ByteOrder.LITTLE_ENDIAN); + } else { + throw new IllegalArgumentException("Invalid WKB byte order: " + order); + } + + long typeCode = buffer.getInt() & 0xFFFFFFFFL; + int geometryType = (int) (typeCode % 1000); + Preconditions.checkArgument( + typeCode / 1000 == 0, "Unsupported WKB: only 2D geometries are supported"); + Preconditions.checkArgument( + expectedType == ANY_GEOMETRY || geometryType == expectedType, + "Invalid WKB: expected geometry type %s but found %s", + expectedType, + geometryType); + + switch (geometryType) { + case TYPE_POINT: + readCoordinate(buffer); + break; + case TYPE_LINE_STRING: + readCoordinateSequence(buffer, true); + break; + case TYPE_POLYGON: + readPolygon(buffer); + break; + case TYPE_MULTI_POINT: + readCollection(buffer, depth, TYPE_POINT); + break; + case TYPE_MULTI_LINE_STRING: + readCollection(buffer, depth, TYPE_LINE_STRING); + break; + case TYPE_MULTI_POLYGON: + readCollection(buffer, depth, TYPE_POLYGON); + break; + case TYPE_GEOMETRY_COLLECTION: + readCollection(buffer, depth, ANY_GEOMETRY); + break; + default: + throw new IllegalArgumentException("Invalid or unsupported WKB geometry type: " + typeCode); + } + } + + private void readPolygon(ByteBuffer buffer) { + int numRings = readCount(buffer); + if (numRings > 0) { + readCoordinateSequence(buffer, true); + } + + // interior rings are contained by the exterior ring and cannot widen the bounds + for (int i = 1; i < numRings; i += 1) { + readCoordinateSequence(buffer, false); Review Comment: 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. ########## api/src/test/java/org/apache/iceberg/geospatial/TestGeometryBoundsCollector.java: ########## @@ -0,0 +1,196 @@ +/* + * 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.geospatial; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +public class TestGeometryBoundsCollector { Review Comment: 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. ########## api/src/test/java/org/apache/iceberg/geospatial/TestGeometryBoundsCollector.java: ########## @@ -0,0 +1,196 @@ +/* + * 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.geospatial; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +public class TestGeometryBoundsCollector { + + @ParameterizedTest(name = "{0}") + @MethodSource("boundingBoxCases") + public void testBoundingBox(String wkt, String hexWkb, BoundingBox expected) { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + ByteBuffer wkb = decode(hexWkb); + int position = wkb.position(); + int limit = wkb.limit(); + + bounds.add(wkb); + + assertThat(wkb.position()).as(wkt).isEqualTo(position); + assertThat(wkb.limit()).as(wkt).isEqualTo(limit); + assertThat(bounds.boundingBox()).as(wkt).isEqualTo(expected); + } + + @Test + public void testNoBoundsWhenOneDimensionIsMissing() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + bounds.add(decode("0101000000000000000000f03f000000000000f87f")); + + assertThat(bounds.boundingBox()).as("POINT(1 NaN)").isNull(); + } + + @Test + public void testBoundsAcrossValuesWithMissingCoordinates() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + bounds.add(decode("0101000000000000000000f03f000000000000f87f")); + bounds.add(decode("0101000000000000000000f87f0000000000000040")); + + assertThat(bounds.boundingBox()).isEqualTo(box(1, 2, 1, 2)); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("invalidWkbCases") + public void testInvalidWkb(String description, String hexWkb, String expectedMessage) { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + bounds.add(decode("0101000000000000000000f03f0000000000000040")); + + assertThatThrownBy(() -> bounds.add(decode(hexWkb))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining(expectedMessage); + assertThat(bounds.boundingBox()).isEqualTo(box(1, 2, 1, 2)); + } + + @Test + public void testDeeplyNestedWkbIsRejected() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + + assertThatThrownBy(() -> bounds.add(nestedCollections(200))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("nesting too deep"); + } + + /** Returns WKB for a chain of geometry collections, each holding the next, around POINT(1 2). */ + private static ByteBuffer nestedCollections(int depth) { + ByteBuffer buffer = ByteBuffer.allocate(depth * 9 + 21).order(ByteOrder.LITTLE_ENDIAN); + for (int i = 0; i < depth; i += 1) { + buffer.put((byte) 1); + buffer.putInt(7); + buffer.putInt(1); + } + + buffer.put((byte) 1); + buffer.putInt(1); + buffer.putDouble(1.0); + buffer.putDouble(2.0); + buffer.flip(); + return buffer; + } + + private static Stream<Arguments> boundingBoxCases() { + return Stream.of( + Arguments.of("POINT EMPTY", "0101000000000000000000f87f000000000000f87f", null), + Arguments.of("POINT(1 2)", "0101000000000000000000f03f0000000000000040", box(1, 2, 1, 2)), + Arguments.of( + "POINT(1 2) big endian", "00000000013ff00000000000004000000000000000", box(1, 2, 1, 2)), + Arguments.of( + "LINESTRING(0 1,1 0,2 -1,-1 -2,0 1)", + "0102000000050000000000000000000000000000000000f03f000000000000f03f" + + "00000000000000000000000000000040000000000000f0bf000000000000f0bf" + + "00000000000000c00000000000000000000000000000f03f", + box(-1, -2, 2, 1)), + Arguments.of( + "POLYGON((0 0,10 0,0 10,0 0),(1 1,1 2,2 1,1 1))", + "010300000002000000040000000000000000000000000000000000000000000000" + + "000024400000000000000000000000000000000000000000000024400000000000" + + "000000000000000000000004000000000000000000f03f000000000000f03f0000" + + "00000000f03f00000000000000400000000000000040000000000000f03f000000" + + "000000f03f000000000000f03f", + box(0, 0, 10, 10)), + Arguments.of( + "MULTIPOINT((1 2),EMPTY,EMPTY,(3 4))", + "0104000000040000000101000000000000000000f03f000000000000004001010000" + + "00000000000000f87f000000000000f87f0101000000000000000000f87f00000000" + + "0000f87f000000000140080000000000004010000000000000", + box(1, 2, 3, 4)), + Arguments.of( + "MULTILINESTRING((1 2,3 4),(5 6,7 8))", + "010500000002000000010200000002000000000000000000f03f0000000000000040" + + "0000000000000840000000000000104001020000000200000000000000000014400000" + + "0000000018400000000000001c400000000000002040", + box(1, 2, 7, 8)), + Arguments.of( + "MULTIPOLYGON(EMPTY,((0 0,10 0,0 10,0 0),(1 1,1 2,2 1,1 1)))", + "01060000000200000001030000000000000001030000000200000004000000000000000000000000000" + + "00000000000000000000000244000000000000000000000000000000000000000000000244000000000" + + "00000000000000000000000004000000000000000000f03f000000000000f03f000000000000f03f000" + + "00000000000400000000000000040000000000000f03f000000000000f03f000000000000f03f", + box(0, 0, 10, 10)), + Arguments.of( + "GEOMETRYCOLLECTION(POINT(1 2),LINESTRING EMPTY,POLYGON EMPTY," + + "MULTIPOINT EMPTY,MULTILINESTRING EMPTY,MULTIPOLYGON EMPTY," + + "GEOMETRYCOLLECTION(POINT EMPTY,LINESTRING EMPTY,POLYGON EMPTY," + + "MULTIPOINT EMPTY,MULTILINESTRING EMPTY,MULTIPOLYGON EMPTY))", + "0107000000070000000101000000000000000000f03f000000000000004001020000" + + "00000000000103000000000000000104000000000000000105000000000000000106" + + "000000000000000107000000060000000101000000000000000000f87f0000000000" + + "00f87f01020000000000000001030000000000000001040000000000000001050000" + + "0000000000010600000000000000", + box(1, 2, 1, 2))); + } + + private static Stream<Arguments> invalidWkbCases() { + return Stream.of( + Arguments.of( + "trailing data", "01010000000000000000000840000000000000104000", "trailing data"), + Arguments.of( + "invalid multi-point child", + "010400000001000000010200000000000000", + "expected geometry type"), + Arguments.of( + "unsupported Z geometry", + "01e9030000000000000000f03f00000000000000400000000000000840", + "only 2D geometries are supported"), + Arguments.of( + "unsupported M geometry", + "01d1070000000000000000f03f00000000000000400000000000000840", + "only 2D geometries are supported"), + Arguments.of( + "unsupported ZM geometry", + "01b90b0000000000000000f03f000000000000004000000000000008400000000000001040", + "only 2D geometries are supported"), + Arguments.of("truncated point", "0101000000", "unexpected end of buffer")); + } + + private static GeospatialBound xy(double xCoord, double yCoord) { + return GeospatialBound.createXY(xCoord, yCoord); + } + + private static BoundingBox box(double minX, double minY, double maxX, double maxY) { + return new BoundingBox(xy(minX, minY), xy(maxX, maxY)); + } + + private static ByteBuffer decode(String hex) { + byte[] bytes = new byte[hex.length() / 2]; + for (int i = 0; i < bytes.length; i += 1) { + int offset = i * 2; + bytes[i] = (byte) Integer.parseInt(hex.substring(offset, offset + 2), 16); + } + + return ByteBuffer.wrap(bytes); + } Review Comment: 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. ########## api/src/test/java/org/apache/iceberg/geospatial/TestGeometryBoundsCollector.java: ########## @@ -0,0 +1,196 @@ +/* + * 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.geospatial; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +public class TestGeometryBoundsCollector { + + @ParameterizedTest(name = "{0}") + @MethodSource("boundingBoxCases") + public void testBoundingBox(String wkt, String hexWkb, BoundingBox expected) { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + ByteBuffer wkb = decode(hexWkb); + int position = wkb.position(); + int limit = wkb.limit(); + + bounds.add(wkb); + + assertThat(wkb.position()).as(wkt).isEqualTo(position); + assertThat(wkb.limit()).as(wkt).isEqualTo(limit); + assertThat(bounds.boundingBox()).as(wkt).isEqualTo(expected); + } + + @Test + public void testNoBoundsWhenOneDimensionIsMissing() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + bounds.add(decode("0101000000000000000000f03f000000000000f87f")); + + assertThat(bounds.boundingBox()).as("POINT(1 NaN)").isNull(); + } + + @Test + public void testBoundsAcrossValuesWithMissingCoordinates() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + bounds.add(decode("0101000000000000000000f03f000000000000f87f")); + bounds.add(decode("0101000000000000000000f87f0000000000000040")); + + assertThat(bounds.boundingBox()).isEqualTo(box(1, 2, 1, 2)); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("invalidWkbCases") + public void testInvalidWkb(String description, String hexWkb, String expectedMessage) { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + bounds.add(decode("0101000000000000000000f03f0000000000000040")); + + assertThatThrownBy(() -> bounds.add(decode(hexWkb))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining(expectedMessage); + assertThat(bounds.boundingBox()).isEqualTo(box(1, 2, 1, 2)); + } + + @Test + public void testDeeplyNestedWkbIsRejected() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + + assertThatThrownBy(() -> bounds.add(nestedCollections(200))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("nesting too deep"); + } + + /** Returns WKB for a chain of geometry collections, each holding the next, around POINT(1 2). */ + private static ByteBuffer nestedCollections(int depth) { + ByteBuffer buffer = ByteBuffer.allocate(depth * 9 + 21).order(ByteOrder.LITTLE_ENDIAN); + for (int i = 0; i < depth; i += 1) { + buffer.put((byte) 1); + buffer.putInt(7); + buffer.putInt(1); + } + + buffer.put((byte) 1); + buffer.putInt(1); + buffer.putDouble(1.0); + buffer.putDouble(2.0); + buffer.flip(); + return buffer; + } + + private static Stream<Arguments> boundingBoxCases() { + return Stream.of( + Arguments.of("POINT EMPTY", "0101000000000000000000f87f000000000000f87f", null), + Arguments.of("POINT(1 2)", "0101000000000000000000f03f0000000000000040", box(1, 2, 1, 2)), + Arguments.of( + "POINT(1 2) big endian", "00000000013ff00000000000004000000000000000", box(1, 2, 1, 2)), + Arguments.of( + "LINESTRING(0 1,1 0,2 -1,-1 -2,0 1)", + "0102000000050000000000000000000000000000000000f03f000000000000f03f" + + "00000000000000000000000000000040000000000000f0bf000000000000f0bf" + + "00000000000000c00000000000000000000000000000f03f", + box(-1, -2, 2, 1)), + Arguments.of( + "POLYGON((0 0,10 0,0 10,0 0),(1 1,1 2,2 1,1 1))", + "010300000002000000040000000000000000000000000000000000000000000000" + + "000024400000000000000000000000000000000000000000000024400000000000" + + "000000000000000000000004000000000000000000f03f000000000000f03f0000" + + "00000000f03f00000000000000400000000000000040000000000000f03f000000" + + "000000f03f000000000000f03f", + box(0, 0, 10, 10)), + Arguments.of( + "MULTIPOINT((1 2),EMPTY,EMPTY,(3 4))", + "0104000000040000000101000000000000000000f03f000000000000004001010000" + + "00000000000000f87f000000000000f87f0101000000000000000000f87f00000000" + + "0000f87f000000000140080000000000004010000000000000", + box(1, 2, 3, 4)), + Arguments.of( + "MULTILINESTRING((1 2,3 4),(5 6,7 8))", + "010500000002000000010200000002000000000000000000f03f0000000000000040" + + "0000000000000840000000000000104001020000000200000000000000000014400000" + + "0000000018400000000000001c400000000000002040", + box(1, 2, 7, 8)), + Arguments.of( + "MULTIPOLYGON(EMPTY,((0 0,10 0,0 10,0 0),(1 1,1 2,2 1,1 1)))", + "01060000000200000001030000000000000001030000000200000004000000000000000000000000000" + + "00000000000000000000000244000000000000000000000000000000000000000000000244000000000" + + "00000000000000000000000004000000000000000000f03f000000000000f03f000000000000f03f000" + + "00000000000400000000000000040000000000000f03f000000000000f03f000000000000f03f", + box(0, 0, 10, 10)), + Arguments.of( + "GEOMETRYCOLLECTION(POINT(1 2),LINESTRING EMPTY,POLYGON EMPTY," + + "MULTIPOINT EMPTY,MULTILINESTRING EMPTY,MULTIPOLYGON EMPTY," + + "GEOMETRYCOLLECTION(POINT EMPTY,LINESTRING EMPTY,POLYGON EMPTY," + + "MULTIPOINT EMPTY,MULTILINESTRING EMPTY,MULTIPOLYGON EMPTY))", + "0107000000070000000101000000000000000000f03f000000000000004001020000" + + "00000000000103000000000000000104000000000000000105000000000000000106" + + "000000000000000107000000060000000101000000000000000000f87f0000000000" + + "00f87f01020000000000000001030000000000000001040000000000000001050000" + + "0000000000010600000000000000", + box(1, 2, 1, 2))); + } + + private static Stream<Arguments> invalidWkbCases() { + return Stream.of( + Arguments.of( + "trailing data", "01010000000000000000000840000000000000104000", "trailing data"), + Arguments.of( + "invalid multi-point child", + "010400000001000000010200000000000000", + "expected geometry type"), + Arguments.of( + "unsupported Z geometry", + "01e9030000000000000000f03f00000000000000400000000000000840", + "only 2D geometries are supported"), + Arguments.of( + "unsupported M geometry", + "01d1070000000000000000f03f00000000000000400000000000000840", + "only 2D geometries are supported"), + Arguments.of( + "unsupported ZM geometry", + "01b90b0000000000000000f03f000000000000004000000000000008400000000000001040", + "only 2D geometries are supported"), Review Comment: 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. -- 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]
