Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24707 )
Change subject: IMPALA-15270: Implement Geospatial ST_Buffer Function Using Boost ...................................................................... Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/24707/6/be/src/exprs/geo/geometry-wrapper-wkb.cc File be/src/exprs/geo/geometry-wrapper-wkb.cc: http://gerrit.cloudera.org:8080/#/c/24707/6/be/src/exprs/geo/geometry-wrapper-wkb.cc@634 PS6, Line 634: pairs.insert(move(kv)); > * I intended to take the first provided value, but I checked PostGIS (which All four look right in PS7 - last value wins on a repeated key, the message now names the spelling that was actually typed, the fixed 1000-char cap, and the indentation. http://gerrit.cloudera.org:8080/#/c/24707/6/be/src/exprs/geo/geometry-wrapper-wkb.cc@643 PS6, Line 643: points_per_circle_ *= 4; > Good catch! I updated all types of the variables that hold points per circ The size_t change covers both the truncation and the multiplication, thanks. What is still open is the upper bound: parseIntOption accepts anything up to 2147483647, so points_per_circle_ can reach 8589934588 and goes straight into point_circle/join_round/end_round. Probed against the toolchain's boost 1.74.0-p1 with this patch's strategy set (distance_symmetric, side_straight, join_round, end_round, point_circle), buffering POINT(0 0) by 1: quad_segs=1000 -> 4001 points, 0.00 s, 2.6 MB peak RSS quad_segs=100000 -> 400001 points, 0.04 s, 30.3 MB quad_segs=1000000 -> 4000001 points, 0.46 s, 295.3 MB RSS is the whole probe process, which idles at 2.6 MB, so the first row is the floor rather than the cost. Growth is linear in quad_segs, paid per row, and boost allocates outside the query's MemTracker, so a large value shows up as process memory pressure rather than a query-level memory error. ParseBufferStyleLongestAllowedString asserts that quad_segs=2147483647 is accepted, so the ctest currently pins that in. Would a cap at parse time work, rejected with the same "Invalid value" error? What PostGIS does would pick the number better than I can. http://gerrit.cloudera.org:8080/#/c/24707/6/be/src/exprs/geo/geometry-wrapper-wkb.cc@756 PS6, Line 756: if (!use_spheroid->is_null && use_spheroid->val) { > The current Java implementation does not support the use_spheroid or buffer The NULL results in PS7 read right, and the new query tests cover them. One thing about where the checks ended up: InitFromPrepareArgs runs from GeometryWrapperBufferPrepare, which the framework calls before any eval, so the new is_null checks in st_Buffer_WKB have not run yet at that point - the header comment says those checks "must be done before calling this function", but no caller does them first. For the numeric args it is benign, since AllocateAnyVal memsets the constant AnyVal, so dist->val is 0.0 and use_spheroid->val is false. The one I would keep is the dropped `if (buffer_style->is_null) return true;`: ParseBufferStyle now builds std::string(nullptr, 0) for a constant NULL style, and ParseBufferStyleNullStyleArg walks exactly that path. Constructing a string from a null pointer is UB and sanitizer builds can flag it. RelationWrapperPrepare in geospatial-functions-wkb-ir.cc keeps the same kind of check on its constant args. -- To view, visit http://gerrit.cloudera.org:8080/24707 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6faeb533eb464d32629fa402179dc19c25bd78f2 Gerrit-Change-Number: 24707 Gerrit-PatchSet: 6 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Tue, 25 Aug 2026 23:10:59 +0000 Gerrit-HasComments: Yes
