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: (5 comments) Went through PS6 - one correctness issue around quad_segs, plus a few smaller notes. Nothing blocking beyond the first one. 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)); Four small ones around this parse, take or leave: - insert() keeps the first occurrence, so "quad_segs=8 quad_segs=64" silently uses 8. Rejecting a repeated key reads friendlier than picking one. - When mitre_limit fails to parse, the message names miter_limit (the constant handed to parseDoubleOption), sending the reader after a key they never typed. - BUFFER_STYLE_MAX_LEN works out to 378 and sums a subset of the keys with their longest values, so it needs revisiting whenever a key is added, and 378 is not a number a user can act on. A round fixed cap would do the same job. - Indentation slipped at parseIntOption (line 520) and the miter_limit block (line 674), and "if(buf.size() == 1)" at 591 is missing a space. http://gerrit.cloudera.org:8080/#/c/24707/6/be/src/exprs/geo/geometry-wrapper-wkb.cc@643 PS6, Line 643: points_per_circle_ *= 4; quad_segs above 63 silently produces a coarser buffer instead of a finer one: points_per_circle_ is an int, but DefaultStrategyJoin/End/Point take uint8_t, so the value wraps on the way in. Probe against the toolchain's boost 1.74.0-p1 (the version in impala-config.sh), buffering POINT(0 0) by 1: quad_segs=8 -> 32 points_per_circle -> 33 points, area 3.12145 quad_segs=16 -> 64 -> 65 points, area 3.13655 quad_segs=63 -> 252 -> 253 points, area 3.14127 quad_segs=64 -> 256 -> uint8_t 0 -> 4 points, area 1.29904 quad_segs=100 -> 400 -> uint8_t 144 -> 145 points, area 3.1406 quad_segs=64 is exactly what someone reaches for when they want a smooth circle, and it comes back with 41% of the area and no error. The multiplication is the other half: ParseBufferStyleLongestAllowedString passes quad_segs=2147483647, and RunTestWithContext without a lambda asserts init_result is true, so this line overflows a signed int on a path the tests keep green. Boost takes std::size_t in all three strategy constructors, so would taking size_t (or int) in the DefaultStrategy* helpers, plus a cap on quad_segs at parse time, cover both? The two paths disagree today: with an explicit endcap=round or join=round the value reaches buff::end_round/join_round untruncated, while strategy_point_ always goes through DefaultStrategyPoint. 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) { Reading the code, a NULL third or fourth argument is treated as "not given" rather than as NULL: the 3- and 4-arg st_Buffer_WKB overloads drop their extra arguments, and line 609 returns early on a null style. So ST_Buffer(g, 1, NULL) would return the buffer, while the same function returns NULL when geom or distance is NULL. I did not build this to confirm it. Is the asymmetry deliberate - does the ESRI version behave that way? A query test for both would settle it; the ctest covers the parse side only. http://gerrit.cloudera.org:8080/#/c/24707/6/be/src/exprs/geo/geospatial-functions-wkb-ir.cc File be/src/exprs/geo/geospatial-functions-wkb-ir.cc: http://gerrit.cloudera.org:8080/#/c/24707/6/be/src/exprs/geo/geospatial-functions-wkb-ir.cc@57 PS6, Line 57: delete wrapper; When InitFromPrepareArgs fails the state is deleted and never set, while ParseGeom does wrapper->FromWkb(geom) with no null check. That is safe today only because every failure path here calls ctx->SetError() and ScalarFnCall::OpenEvaluator returns on has_error() before any Eval runs - a future failure path that forgets SetError turns this into a null dereference during the scan. Setting the state unconditionally, or a DCHECK on the invariant, would keep it from resting on that. http://gerrit.cloudera.org:8080/#/c/24707/6/be/src/exprs/geo/geospatial-functions-wkb-ir.cc@644 PS6, Line 644: BufferWrapperWkb* buffer_wrapper = reinterpret_cast<BufferWrapperWkb*>(wrapper); GeometryWrapperWkb has a virtual destructor now, so static_cast is the right tool for this downcast - reinterpret_cast happens to work under single inheritance but is not guaranteed to. -- 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 16:54:11 +0000 Gerrit-HasComments: Yes
