szehon-ho commented on code in PR #17509: URL: https://github.com/apache/iceberg/pull/17509#discussion_r3858317286
########## core/src/main/java/org/apache/iceberg/GeometryBoundsBuilder.java: ########## @@ -0,0 +1,431 @@ +/* + * 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 com.google.errorprone.annotations.FormatMethod; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +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 X and Y bounds are produced. The spec also defines optional Z and M bounds, but producing + * them is a deliberate non-goal here: any Z and M ordinates in a value are read past rather than + * bounded, so an XYZ or XYZM value still parses and contributes its X and Y. + * + * <p>Every ring of a polygon, including interior rings, contributes to the bounds. In a valid + * polygon the holes lie inside the shell, so this is the shell's own box. + */ +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(); + // set when a value could not be fully bounded -- malformed or unsupported WKB, or bytes left + // after the declared geometry -- so an object may be missing from the box; build() then + // suppresses the bounds + private boolean incomplete = false; + + /** + * 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>A value the parser cannot bound never fails the caller. Malformed WKB, a valid OGC type this + * builder does not support (such as PolyhedralSurface, TIN, or Triangle), and bytes left after + * the declared geometry all mean an object may be missing from the box, so {@link #build()} + * returns no bounds for the file instead. Suppressing optional metrics is safe where failing the + * write is not, since the write cannot skip the value or retry past it. + * + * @param wkb a buffer containing one WKB geometry + * @throws IllegalArgumentException if {@code wkb} is null + */ + public void addValue(ByteBuffer wkb) { + Preconditions.checkArgument(wkb != null, "Invalid WKB buffer: null"); + ByteBuffer buffer = wkb.duplicate(); + try { + parseGeometry(buffer, ANY_GEOMETRY, ANY_DIMENSION); + } catch (InvalidWkbException e) { Review Comment: Suggest reinstating a nesting-depth cap that throws `InvalidWkbException`. `catch (InvalidWkbException e)` does not catch `StackOverflowError`, and the recursion `parseGeometry` -> `parseGeometryBodyAndUpdateBound` -> `readCollection` -> `parseGeometry` is bounded only by the input length. A nested `GEOMETRYCOLLECTION` chain costs 9 bytes per level (`01 07000000 01000000`), and `readCount`'s `count <= remaining` check passes at every level for `count = 1`, so a ~50 KB value reaches several thousand frames -- enough to overflow a default 1 MB thread stack across three frames per level. The error then escapes `FileAppender.add()` and aborts the task, which is the exact failure mode the javadoc on line 84 now rules out ("A value the parser cannot bound never fails the caller"). I asked for `MAX_DEPTH` to stop aborting writes in an earlier round and 9dc5c16 removed it outright, which was reasonable at the time -- the only way to reject was to throw. That objection is gone now: a depth counter that throws `InvalidWkbException` lands in this catch and just costs the file its bounds, same as a short element count. ########## core/src/test/java/org/apache/iceberg/TestGeometryBoundsBuilder.java: ########## @@ -0,0 +1,542 @@ +/* + * 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 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("unboundableWkbCases") + void unboundableWkbSuppressesBounds(String description, byte[] wkb) { Review Comment: Suggest adding a case where a bad value sits between two good ones. `unboundableWkbSuppressesBounds` adds a single bad value, so it cannot distinguish "suppress the file's bounds" from "discard this value's contributions" -- both produce `null`. The file-level behavior is the whole point of the flag, and nothing currently fails if a later change scopes suppression per value, which would silently reintroduce the under-covering bounds that get files pruned from queries they should match. `addValue(point(1, 2))`, then a `polygon ring not closed`, then `addValue(point(3, 4))`, asserting `build()` is null. ########## core/src/main/java/org/apache/iceberg/GeometryBoundsBuilder.java: ########## @@ -0,0 +1,431 @@ +/* + * 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 com.google.errorprone.annotations.FormatMethod; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +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 X and Y bounds are produced. The spec also defines optional Z and M bounds, but producing + * them is a deliberate non-goal here: any Z and M ordinates in a value are read past rather than + * bounded, so an XYZ or XYZM value still parses and contributes its X and Y. + * + * <p>Every ring of a polygon, including interior rings, contributes to the bounds. In a valid + * polygon the holes lie inside the shell, so this is the shell's own box. + */ +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(); + // set when a value could not be fully bounded -- malformed or unsupported WKB, or bytes left + // after the declared geometry -- so an object may be missing from the box; build() then + // suppresses the bounds + private boolean incomplete = false; + + /** + * 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>A value the parser cannot bound never fails the caller. Malformed WKB, a valid OGC type this + * builder does not support (such as PolyhedralSurface, TIN, or Triangle), and bytes left after + * the declared geometry all mean an object may be missing from the box, so {@link #build()} + * returns no bounds for the file instead. Suppressing optional metrics is safe where failing the + * write is not, since the write cannot skip the value or retry past it. + * + * @param wkb a buffer containing one WKB geometry + * @throws IllegalArgumentException if {@code wkb} is null + */ + public void addValue(ByteBuffer wkb) { Review Comment: Suggest returning early here when `incomplete` is already set. Once the flag is set the result can never come back, but every remaining row in the file still pays a full WKB walk whose output is discarded. This guard existed before 9dc5c16 removed it along with the depth limit; it's one line and it also caps the blast radius of the recursion. ########## core/src/main/java/org/apache/iceberg/GeometryBoundsBuilder.java: ########## @@ -0,0 +1,431 @@ +/* + * 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 com.google.errorprone.annotations.FormatMethod; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +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 X and Y bounds are produced. The spec also defines optional Z and M bounds, but producing + * them is a deliberate non-goal here: any Z and M ordinates in a value are read past rather than + * bounded, so an XYZ or XYZM value still parses and contributes its X and Y. + * + * <p>Every ring of a polygon, including interior rings, contributes to the bounds. In a valid + * polygon the holes lie inside the shell, so this is the shell's own box. + */ +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(); + // set when a value could not be fully bounded -- malformed or unsupported WKB, or bytes left + // after the declared geometry -- so an object may be missing from the box; build() then + // suppresses the bounds + private boolean incomplete = false; + + /** + * 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>A value the parser cannot bound never fails the caller. Malformed WKB, a valid OGC type this + * builder does not support (such as PolyhedralSurface, TIN, or Triangle), and bytes left after + * the declared geometry all mean an object may be missing from the box, so {@link #build()} + * returns no bounds for the file instead. Suppressing optional metrics is safe where failing the + * write is not, since the write cannot skip the value or retry past it. + * + * @param wkb a buffer containing one WKB geometry + * @throws IllegalArgumentException if {@code wkb} is null + */ + public void addValue(ByteBuffer wkb) { + Preconditions.checkArgument(wkb != null, "Invalid WKB buffer: null"); + ByteBuffer buffer = wkb.duplicate(); + try { + parseGeometry(buffer, ANY_GEOMETRY, ANY_DIMENSION); + } catch (InvalidWkbException e) { + incomplete = true; + return; + } + + // a complete geometry that leaves bytes behind was not fully parsed (for example a multi + // geometry whose declared element count is short), so an object never reached the bounds + if (buffer.hasRemaining()) { + incomplete = true; + } + } + + /** + * Builds the bounding box covering every geometry added, or {@code null} if either the X or Y + * dimension has no value, or if any value could not be bounded (see {@link #addValue}). + */ + public BoundingBox build() { + if (incomplete || !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 Review Comment: Suggest dropping the save/restore, or keeping it and fixing the comment. The comment justifies this as protecting "a sibling read after a nested geometry," but that read can't be misordered: `parseGeometry` starts with a single-byte `get()` and sets the order from the child's own flag before any multi-byte read, and no WKB body has parent data following its children -- `readCollection` reads its count before the loop and nothing after it. So nothing can fail if `buffer.order(callerOrder)` is deleted. Either is fine, but the comment currently describes a scenario the format doesn't produce. ########## core/src/main/java/org/apache/iceberg/GeometryBoundsBuilder.java: ########## @@ -0,0 +1,431 @@ +/* + * 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 com.google.errorprone.annotations.FormatMethod; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +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 X and Y bounds are produced. The spec also defines optional Z and M bounds, but producing + * them is a deliberate non-goal here: any Z and M ordinates in a value are read past rather than + * bounded, so an XYZ or XYZM value still parses and contributes its X and Y. + * + * <p>Every ring of a polygon, including interior rings, contributes to the bounds. In a valid + * polygon the holes lie inside the shell, so this is the shell's own box. + */ +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(); + // set when a value could not be fully bounded -- malformed or unsupported WKB, or bytes left + // after the declared geometry -- so an object may be missing from the box; build() then + // suppresses the bounds + private boolean incomplete = false; + + /** + * 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>A value the parser cannot bound never fails the caller. Malformed WKB, a valid OGC type this + * builder does not support (such as PolyhedralSurface, TIN, or Triangle), and bytes left after + * the declared geometry all mean an object may be missing from the box, so {@link #build()} + * returns no bounds for the file instead. Suppressing optional metrics is safe where failing the + * write is not, since the write cannot skip the value or retry past it. + * + * @param wkb a buffer containing one WKB geometry + * @throws IllegalArgumentException if {@code wkb} is null + */ + public void addValue(ByteBuffer wkb) { + Preconditions.checkArgument(wkb != null, "Invalid WKB buffer: null"); + ByteBuffer buffer = wkb.duplicate(); + try { + parseGeometry(buffer, ANY_GEOMETRY, ANY_DIMENSION); + } catch (InvalidWkbException e) { + incomplete = true; + return; + } + + // a complete geometry that leaves bytes behind was not fully parsed (for example a multi + // geometry whose declared element count is short), so an object never reached the bounds + if (buffer.hasRemaining()) { + incomplete = true; + } + } + + /** + * Builds the bounding box covering every geometry added, or {@code null} if either the X or Y + * dimension has no value, or if any value could not be bounded (see {@link #addValue}). + */ + public BoundingBox build() { + if (incomplete || !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 InvalidWkbException("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); + // only the seven OGC types in XY/XYZ/XYM/XYZM are bounded here; other valid OGC types (such as + // PolyhedralSurface, TIN, and Triangle) are unsupported and cost the value its bounds + checkWkb( + 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 InvalidWkbException( + "Invalid WKB: expected geometry type " + + typeName(expectedType) + + " but found " + + typeName(geometryType)); + } + if (expectedDimension != ANY_DIMENSION && dimensionGroup != expectedDimension) { + throw new InvalidWkbException( + "Invalid WKB: expected dimensions " + + dimensionName(expectedDimension) + + " but found " + + dimensionName(dimensionGroup)); + } + + int numDimensions = numDimensions(dimensionGroup); + + switch (geometryType) { Review Comment: Suggest switch expressions here and in `numDimensions`, `typeName`, and `dimensionName`. `AGENTS.md` asks for `case X -> ...` over statement switches, and the three value-returning ones convert cleanly. Related: the `default:` on line 202 is unreachable, since `checkWkb` on line 155 already constrains `geometryType` to 1-7, so its message can never be produced. -- 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]
