szehon-ho commented on code in PR #17509: URL: https://github.com/apache/iceberg/pull/17509#discussion_r3807759952
########## core/src/test/java/org/apache/iceberg/TestGeometryBoundsBuilder.java: ########## @@ -0,0 +1,529 @@ +/* + * 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(); + + bounds.addValue(ByteBuffer.wrap(wkb(collection(point(1, 2), pointZ(3, 4, 5))))); + + 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 nestingAtTheLimitIsAccepted() { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + // 100 collection wrappers around POINT(1 2): the outermost is depth 0, the point is depth 100 + bounds.addValue(ByteBuffer.wrap(nestedCollections(100))); + + assertThat(bounds.build()).isEqualTo(box(1, 2, 1, 2)); + } + + @Test + void nestingPastTheLimitIsRejected() { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + + assertThatThrownBy(() -> bounds.addValue(ByteBuffer.wrap(nestedCollections(101)))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("nesting too deep"); + } + + @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 interiorRingOutsideShellIsNotCovered() { + GeometryBoundsBuilder bounds = new GeometryBoundsBuilder(); + // documented limitation: only the exterior ring is read, so an interior ring past the shell is + // not covered; this pins the behavior so a future change to read every ring is noticed + bounds.addValue( + ByteBuffer.wrap(wkb(polygon(ring(0, 0, 1, 0, 0, 1, 0, 0), ring(0, 0, 9, 0, 0, 9, 0, 0))))); + + assertThat(bounds.build()).isEqualTo(box(0, 0, 1, 1)); + } + + @Test + void stateIsUndefinedAfterAddValueThrows() { Review Comment: Suggest dropping this and keeping the javadoc contract on `addValue`. An undefined contract isn't testable, and what it asserts — a truncated point throwing — is already covered by `truncated point` in `invalidWkbCases`. -- 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]
