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 7: (2 comments) Re-read PS7 - the size_t types, last-wins on duplicate keys, the miter/mitre message, the fixed cap, static_cast and the DCHECK all look right. Three things left, one of them a crash: - Buffer() takes the distance from GetConstantArg in the branch where the argument is not constant, so ST_Buffer(geom_col, dist_col) dereferences a null pointer. - The three new queries in geospatial-wkb-serialization.test also run under HIVE_ESRI, where the 4-arg ST_Buffer is not registered. - quad_segs still has no upper bound; quad_segs=1000000 already costs ~300 MB inside boost per call. Details inline. http://gerrit.cloudera.org:8080/#/c/24707/7/be/src/exprs/geo/geometry-wrapper-wkb.cc File be/src/exprs/geo/geometry-wrapper-wkb.cc: http://gerrit.cloudera.org:8080/#/c/24707/7/be/src/exprs/geo/geometry-wrapper-wkb.cc@576 PS7, Line 576: DoubleVal* distance = reinterpret_cast<DoubleVal*>( GetConstantArg returns NULL when the argument is not constant (udf.h: "Returns NULL if the argument is not constant"), and this else branch is exactly the not-constant case, so distance->val dereferences null on the first row of ST_Buffer(geom_col, dist_col) - an impalad crash rather than a query error. PS6 read the eval-time argument here, which is the only place that value exists on this path. Nothing in the tests hits it: every ST_Buffer query test passes a literal distance, and the ctests leave constant_args[1] null but never reach Buffer(), only InitFromPrepareArgs. One query test with a column as the distance would cover it. http://gerrit.cloudera.org:8080/#/c/24707/7/testdata/workloads/functional-query/queries/QueryTest/geospatial-wkb-serialization.test File testdata/workloads/functional-query/queries/QueryTest/geospatial-wkb-serialization.test: http://gerrit.cloudera.org:8080/#/c/24707/7/testdata/workloads/functional-query/queries/QueryTest/geospatial-wkb-serialization.test@179 PS7, Line 179: select ST_AsText(ST_Buffer(ST_GeomFromText('point (0 0)'), 1, false, 'quad_segs=4 quad_segs=64')); geospatial-wkb-serialization.test runs in both modes: TestGeospatialFuctions::test_wkb_serialization on the default cluster, where start-impala-cluster.py passes geospatial_library=HIVE_ESRI, and TestGeospatialLibrary::test_wkb_experimental_serialization under WKB_EXPERIMENTAL. In HIVE_ESRI mode initBuiltins takes the addNatives branch, addWkbNatives is never called, and the only ST_Buffer left is the Java one in java/hive-geospatial-functions, whose single evaluate() takes (BytesWritable, DoubleWritable) - so these three queries fail analysis there. geospatial-esri-specific-overloads.test is the ESRI-only file; a WKB-only counterpart, run just from test_geospatial_library.py, would mirror it. Separately, the expected value pins boost's exact double formatting across 257 vertices. ST_NumPoints or ST_Area on the result shows last-wins just as clearly and survives a boost bump. -- 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: 7 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:11:16 +0000 Gerrit-HasComments: Yes
