Jason Fehr 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) > 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. I checked both PostGIS and Apache Sedona, and neither has a max value for quad segs. However, I added a max value of 999,999 just to help with memory utilization. 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: "Retu Done 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: TestGeospatialFuction Since this change depends on https://gerrit.cloudera.org/c/24534 (which sets WKB_EXPERIMENTAL as the default geospatial library), the tests in test_geospatial_functions.py will run in WKB_EXPERIMENTAL mode which means the 3 and 4 arg overloads will be available. Great idea on using ST_NumPoints instead of the hardcoded buffer polygon! The purpose of this test is to ensure the second value of "quad_segs" is used and thus only needs to assert the total number of points is correct. -- 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: Wed, 26 Aug 2026 16:58:51 +0000 Gerrit-HasComments: Yes
