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. -- 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]
