szehon-ho commented on code in PR #17509: URL: https://github.com/apache/iceberg/pull/17509#discussion_r3817870541
########## core/src/main/java/org/apache/iceberg/GeometryBoundsBuilder.java: ########## @@ -0,0 +1,396 @@ +/* + * 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 java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Arrays; +import org.apache.iceberg.geospatial.BoundingBox; +import org.apache.iceberg.geospatial.GeospatialBound; +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. + * + * <p>Coordinates are tracked independently for the X and Y dimensions. A {@code NaN} ordinate marks + * an empty value and does not contribute to its dimension; an infinite ordinate is a real position + * and is kept as a bound, since the spec forbids only NaN as a lower or upper bound. No bounds are + * produced unless both dimensions are present. + * + * <p>These bounds apply to {@code geometry} columns, whose edges are always interpolated linearly, + * so a box that contains every vertex contains the whole geometry. They are not valid for {@code + * geography} columns: geodesic edges can reach beyond their endpoints, longitude is periodic, and a + * geography box may cross the antimeridian. + * + * <p>Only the X and Y dimensions contribute to the box. Z and M ordinates are valid in the ISO WKB + * serializations that Iceberg accepts, so they are read past and ignored rather than rejected. + * + * <p>Every ring of a polygon contributes, so the box covers the whole polygon even when an interior + * ring extends past the shell. This matches the envelope a library such as JTS derives from all of + * a geometry's coordinates. + */ +class GeometryBoundsBuilder { + + 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; + + // ISO WKB encodes the dimensions of a geometry in the thousands digit of its type code + private static final int DIMENSION_DIVISOR = 1000; + private static final int XY_GROUP = 0; + private static final int XYZ_GROUP = 1; + private static final int XYM_GROUP = 2; + private static final int XYZM_GROUP = 3; + private static final int ANY_DIMENSION = -1; + + private static final int MIN_RING_POINTS = 4; + + private final DimensionBounds xBounds = new DimensionBounds(); + private final DimensionBounds yBounds = new DimensionBounds(); + + /** + * Adds one WKB geometry value to these bounds. + * + * <p>The input is read through a duplicate, so its position and limit are left unchanged. + * + * <p>If this throws, the builder's state is undefined: coordinates parsed before the failure may + * already be folded in, so a caller that continues after a rejected value must discard this + * builder. + * + * @param wkb a buffer containing exactly one WKB geometry + * @throws IllegalArgumentException if the WKB is malformed + */ + public void addValue(ByteBuffer wkb) { + Preconditions.checkArgument(wkb != null, "Invalid WKB buffer: null"); + ByteBuffer buffer = wkb.duplicate(); + parseGeometry(buffer, ANY_GEOMETRY, ANY_DIMENSION); Review Comment: Suggest checking that the buffer is fully consumed and suppressing the box for the file when it isn't, rather than ignoring what is left. A `MULTIPOINT` declaring 1 element followed by two point bodies parses the first child and returns, so the second point never reaches the bounds and the box no longer contains every object in the file — pruning it from a query it should match, with no error. Leftover bytes are the only detectable sign of that, since the parsed prefix is itself valid WKB. Dropping the box is safe where throwing is not, because metrics are optional. The javadoc on line 84 still promises exactly one geometry. ########## core/src/main/java/org/apache/iceberg/GeometryBoundsBuilder.java: ########## @@ -0,0 +1,396 @@ +/* + * 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 java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Arrays; +import org.apache.iceberg.geospatial.BoundingBox; +import org.apache.iceberg.geospatial.GeospatialBound; +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. + * + * <p>Coordinates are tracked independently for the X and Y dimensions. A {@code NaN} ordinate marks + * an empty value and does not contribute to its dimension; an infinite ordinate is a real position + * and is kept as a bound, since the spec forbids only NaN as a lower or upper bound. No bounds are + * produced unless both dimensions are present. + * + * <p>These bounds apply to {@code geometry} columns, whose edges are always interpolated linearly, + * so a box that contains every vertex contains the whole geometry. They are not valid for {@code + * geography} columns: geodesic edges can reach beyond their endpoints, longitude is periodic, and a + * geography box may cross the antimeridian. + * + * <p>Only the X and Y dimensions contribute to the box. Z and M ordinates are valid in the ISO WKB + * serializations that Iceberg accepts, so they are read past and ignored rather than rejected. + * + * <p>Every ring of a polygon contributes, so the box covers the whole polygon even when an interior + * ring extends past the shell. This matches the envelope a library such as JTS derives from all of + * a geometry's coordinates. + */ +class GeometryBoundsBuilder { + + 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; + + // ISO WKB encodes the dimensions of a geometry in the thousands digit of its type code + private static final int DIMENSION_DIVISOR = 1000; + private static final int XY_GROUP = 0; + private static final int XYZ_GROUP = 1; + private static final int XYM_GROUP = 2; + private static final int XYZM_GROUP = 3; + private static final int ANY_DIMENSION = -1; + + private static final int MIN_RING_POINTS = 4; + + private final DimensionBounds xBounds = new DimensionBounds(); + private final DimensionBounds yBounds = new DimensionBounds(); + + /** + * Adds one WKB geometry value to these bounds. + * + * <p>The input is read through a duplicate, so its position and limit are left unchanged. + * + * <p>If this throws, the builder's state is undefined: coordinates parsed before the failure may + * already be folded in, so a caller that continues after a rejected value must discard this + * builder. + * + * @param wkb a buffer containing exactly one WKB geometry + * @throws IllegalArgumentException if the WKB is malformed + */ + public void addValue(ByteBuffer wkb) { + Preconditions.checkArgument(wkb != null, "Invalid WKB buffer: null"); + ByteBuffer buffer = wkb.duplicate(); + parseGeometry(buffer, ANY_GEOMETRY, ANY_DIMENSION); + } + + /** + * Builds the bounding box covering every geometry added, or {@code null} if either the X or Y + * dimension has no value. + */ + public BoundingBox build() { + 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 expectedType, int expectedDimension) { + // a geometry header is a one-byte order flag followed by a four-byte type code: + // +-------+-----------------------+ + // | order | type code | + // | (1 B) | (4 B) | + // +-------+-----------------------+ + checkRemaining(buffer, Byte.BYTES + Integer.BYTES); + + // each geometry sets its own byte order; restore the caller's order before returning so a + // sibling read after a nested geometry is not misread with the wrong endianness + ByteOrder callerOrder = buffer.order(); + 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); + } + + parseGeometryBodyAndUpdateBound(buffer, expectedType, expectedDimension); + buffer.order(callerOrder); + } + + private void parseGeometryBodyAndUpdateBound( + ByteBuffer buffer, int expectedType, int expectedDimension) { + long typeCode = Integer.toUnsignedLong(buffer.getInt()); + int dimensionGroup = (int) (typeCode / DIMENSION_DIVISOR); + int geometryType = (int) (typeCode % DIMENSION_DIVISOR); + Preconditions.checkArgument( + geometryType >= TYPE_POINT + && geometryType <= TYPE_GEOMETRY_COLLECTION + && dimensionGroup <= XYZM_GROUP, + "Invalid or unsupported WKB geometry type: %s", + typeCode); + // an element of a multi geometry or collection must match its parent's member type and + // dimensions; if/throw so the message is built only when a value is actually rejected + if (expectedType != ANY_GEOMETRY && geometryType != expectedType) { + throw new IllegalArgumentException( + "Invalid WKB: expected geometry type " + + typeName(expectedType) + + " but found " + + typeName(geometryType)); + } + if (expectedDimension != ANY_DIMENSION && dimensionGroup != expectedDimension) { + throw new IllegalArgumentException( + "Invalid WKB: expected dimensions " + + dimensionName(expectedDimension) + + " but found " + + dimensionName(dimensionGroup)); + } + + int numDimensions = numDimensions(dimensionGroup); + + switch (geometryType) { + case TYPE_POINT: + readCoordinate(buffer, numDimensions); + break; + case TYPE_LINE_STRING: + readCoordinateSequence(buffer, numDimensions); + break; + case TYPE_POLYGON: + readPolygon(buffer, numDimensions); + break; + case TYPE_MULTI_POINT: + readCollection(buffer, TYPE_POINT, dimensionGroup); + break; + case TYPE_MULTI_LINE_STRING: + readCollection(buffer, TYPE_LINE_STRING, dimensionGroup); + break; + case TYPE_MULTI_POLYGON: + readCollection(buffer, TYPE_POLYGON, dimensionGroup); + break; + case TYPE_GEOMETRY_COLLECTION: + readCollection(buffer, ANY_GEOMETRY, dimensionGroup); + break; + default: + throw new IllegalArgumentException("Invalid or unsupported WKB geometry type: " + typeCode); + } + } + + private static int numDimensions(int dimensionGroup) { + switch (dimensionGroup) { + case XY_GROUP: + return 2; + case XYZ_GROUP: + case XYM_GROUP: + return 3; + default: // XYZM_GROUP, the only remaining group the caller accepts + return 4; + } + } + + private static String typeName(int geometryType) { + switch (geometryType) { + case TYPE_POINT: + return "Point"; + case TYPE_LINE_STRING: + return "LineString"; + case TYPE_POLYGON: + return "Polygon"; + case TYPE_MULTI_POINT: + return "MultiPoint"; + case TYPE_MULTI_LINE_STRING: + return "MultiLineString"; + case TYPE_MULTI_POLYGON: + return "MultiPolygon"; + case TYPE_GEOMETRY_COLLECTION: + return "GeometryCollection"; + default: + return String.valueOf(geometryType); + } + } + + private static String dimensionName(int dimensionGroup) { + switch (dimensionGroup) { + case XY_GROUP: + return "XY"; + case XYZ_GROUP: + return "XYZ"; + case XYM_GROUP: + return "XYM"; + default: + return "XYZM"; + } + } + + // a ring count, then that many rings, each a coordinate sequence: + // +----------+-----------------------+ + // | # rings | ring 0, ring 1, ... | + // | (4 B) | (each a point seq) | + // +----------+-----------------------+ + private void readPolygon(ByteBuffer buffer, int numDimensions) { + // every ring contributes, including interior rings: a hole extending past the shell would + // otherwise under-cover the polygon, pruning a file from a query it should match + int numRings = readCount(buffer); + for (int i = 0; i < numRings; i += 1) { + readRing(buffer, numDimensions); + } + } + + // a point count, then that many coordinates forming a linear ring; a non-empty ring must be + // closed (its first and last points are equal) and hold at least four points: + // +----------+-----------------------+ + // | # points | coord 0 .. coord n-1 | + // | (4 B) | (n coordinates) | + // +----------+-----------------------+ + private void readRing(ByteBuffer buffer, int numDimensions) { + int numPoints = readCount(buffer); + checkRemaining(buffer, (long) numPoints * numDimensions * Double.BYTES); + if (numPoints == 0) { + return; + } + + Preconditions.checkArgument( + numPoints >= MIN_RING_POINTS, + "Invalid WKB: polygon ring has fewer than %s points: %s", + MIN_RING_POINTS, + numPoints); + + // capture the first and last full coordinates to verify closure; middle points only bound + double[] first = new double[numDimensions]; + double[] last = new double[numDimensions]; + readCoordinate(buffer, first); + for (int i = 1; i < numPoints - 1; i += 1) { + readCoordinate(buffer, numDimensions); + } + readCoordinate(buffer, last); + + Preconditions.checkArgument( + Arrays.equals(first, last), "Invalid WKB: polygon ring is not closed"); Review Comment: Suggest comparing only the first and last X and Y, with `==`. `Arrays.equals` uses `Double.equals` semantics, so `0.0` and `-0.0` count as unequal, and `first`/`last` are `numDimensions` wide, so Z and M have to match too. JTS closes a ring on `Coordinate.equals2D`: XY only, and `-0.0 == 0.0`. So a measured ring whose closing vertex carries the perimeter as M, `POLYGON M ((0 0 0, 1 0 1, 1 1 2, 0 0 3))`, is closed there and rejected here — and the throw aborts the write. Comparing two X/Y pairs also drops the two arrays allocated per ring. ########## core/src/main/java/org/apache/iceberg/GeometryBoundsBuilder.java: ########## @@ -0,0 +1,396 @@ +/* + * 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 java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Arrays; +import org.apache.iceberg.geospatial.BoundingBox; +import org.apache.iceberg.geospatial.GeospatialBound; +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. + * + * <p>Coordinates are tracked independently for the X and Y dimensions. A {@code NaN} ordinate marks + * an empty value and does not contribute to its dimension; an infinite ordinate is a real position + * and is kept as a bound, since the spec forbids only NaN as a lower or upper bound. No bounds are + * produced unless both dimensions are present. + * + * <p>These bounds apply to {@code geometry} columns, whose edges are always interpolated linearly, + * so a box that contains every vertex contains the whole geometry. They are not valid for {@code + * geography} columns: geodesic edges can reach beyond their endpoints, longitude is periodic, and a + * geography box may cross the antimeridian. + * + * <p>Only the X and Y dimensions contribute to the box. Z and M ordinates are valid in the ISO WKB + * serializations that Iceberg accepts, so they are read past and ignored rather than rejected. + * + * <p>Every ring of a polygon contributes, so the box covers the whole polygon even when an interior + * ring extends past the shell. This matches the envelope a library such as JTS derives from all of + * a geometry's coordinates. Review Comment: Suggest either restoring a polygon case that pins this or trimming the claim. A hole in a valid polygon never widens the shell's box, so nothing fails if bounds come from the shell alone — the other rings still have to be consumed either way. The Test Plan also still lists the interior-ring case that 8f344ca removed. ########## core/src/test/java/org/apache/iceberg/TestGeometryBoundsBuilder.java: ########## @@ -0,0 +1,520 @@ +/* + * 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.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Arrays; +import java.util.stream.Stream; +import org.apache.iceberg.geospatial.BoundingBox; +import org.apache.iceberg.geospatial.GeospatialBound; +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; + +class TestGeometryBoundsBuilder { + + @ParameterizedTest(name = "{0}") + @MethodSource("boundingBoxCases") + void boundingBox(String wkt, Geom geom, BoundingBox expected) { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + ByteBuffer wkb = ByteBuffer.wrap(wkb(geom)); + int position = wkb.position(); + int limit = wkb.limit(); + + bounds.addValue(wkb); + + assertThat(wkb.position()).as(wkt).isEqualTo(position); + assertThat(wkb.limit()).as(wkt).isEqualTo(limit); + assertThat(bounds.build()).as(wkt).isEqualTo(expected); + } + + @Test + void boundsFromBufferWithOffset() { + byte[] padded = new byte[64]; + byte[] wkb = wkb(point(1, 2)); + System.arraycopy(wkb, 0, padded, 11, wkb.length); + ByteBuffer slice = ByteBuffer.wrap(padded, 11, wkb.length).slice(); + + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + bounds.addValue(slice); + + assertThat(slice.position()).isEqualTo(0); + assertThat(bounds.build()).isEqualTo(box(1, 2, 1, 2)); + } + + @Test + void noBoundsWhenOneDimensionIsMissing() { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + bounds.addValue(ByteBuffer.wrap(wkb(point(1, Double.NaN)))); + + assertThat(bounds.build()).as("POINT(1 NaN)").isNull(); + } + + @Test + void boundsAcrossValuesWithMissingCoordinates() { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + bounds.addValue(ByteBuffer.wrap(wkb(point(1, Double.NaN)))); + bounds.addValue(ByteBuffer.wrap(wkb(point(Double.NaN, 2)))); + + assertThat(bounds.build()).isEqualTo(box(1, 2, 1, 2)); + } + + @Test + void infiniteOrdinateIsKeptAsBound() { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + // the spec forbids only NaN as a bound, so an infinite ordinate is kept as a real position + bounds.addValue(ByteBuffer.wrap(wkb(point(Double.POSITIVE_INFINITY, 2)))); + + assertThat(bounds.build()) + .as("POINT(Infinity 2)") + .isEqualTo(box(Double.POSITIVE_INFINITY, 2, Double.POSITIVE_INFINITY, 2)); + } + + @Test + void infiniteOrdinateWidensBounds() { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + bounds.addValue(ByteBuffer.wrap(wkb(point(1, 2)))); + // an infinite coordinate is a real position, so it widens the box toward that infinity + bounds.addValue( + ByteBuffer.wrap(wkb(point(Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY)))); + + assertThat(bounds.build()) + .isEqualTo(box(1, Double.NEGATIVE_INFINITY, Double.POSITIVE_INFINITY, 2)); + } + + @Test + void nanIsStillSkippedWhileInfiniteIsKept() { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + // NaN X is skipped (empty ordinate) while infinite Y is kept, so only Y produces a bound; + // with X missing, no box is produced + bounds.addValue(ByteBuffer.wrap(wkb(point(Double.NaN, Double.POSITIVE_INFINITY)))); + + assertThat(bounds.build()).as("POINT(NaN Infinity)").isNull(); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("extraDimensionCases") + void extraDimensionsAreIgnored(String description, Geom geom, BoundingBox expected) { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + + bounds.addValue(ByteBuffer.wrap(wkb(geom))); + + assertThat(bounds.build()).as(description).isEqualTo(expected); + } + + @Test + void boundsAcrossValuesWithDifferentDimensions() { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + bounds.addValue(ByteBuffer.wrap(wkb(pointZ(1, 2, 3)))); + bounds.addValue(ByteBuffer.wrap(wkb(pointZM(1, 2, 3, 4)))); + + assertThat(bounds.build()).isEqualTo(box(1, 2, 1, 2)); + } + + @Test + void extraDimensionsNestedInCollectionAreIgnored() { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + // a 3D collection holding 3D children (dimensions must match the parent): each child's Z is + // read past, so only XY bounds the box + bounds.addValue(ByteBuffer.wrap(wkb(collectionZ(pointZ(1, 2, 9), pointZ(3, 4, 9))))); + + assertThat(bounds.build()).isEqualTo(box(1, 2, 3, 4)); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("invalidWkbCases") + void invalidWkb(String description, byte[] wkb, String expectedMessage) { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + + assertThatThrownBy(() -> bounds.addValue(ByteBuffer.wrap(wkb))) + .as(description) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining(expectedMessage); + } + + @Test + void bigEndianParentWithLittleEndianChild() { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + // a big-endian multi point holding a little-endian point, the reverse of the MULTIPOINT case + bounds.addValue(ByteBuffer.wrap(wkb(multiPointBigEndian(point(1, 2))))); + + assertThat(bounds.build()).isEqualTo(box(1, 2, 1, 2)); + } + + @Test + void readsFromADirectBuffer() { + byte[] wkb = wkb(point(1, 2)); + ByteBuffer direct = ByteBuffer.allocateDirect(wkb.length); + direct.put(wkb).flip(); + + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + bounds.addValue(direct); + + assertThat(direct.hasArray()).isFalse(); + assertThat(bounds.build()).isEqualTo(box(1, 2, 1, 2)); + } + + @Test + void stateIsUndefinedAfterAddValueThrows() { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + bounds.addValue(ByteBuffer.wrap(wkb(point(1, 2)))); + + // adding a malformed value throws; per the addValue contract the builder must then be + // discarded, + // so this only documents that a caller cannot keep using it, not a guaranteed rolled-back state + assertThatThrownBy(() -> bounds.addValue(ByteBuffer.wrap(truncate(wkb(point(5, 6)), 5)))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("unexpected end of buffer"); + } + + private static Stream<Arguments> boundingBoxCases() { + return Stream.of( + Arguments.of("POINT EMPTY", emptyPoint(), null), + Arguments.of("POINT(1 2)", point(1, 2), box(1, 2, 1, 2)), + Arguments.of("POINT(1 2) big endian", pointBigEndian(1, 2), box(1, 2, 1, 2)), + Arguments.of( + "LINESTRING(0 1,1 0,2 -1,-1 -2,0 1)", + lineString(0, 1, 1, 0, 2, -1, -1, -2, 0, 1), + box(-1, -2, 2, 1)), + Arguments.of( + "POLYGON((0 0,10 0,0 10,0 0),(1 1,1 2,2 1,1 1))", + polygon(ring(0, 0, 10, 0, 0, 10, 0, 0), ring(1, 1, 1, 2, 2, 1, 1, 1)), + box(0, 0, 10, 10)), + Arguments.of( + "MULTIPOINT((1 2),EMPTY,EMPTY,(3 4))", + // the last child is big-endian, so this also covers a mixed-endian child + multiPoint(point(1, 2), emptyPoint(), emptyPoint(), pointBigEndian(3, 4)), + box(1, 2, 3, 4)), + Arguments.of( + "MULTILINESTRING((1 2,3 4),(5 6,7 8))", + multiLineString(lineString(1, 2, 3, 4), lineString(5, 6, 7, 8)), + 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)))", + multiPolygon( + emptyPolygon(), + polygon(ring(0, 0, 10, 0, 0, 10, 0, 0), ring(1, 1, 1, 2, 2, 1, 1, 1))), + 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))", + collection( + point(1, 2), + emptyLineString(), + emptyPolygon(), + emptyMultiPoint(), + emptyMultiLineString(), + emptyMultiPolygon(), + collection( + emptyPoint(), + emptyLineString(), + emptyPolygon(), + emptyMultiPoint(), + emptyMultiLineString(), + emptyMultiPolygon())), + box(1, 2, 1, 2))); + } + + private static Stream<Arguments> extraDimensionCases() { + return Stream.of( + Arguments.of("POINT Z(1 2 3)", pointZ(1, 2, 3), box(1, 2, 1, 2)), + Arguments.of("POINT M(1 2 3)", pointM(1, 2, 3), box(1, 2, 1, 2)), + Arguments.of("POINT ZM(1 2 3 4)", pointZM(1, 2, 3, 4), box(1, 2, 1, 2)), + Arguments.of( + "LINESTRING Z(0 1 9,2 -1 9)", lineStringZ(0, 1, 9, 2, -1, 9), box(0, -1, 2, 1))); Review Comment: Suggest adding a `POLYGON Z` case. Every polygon in the suite is 2D, so `readRing` never runs with `numDimensions` above 2 and the closure comparison over Z and M is uncovered. -- 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]
