szehon-ho commented on code in PR #17509: URL: https://github.com/apache/iceberg/pull/17509#discussion_r3807722160
########## core/src/main/java/org/apache/iceberg/GeometryBoundsCollector.java: ########## @@ -0,0 +1,309 @@ +/* + * 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 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. {@code NaN} values do not + * contribute to a dimension, and 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>The bounds of a polygon are derived from its exterior ring alone, which assumes OGC-valid + * polygons whose interior rings lie within the shell. This matches the envelope computed for a + * polygon by geometry libraries such as JTS. Iceberg does not validate geometries, so a polygon + * with a hole extending past its shell produces bounds that do not contain the geometry. + */ +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; + + // ISO WKB encodes the dimensions of a geometry in the thousands digit of its type code + 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 MAX_DEPTH = 100; + + private final DimensionBounds xBounds = new DimensionBounds(); + private final DimensionBounds yBounds = 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. + * + * <p>If this throws, the collector's state is undefined: coordinates parsed before the failure + * may already be folded in. A caller that continues after a rejected value must discard this + * collector. + * + * @param wkb a buffer containing exactly one WKB geometry + * @throws IllegalArgumentException if the WKB is malformed or has a non-finite coordinate + */ + public void add(ByteBuffer wkb) { + Preconditions.checkArgument(wkb != null, "Invalid WKB buffer: null"); + ByteBuffer buffer = wkb.duplicate(); + parseGeometry(buffer, 0, ANY_GEOMETRY); + Preconditions.checkArgument(!buffer.hasRemaining(), "Invalid WKB: trailing data"); + } + + /** + * Returns the accumulated bounding box, or {@code null} if either the X or Y dimension 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); + + // 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); + } + + try { + parseGeometryBody(buffer, depth, expectedType); + } finally { + buffer.order(callerOrder); + } + } + + private void parseGeometryBody(ByteBuffer buffer, int depth, int expectedType) { + long typeCode = buffer.getInt() & 0xFFFFFFFFL; + long dimensionGroup = typeCode / 1000; + int geometryType = (int) (typeCode % 1000); + Preconditions.checkArgument( + geometryType >= TYPE_POINT + && geometryType <= TYPE_GEOMETRY_COLLECTION + && dimensionGroup <= XYZM_GROUP, + "Invalid or unsupported WKB geometry type: %s", + typeCode); + Preconditions.checkArgument( + expectedType == ANY_GEOMETRY || geometryType == expectedType, + "Invalid WKB: expected geometry type %s but found %s", + typeName(expectedType), + typeName(geometryType)); + + int numDimensions = numDimensions(dimensionGroup); + + switch (geometryType) { + case TYPE_POINT: + readCoordinate(buffer, numDimensions); + break; + case TYPE_LINE_STRING: + readCoordinateSequence(buffer, numDimensions, true); + break; + case TYPE_POLYGON: + readPolygon(buffer, numDimensions); + 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 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 int numDimensions(long dimensionGroup) { + switch ((int) 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 void readPolygon(ByteBuffer buffer, int numDimensions) { + int numRings = readCount(buffer); + if (numRings > 0) { + readCoordinateSequence(buffer, numDimensions, true); + } + + for (int i = 1; i < numRings; i += 1) { + readCoordinateSequence(buffer, numDimensions, false); + } + } + + private void readCollection(ByteBuffer buffer, int depth, int expectedChildType) { + int numElements = readCount(buffer); + for (int i = 0; i < numElements; i += 1) { + // each child carries its own byte order, type code, and dimensions + parseGeometry(buffer, depth + 1, expectedChildType); + } + } + + private void readCoordinateSequence(ByteBuffer buffer, int numDimensions, boolean updateBounds) { + int numPoints = readCount(buffer); + long numBytes = (long) numPoints * numDimensions * Double.BYTES; + checkRemaining(buffer, numBytes); + if (!updateBounds) { + buffer.position(buffer.position() + (int) numBytes); + return; + } + + for (int i = 0; i < numPoints; i += 1) { + readCoordinate(buffer, numDimensions); + } + } + + private void readCoordinate(ByteBuffer buffer, int numDimensions) { + checkRemaining(buffer, (long) numDimensions * Double.BYTES); + double xCoord = buffer.getDouble(); + double yCoord = buffer.getDouble(); + // only X and Y contribute to the box; skip any Z and M ordinates + for (int i = 2; i < numDimensions; i += 1) { + buffer.getDouble(); + } + + // NaN marks an empty ordinate and is skipped per the spec, but an infinite coordinate is a real + // position that a finite box cannot cover; rejecting it avoids silently producing bounds that + // omit an object in the file. + Preconditions.checkArgument( + !Double.isInfinite(xCoord) && !Double.isInfinite(yCoord), + "Invalid WKB: coordinate is not finite"); + + xBounds.add(xCoord); + yBounds.add(yCoord); + } + + private static int readCount(ByteBuffer buffer) { + checkRemaining(buffer, Integer.BYTES); + long count = buffer.getInt() & 0xFFFFFFFFL; + // every element or point occupies at least one more byte, so a count larger than the bytes left + // cannot be valid; catch it here with a precise message instead of looping until the buffer + // ends + Preconditions.checkArgument( Review Comment: Suggest adding an `invalidWkbCases` entry for this, e.g. a `LINESTRING` header declaring 1000 points followed by 20 bytes. Nothing in the suite reaches this precondition today. `truncated point` has no count field at all and fails in `checkRemaining` with "unexpected end of buffer", and `multi point with a line string child` carries `count = 1` against ample remaining bytes. Since this check was added specifically to replace a vague truncation error with a precise one, and it's the only path to this message, a future edit could delete it and every test would still pass. For what it's worth, I confirmed the bound can't false-reject: the cheapest element is a 4-byte empty ring, so `remaining >= count` holds for all well-formed input. ########## core/src/main/java/org/apache/iceberg/GeometryBoundsCollector.java: ########## @@ -0,0 +1,309 @@ +/* + * 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 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. {@code NaN} values do not + * contribute to a dimension, and 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>The bounds of a polygon are derived from its exterior ring alone, which assumes OGC-valid + * polygons whose interior rings lie within the shell. This matches the envelope computed for a + * polygon by geometry libraries such as JTS. Iceberg does not validate geometries, so a polygon + * with a hole extending past its shell produces bounds that do not contain the geometry. + */ +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; + + // ISO WKB encodes the dimensions of a geometry in the thousands digit of its type code + 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 MAX_DEPTH = 100; + + private final DimensionBounds xBounds = new DimensionBounds(); + private final DimensionBounds yBounds = 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. + * + * <p>If this throws, the collector's state is undefined: coordinates parsed before the failure + * may already be folded in. A caller that continues after a rejected value must discard this + * collector. + * + * @param wkb a buffer containing exactly one WKB geometry + * @throws IllegalArgumentException if the WKB is malformed or has a non-finite coordinate + */ + public void add(ByteBuffer wkb) { + Preconditions.checkArgument(wkb != null, "Invalid WKB buffer: null"); + ByteBuffer buffer = wkb.duplicate(); + parseGeometry(buffer, 0, ANY_GEOMETRY); + Preconditions.checkArgument(!buffer.hasRemaining(), "Invalid WKB: trailing data"); + } + + /** + * Returns the accumulated bounding box, or {@code null} if either the X or Y dimension 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); + + // 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); + } + + try { + parseGeometryBody(buffer, depth, expectedType); + } finally { + buffer.order(callerOrder); + } + } + + private void parseGeometryBody(ByteBuffer buffer, int depth, int expectedType) { + long typeCode = buffer.getInt() & 0xFFFFFFFFL; + long dimensionGroup = typeCode / 1000; + int geometryType = (int) (typeCode % 1000); + Preconditions.checkArgument( + geometryType >= TYPE_POINT + && geometryType <= TYPE_GEOMETRY_COLLECTION + && dimensionGroup <= XYZM_GROUP, + "Invalid or unsupported WKB geometry type: %s", + typeCode); + Preconditions.checkArgument( + expectedType == ANY_GEOMETRY || geometryType == expectedType, + "Invalid WKB: expected geometry type %s but found %s", + typeName(expectedType), + typeName(geometryType)); + + int numDimensions = numDimensions(dimensionGroup); + + switch (geometryType) { + case TYPE_POINT: + readCoordinate(buffer, numDimensions); + break; + case TYPE_LINE_STRING: + readCoordinateSequence(buffer, numDimensions, true); + break; + case TYPE_POLYGON: + readPolygon(buffer, numDimensions); + 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 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 int numDimensions(long dimensionGroup) { + switch ((int) 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 void readPolygon(ByteBuffer buffer, int numDimensions) { + int numRings = readCount(buffer); + if (numRings > 0) { + readCoordinateSequence(buffer, numDimensions, true); + } + + for (int i = 1; i < numRings; i += 1) { + readCoordinateSequence(buffer, numDimensions, false); + } + } + + private void readCollection(ByteBuffer buffer, int depth, int expectedChildType) { + int numElements = readCount(buffer); + for (int i = 0; i < numElements; i += 1) { + // each child carries its own byte order, type code, and dimensions + parseGeometry(buffer, depth + 1, expectedChildType); + } + } + + private void readCoordinateSequence(ByteBuffer buffer, int numDimensions, boolean updateBounds) { + int numPoints = readCount(buffer); + long numBytes = (long) numPoints * numDimensions * Double.BYTES; + checkRemaining(buffer, numBytes); + if (!updateBounds) { + buffer.position(buffer.position() + (int) numBytes); + return; + } + + for (int i = 0; i < numPoints; i += 1) { + readCoordinate(buffer, numDimensions); + } + } + + private void readCoordinate(ByteBuffer buffer, int numDimensions) { + checkRemaining(buffer, (long) numDimensions * Double.BYTES); + double xCoord = buffer.getDouble(); + double yCoord = buffer.getDouble(); + // only X and Y contribute to the box; skip any Z and M ordinates + for (int i = 2; i < numDimensions; i += 1) { + buffer.getDouble(); + } + + // NaN marks an empty ordinate and is skipped per the spec, but an infinite coordinate is a real + // position that a finite box cannot cover; rejecting it avoids silently producing bounds that + // omit an object in the file. + Preconditions.checkArgument( Review Comment: Suggest deleting this check and letting ±Infinity become a bound. Infinite bounds are representable (`GeospatialBound` writes raw doubles, and applies no finiteness validation), spec-legal (`format/spec.md:748` bans only NaN as a bound, and `format/spec.md:651` places `-Infinity`/`Infinity` inside the ordered value range), and `DoubleFieldMetrics.Builder.addValue` already accepts them — a `double` column holding `+Infinity` writes `+Infinity` as its upper bound today. So the comment's reasoning — that an infinite ordinate is "a real position that a finite box cannot cover" — doesn't hold: `xmax = +Infinity` is constructible, round-trips exactly, and does contain the object. Nothing is lost by accepting it. As written this repeats the Z/M shape from the last round. Once #17161 calls `addValue` as the first statement of `GeometryWriter.write()` with no try/catch, this propagates out of `FileAppender.add()` and aborts the task, so a `POINT(Infinity 0)` produced by PostGIS or Sedona fails the `INSERT` and the file can never be compacted. ########## core/src/main/java/org/apache/iceberg/GeometryBoundsCollector.java: ########## @@ -0,0 +1,309 @@ +/* + * 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 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. {@code NaN} values do not + * contribute to a dimension, and 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>The bounds of a polygon are derived from its exterior ring alone, which assumes OGC-valid + * polygons whose interior rings lie within the shell. This matches the envelope computed for a + * polygon by geometry libraries such as JTS. Iceberg does not validate geometries, so a polygon + * with a hole extending past its shell produces bounds that do not contain the geometry. + */ +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; + + // ISO WKB encodes the dimensions of a geometry in the thousands digit of its type code + 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 MAX_DEPTH = 100; + + private final DimensionBounds xBounds = new DimensionBounds(); + private final DimensionBounds yBounds = 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. + * + * <p>If this throws, the collector's state is undefined: coordinates parsed before the failure + * may already be folded in. A caller that continues after a rejected value must discard this + * collector. + * + * @param wkb a buffer containing exactly one WKB geometry + * @throws IllegalArgumentException if the WKB is malformed or has a non-finite coordinate + */ + public void add(ByteBuffer wkb) { + Preconditions.checkArgument(wkb != null, "Invalid WKB buffer: null"); + ByteBuffer buffer = wkb.duplicate(); + parseGeometry(buffer, 0, ANY_GEOMETRY); + Preconditions.checkArgument(!buffer.hasRemaining(), "Invalid WKB: trailing data"); + } + + /** + * Returns the accumulated bounding box, or {@code null} if either the X or Y dimension 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); + + // 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); + } + + try { + parseGeometryBody(buffer, depth, expectedType); + } finally { + buffer.order(callerOrder); + } + } + + private void parseGeometryBody(ByteBuffer buffer, int depth, int expectedType) { + long typeCode = buffer.getInt() & 0xFFFFFFFFL; + long dimensionGroup = typeCode / 1000; + int geometryType = (int) (typeCode % 1000); + Preconditions.checkArgument( + geometryType >= TYPE_POINT + && geometryType <= TYPE_GEOMETRY_COLLECTION + && dimensionGroup <= XYZM_GROUP, + "Invalid or unsupported WKB geometry type: %s", + typeCode); + Preconditions.checkArgument( Review Comment: Suggest replacing this precondition with an `if` and a `throw`, so the message is built only on failure. Precondition arguments are evaluated eagerly, so both `typeName` calls run on every parsed geometry. At the top level `expectedType` is `ANY_GEOMETRY`, which falls to `typeName`'s `default` branch and runs `String.valueOf(0)` — a `byte[]` plus a `String` per value written, in a per-row write path. The `typeCode` argument on the preceding precondition is fine, since Guava has a primitive `long` overload. ########## core/src/main/java/org/apache/iceberg/GeometryBoundsCollector.java: ########## @@ -0,0 +1,309 @@ +/* + * 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 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. {@code NaN} values do not + * contribute to a dimension, and 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>The bounds of a polygon are derived from its exterior ring alone, which assumes OGC-valid + * polygons whose interior rings lie within the shell. This matches the envelope computed for a + * polygon by geometry libraries such as JTS. Iceberg does not validate geometries, so a polygon + * with a hole extending past its shell produces bounds that do not contain the geometry. + */ +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; + + // ISO WKB encodes the dimensions of a geometry in the thousands digit of its type code + 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 MAX_DEPTH = 100; + + private final DimensionBounds xBounds = new DimensionBounds(); + private final DimensionBounds yBounds = 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. + * + * <p>If this throws, the collector's state is undefined: coordinates parsed before the failure + * may already be folded in. A caller that continues after a rejected value must discard this + * collector. + * + * @param wkb a buffer containing exactly one WKB geometry + * @throws IllegalArgumentException if the WKB is malformed or has a non-finite coordinate + */ + public void add(ByteBuffer wkb) { + Preconditions.checkArgument(wkb != null, "Invalid WKB buffer: null"); + ByteBuffer buffer = wkb.duplicate(); + parseGeometry(buffer, 0, ANY_GEOMETRY); + Preconditions.checkArgument(!buffer.hasRemaining(), "Invalid WKB: trailing data"); + } + + /** + * Returns the accumulated bounding box, or {@code null} if either the X or Y dimension 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); + + // 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); + } + + try { + parseGeometryBody(buffer, depth, expectedType); + } finally { + buffer.order(callerOrder); + } + } + + private void parseGeometryBody(ByteBuffer buffer, int depth, int expectedType) { + long typeCode = buffer.getInt() & 0xFFFFFFFFL; + long dimensionGroup = typeCode / 1000; + int geometryType = (int) (typeCode % 1000); + Preconditions.checkArgument( + geometryType >= TYPE_POINT + && geometryType <= TYPE_GEOMETRY_COLLECTION + && dimensionGroup <= XYZM_GROUP, + "Invalid or unsupported WKB geometry type: %s", + typeCode); + Preconditions.checkArgument( + expectedType == ANY_GEOMETRY || geometryType == expectedType, + "Invalid WKB: expected geometry type %s but found %s", + typeName(expectedType), + typeName(geometryType)); + + int numDimensions = numDimensions(dimensionGroup); + + switch (geometryType) { + case TYPE_POINT: + readCoordinate(buffer, numDimensions); + break; + case TYPE_LINE_STRING: + readCoordinateSequence(buffer, numDimensions, true); + break; + case TYPE_POLYGON: + readPolygon(buffer, numDimensions); + 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 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 int numDimensions(long dimensionGroup) { + switch ((int) 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 void readPolygon(ByteBuffer buffer, int numDimensions) { Review Comment: Suggest passing interior rings through with `updateBounds` true, then deleting the parameter. I accepted the shell-only behavior last round once it was documented, and it is — but two things I didn't weigh then are worth surfacing before the test freezes it in. The failure mode is silent rather than loud: for a polygon whose hole extends past its shell, the box under-covers, `format/spec.md:768` is violated, and the file gets pruned from a query it should match, so the user sees missing rows and no error. Iceberg never validates geometry validity, so nothing upstream catches it. And reading every ring is a net deletion. `updateBounds` exists only for this case, so dropping it removes the branch in `readCoordinateSequence`, the parameter, the javadoc caveat, and the pinning test. The cost is reading doubles rather than advancing `position` over bytes already in cache, and a larger box is always conservative — for valid polygons it's byte-identical. The counterargument is real: JTS `Polygon.computeEnvelopeInternal()` uses the shell alone, so Iceberg would disagree with Parquet's footer `GeospatialStatistics`. If matching the footer is the priority this is defensible, but then the disagreement is that Iceberg is right and the footer is wrong. ########## core/src/test/java/org/apache/iceberg/TestGeometryBoundsCollector.java: ########## @@ -0,0 +1,501 @@ +/* + * 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 TestGeometryBoundsCollector { + + @ParameterizedTest(name = "{0}") + @MethodSource("boundingBoxCases") + void boundingBox(String wkt, Geom geom, BoundingBox expected) { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + ByteBuffer wkb = ByteBuffer.wrap(wkb(geom)); + 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 + 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(); + + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + bounds.add(slice); + + assertThat(slice.position()).isEqualTo(0); + assertThat(bounds.boundingBox()).isEqualTo(box(1, 2, 1, 2)); + } + + @Test + void noBoundsWhenOneDimensionIsMissing() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + bounds.add(ByteBuffer.wrap(wkb(point(1, Double.NaN)))); + + assertThat(bounds.boundingBox()).as("POINT(1 NaN)").isNull(); + } + + @Test + void boundsAcrossValuesWithMissingCoordinates() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + bounds.add(ByteBuffer.wrap(wkb(point(1, Double.NaN)))); + bounds.add(ByteBuffer.wrap(wkb(point(Double.NaN, 2)))); + + assertThat(bounds.boundingBox()).isEqualTo(box(1, 2, 1, 2)); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("extraDimensionCases") + void extraDimensionsAreIgnored(String description, Geom geom, BoundingBox expected) { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + + bounds.add(ByteBuffer.wrap(wkb(geom))); + + assertThat(bounds.boundingBox()).as(description).isEqualTo(expected); + } + + @Test + void boundsAcrossValuesWithDifferentDimensions() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + bounds.add(ByteBuffer.wrap(wkb(pointZ(1, 2, 3)))); + bounds.add(ByteBuffer.wrap(wkb(pointZM(1, 2, 3, 4)))); + + assertThat(bounds.boundingBox()).isEqualTo(box(1, 2, 1, 2)); + } + + @Test + void extraDimensionsNestedInCollectionAreIgnored() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + + bounds.add(ByteBuffer.wrap(wkb(collection(point(1, 2), pointZ(3, 4, 5))))); + + assertThat(bounds.boundingBox()).isEqualTo(box(1, 2, 3, 4)); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("invalidWkbCases") + void invalidWkb(String description, byte[] wkb, String expectedMessage) { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + + assertThatThrownBy(() -> bounds.add(ByteBuffer.wrap(wkb))) + .as(description) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining(expectedMessage); + } + + @Test + void nestingAtTheLimitIsAccepted() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + // 100 collection wrappers around POINT(1 2): the outermost is depth 0, the point is depth 100 + bounds.add(ByteBuffer.wrap(nestedCollections(100))); + + assertThat(bounds.boundingBox()).isEqualTo(box(1, 2, 1, 2)); + } + + @Test + void nestingPastTheLimitIsRejected() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + + assertThatThrownBy(() -> bounds.add(ByteBuffer.wrap(nestedCollections(101)))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("nesting too deep"); + } + + @Test + void bigEndianParentWithLittleEndianChild() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + // a big-endian multi point holding a little-endian point, the reverse of the MULTIPOINT case + bounds.add(ByteBuffer.wrap(wkb(multiPointBigEndian(point(1, 2))))); + + assertThat(bounds.boundingBox()).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(); + + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + bounds.add(direct); + + assertThat(direct.hasArray()).isFalse(); + assertThat(bounds.boundingBox()).isEqualTo(box(1, 2, 1, 2)); + } + + @Test + void interiorRingOutsideShellIsNotCovered() { + GeometryBoundsCollector bounds = new GeometryBoundsCollector(); + // 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.add( + ByteBuffer.wrap(wkb(polygon(ring(0, 0, 1, 0, 0, 1, 0, 0), ring(0, 0, 9, 0, 0, 9, 0, 0))))); + + assertThat(bounds.boundingBox()).isEqualTo(box(0, 0, 1, 1)); + } + + @Test + void stateIsUndefinedAfterAddThrows() { Review Comment: Suggest removing this test and keeping the javadoc contract on `add`. The comment concedes it documents "not a guaranteed rolled-back state," and what it actually asserts — that a truncated point throws "unexpected end of buffer" — is already covered by the `truncated point` case in `invalidWkbCases`. An undefined contract isn't testable, so the javadoc is the right and only place for it. -- 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]
