feat: perf wins + ~35 new functions (monolithic review branch)

[pierre-warnier]

Scope

Large branch intended for bulk Copilot review. Will be decomposed into ~20 small atomic PRs before upstream submission. Do not merge as-is — this exists to catch issues early.

Perf / correctness (committed)

• Envelope pre-check, BFS→DFS, early-exit in spatial join (commit 5c0c4a2, 618103e)
• Row_array Hilbert permutation → 1.7× speedup (ea6f4bc)
• Full STR bulk loading for persistent R-tree (6a82a94)
• Batch bbox extraction in probe (5229320)
• Eliminate intermediate memcpy in GEOS deser (aa9d8d4)
• Shewchuk robust predicates replacing naive orient2d_fast (508bda5)
• Native ST_Intersects for GEOMETRY without GEOS (e5cc7bc)
• Fix: geometry_version.test for DuckDB v1.5 CRS type changes (582186d)

New functions (grouped)

Linestring editors — ST_AddPoint, ST_SetPoint, ST_RemovePoint, ST_SwapOrdinates, ST_ShiftLongitude
Geometry constructors — ST_ForceCollection, ST_Scroll, ST_QuantizeCoordinates, ST_MemSize, ST_Polygon, ST_LineFromMultiPoint
3D measurement — ST_3DLength, ST_3DPerimeter, ST_3DDistance, ST_DFullyWithin
Distance/analysis — ST_LongestLine, ST_Summary
Algorithms — ST_ChaikinSmoothing, ST_GeometricMedian, ST_SimplifyVW, ST_AsLatLonText
Validity — ST_IsValidDetail, ST_RelateMatch
Encoded polyline — ST_AsEncodedPolyline, ST_LineFromEncodedPolyline
Linear referencing — ST_AddMeasure, ST_3DLineInterpolatePoint
GeoHash decode — ST_GeomFromGeoHash, ST_Box2dFromGeoHash
Grids — ST_SquareGrid, ST_HexagonGrid (table functions)
Dump — ST_DumpPoints, ST_DumpRings, ST_DumpSegments (table functions)
EWKT / EWKB — ST_AsEWKT, ST_GeomFromEWKT, ST_AsEWKB, ST_GeomFromEWKB
Processing — ST_Subdivide, ST_Split (GEOS-backed)
Clustering agg — ST_ClusterIntersecting, ST_ClusterWithin
Geodesic — ST_Project (haversine forward)
PostGIS compat — ST_SRID, ST_SetSRID (no-op, for API compat)
Binary format — ST_AsTWKB, ST_GeomFromTWKB

Review focus

• Correctness of distance math (especially 3D and haversine)
• Memory handling in vertex array manipulation functions (linestring editors)
• Edge cases: empty geometries, single-point lines, degenerate polygons
• NULL and validity propagation
• Thread safety (all scalar, so stateless — but want to be sure)

Out of scope

• Any architectural changes
• New types
• Spatial join optimizer changes (already covered in KNN PR stack fix: ST_Distance_Sphere::BindData::Copy std::move for GCC 11+ duckdb/duckdb-spatial#795-test: comprehensive KNN spatial join test suite duckdb/duckdb-spatial#798)

[Copilot]

Pull request overview

Large spatial extension branch adding many new spatial functions/table functions plus multiple performance/correctness improvements across R-tree build/scan, spatial join, GEOS deserialization, and SGL predicates.

Changes:

• Add new scalar + table functions (grid generation, dump functions, EWKT/EWKB/TWKB, line editors, 3D functions, geodesic projection, etc.) with extensive SQL tests.
• Improve R-tree creation/scan locality (STR X-sort, Hilbert row permutation) and spatial join probe bbox extraction.
• Introduce robust predicates (Shewchuk-style) and several math micro-optimizations (distance/length/perimeter, haversine).

Reviewed changes

Copilot reviewed 30 out of 31 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
test/sql/index/rtree_str_quality.test	Adds STR bulk-load quality checks via spatial query correctness on structured/random point sets.
test/sql/grid/st_square_grid.test	Adds functional + error-case coverage for ST_SquareGrid.
test/sql/grid/st_hexagon_grid.test	Adds functional + error-case coverage for ST_HexagonGrid.
test/sql/geos/st_subdivide_split.test	Adds tests for GEOS-backed ST_Subdivide and ST_Split.
test/sql/geos/st_isvaliddetail_relatematch.test	Adds tests for ST_IsValidDetail and ST_RelateMatch.
test/sql/geos/st_cluster_agg.test	Adds tests for ST_ClusterIntersecting and ST_ClusterWithin aggregates.
test/sql/geometry/st_swap_shift.test	Adds tests for ST_SwapOrdinates and ST_ShiftLongitude.
test/sql/geometry/st_project.test	Adds tests for geodesic ST_Project.
test/sql/geometry/st_native_intersects.test	Adds tests for native ST_Intersects behavior on common geometry pairs.
test/sql/geometry/st_longestline_summary.test	Adds tests for ST_LongestLine and ST_Summary.
test/sql/geometry/st_linestring_editors.test	Adds tests for ST_AddPoint, ST_SetPoint, ST_RemovePoint.
test/sql/geometry/st_linear_ref.test	Adds tests for ST_AddMeasure and ST_3DLineInterpolatePoint.
test/sql/geometry/st_geometry_constructors.test	Adds tests for several constructor/utility functions (ForceCollection/Scroll/Quantize/MemSize/Polygon/LineFromMultiPoint).
test/sql/geometry/st_geohash_decode.test	Adds tests for GeoHash decode helpers.
test/sql/geometry/st_ewkt_ewkb.test	Adds tests for EWKT/EWKB conversion helpers.
test/sql/geometry/st_encoded_polyline.test	Adds tests for encoded polyline encode/decode.
test/sql/geometry/st_dump_functions.test	Adds tests for ST_DumpPoints, ST_DumpRings, ST_DumpSegments table functions.
test/sql/geometry/st_algorithms.test	Adds tests for smoothing/median/simplify/as-latlon functions.
test/sql/geometry/st_3d_functions.test	Adds tests for 3D distance/length/perimeter + ST_DFullyWithin.
test/sql/geometry/geometry_version.test	Updates compatibility expectations for DuckDB v1.5 GEOMETRY/CRS binding behavior.
src/spatial/operators/spatial_join_physical.cpp	Optimizes R-tree scan (BFS→DFS) and batches probe bbox extraction.
src/spatial/modules/main/spatial_functions_table.cpp	Implements new table functions: square/hex grids and dump functions + docs/registration.
src/spatial/modules/main/spatial_functions_scalar.cpp	Adds many new scalar functions + a native ST_Intersects GEOMETRY variant and various perf tweaks.
src/spatial/modules/geos/geos_serde.cpp	Removes intermediate memcpy when building GEOS coord sequences from blobs.
src/spatial/modules/geos/geos_module.cpp	Adds GEOS-backed ST_IsValidDetail, ST_RelateMatch, ST_Subdivide, ST_Split, and clustering aggregates.
src/spatial/index/rtree/rtree_index_create_physical.cpp	Adds STR step-1 X-center sort before bottom-up build.
src/sgl/sgl.hpp	Micro-optimizes haversine implementation.
src/sgl/sgl.cpp	Switches orientation predicate to robust implementation; minor distance micro-optimizations; envelope early-exit for line-line distance.
src/sgl/robust_predicates.hpp	Declares robust predicate APIs with documentation.
src/sgl/robust_predicates.cpp	Implements robust predicates initialization + orient2d/incircle.
src/sgl/CMakeLists.txt	Adds robust_predicates.cpp to build.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

ST_ForceCollection skips deep copying for multipart child geometries (// Skip deep copy for now). This drops data for types like MULTIPOLYGON (polygon parts are multipart because they contain rings), yielding empty/invalid output geometries. Please implement a proper deep copy of nested multipart geometries (parts + vertex arrays) or use an existing clone/copy utility so all sub-geometry structure is preserved.

Suggested change

	// Create a fresh part geometry on the arena
	auto *part = static_cast<sgl::geometry *>(alloc.alloc(sizeof(sgl::geometry)));
	new (part) sgl::geometry(geom.get_type(), geom.has_z(), geom.has_m());
	if (geom.is_multi_part()) {
	// For multi types, copy part structure
	auto *child = geom.get_first_part();
	for (uint32_t i = 0; i < geom.get_part_count(); i++) {
	auto *child_copy = static_cast<sgl::geometry *>(alloc.alloc(sizeof(sgl::geometry)));
	new (child_copy) sgl::geometry(child->get_type(), child->has_z(), child->has_m());
	if (child->is_multi_part()) {
	// Skip deep copy for now
	} else {
	child_copy->set_vertex_array(child->get_vertex_array(), child->get_vertex_count());
	}
	part->append_part(child_copy);
	child = child->get_next();
	}
	} else {
	// For simple types, just copy vertex data
	part->set_vertex_array(geom.get_vertex_array(), geom.get_vertex_count());
	}
	gc.append_part(part);
	auto clone_geometry = [&](auto &self, const sgl::geometry &source) -> sgl::geometry * {
	auto *copy = static_cast<sgl::geometry *>(alloc.alloc(sizeof(sgl::geometry)));
	new (copy) sgl::geometry(source.get_type(), source.has_z(), source.has_m());
	if (source.is_multi_part()) {
	auto *child = source.get_first_part();
	for (uint32_t i = 0; i < source.get_part_count(); i++) {
	copy->append_part(self(self, *child));
	child = child->get_next();
	}
	} else {
	copy->set_vertex_array(source.get_vertex_array(), source.get_vertex_count());
	}
	return copy;
	};
	gc.append_part(clone_geometry(clone_geometry, geom));

[Copilot]

ST_3DDistance is documented/registered as “minimum 3D distance between two geometries”, but the implementation computes the minimum distance only between vertex pairs. This is incorrect for lines/polygons where the minimum distance can occur between interior points on segments/faces, not at vertices. Please either (a) implement true geometry-to-geometry distance in 3D (segment–segment, point–segment, etc., recursively), or (b) tighten the contract/name/docs to explicitly state it is vertex-to-vertex distance.

[Copilot]

ST_DFullyWithin currently computes “fully within distance” using only vertex-to-vertex distances (max over vertices in geom1 of min distance to vertices in geom2). This is not equivalent to “every point of geom1 is within distance d of geom2” for lines/polygons (farthest point can lie along an edge, and distance to geom2 should be to segments/areas, not just vertices). Please implement the correct definition (e.g., max distance from geom1 to geom2 using segment/triangle distance, or GEOS GEOSHausdorffDistanceDensify/buffer+within when GEOS is available), or change the function name/docs to reflect the approximation.

[Copilot]

Same alignment concern as in the LINESTRING case: ptr from BinaryReader::Reserve is not guaranteed to be double-aligned, but is cast to const double* and passed to GEOS. Please restore an aligned copy (or add an aligned reserve API) before calling GEOSCoordSeq_copyFromBuffer_r.

[Copilot]

Header comment claims incircle returns an “Exact result” and “never give incorrect results due to floating-point roundoff errors”, but the implementation explicitly returns the approximate determinant in the hard cases (see robust_predicates.cpp:174-177). Please adjust the header documentation to match actual behavior, or implement the full adaptive refinement so the function is truly exact.

[Copilot]

ST_Intersects GEOMETRY variant returns has_a && has_b as a final fallback (after only checking bbox overlap). This produces false positives: any two geometries with intersecting bounding boxes will be reported as intersecting, even when they do not. Please change the fallback to a correct predicate (e.g., implement the missing geometry cases using SGL primitives / distance==0 only when it can be computed exactly, or return an error/NULL when unsupported), rather than returning bbox overlap as a surrogate for intersection.

[Copilot]

GEOS deserialization now passes reinterpret_cast<const double*>(ptr) where ptr comes from BinaryReader::Reserve, which does not guarantee double alignment (it returns a const char* into an arbitrary buffer). On architectures requiring aligned access this can crash or produce UB. Please either copy into an aligned double buffer (as before), or ensure/guarantee alignment from the reader (e.g., ReserveAligned/memcpy into arena.AllocateAligned).

[Copilot]

robust::init() uses a non-atomic initialized flag and writes multiple global constants. If orient2d/incircle are called concurrently on first use, this can race and lead to partially initialized constants. Please make initialization thread-safe (e.g., std::once_flag/std::call_once, or function-local statics that are guaranteed thread-safe in C++11+).

[Copilot]

The comment says “Row-major order: iterate j (rows) first, then i (columns)”, but the index math (col_offset = flat_idx % cols; row_offset = flat_idx / cols;) makes i vary fastest (inner loop), i.e., iterating columns first. Please correct the comment to reflect the actual output order to avoid confusion for users relying on ordering.

Suggested change

	// Row-major order: iterate j (rows) first, then i (columns)
	// Row-major order: iterate i (columns) fastest within each j (row)

[pierre-warnier]

All 9 Copilot review items addressed in b339bef:

1. ST_ForceCollection MULTIPOLYGON data loss — replaced shallow copy with a recursive clone_recursive lambda that walks parts and copies vertex arrays.
2. ST_3DDistance vertex-only bug — restricted inputs to POINT geometries; InvalidInputException for LINESTRING/POLYGON. Tests updated.
3. ST_DFullyWithin vertex-only bug — same POINT-only restriction + test updates.
4. GEOS alignment UB (LINESTRING) — restored memcpy into aligned arena buffer before GEOSCoordSeq_copyFromBuffer_r.
5. GEOS alignment UB (POLYGON rings) — same alignment fix applied to the ring path.
6. robust_predicates incircle doc overclaim — header comment updated to reflect "fast path only" semantics (approximate determinant in hard cases).
7. ST_Intersects false positives — fallback no longer return has_a && has_b; computes sgl::ops::get_euclidean_distance and checks dist <= 0.
8. robust::init race — wrapped in std::call_once with a local std::once_flag.
9. ST_GeneratePoints grid iteration comment — fixed to reflect that the inner loop varies i (columns) fastest.

Full suite: 1633 assertions / 127 test cases passing locally.

[Copilot]

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

ST_Intersects ExecuteGeometry assumes POINT/LINESTRING inputs are non-empty. For empty LINESTRINGs, n can be 0/1 and for (uint32_t i = 0; i < n - 1; i++) underflows, leading to out-of-bounds reads. Also get_vertex_xy(0) is called for POINT without checking get_vertex_count() > 0. Add early returns for empty geometries (e.g., if point/line has 0 vertices return false) and guard the loop with if (n < 2) return false;.

[Copilot]

ST_SimplifyVW can simplify closed rings (polygon rings are LINESTRING parts) down to 2 vertices because the stop condition is remaining > 2. For closed rings this can yield an invalid ring (e.g., two identical endpoints), producing invalid polygons. Consider detecting closed linestrings (first==last) and enforcing a minimum of 4 vertices (or returning EMPTY) and ensuring closure is preserved after simplification.

[Copilot]

Probe-side bbox extraction now assumes the join key is a GEOMETRY blob (GetData<string_t> + Serde::TryGetBounds). Spatial join predicates like && accept BOX_2D on one side (see spatial_functions_scalar.cpp:3335), so the probe key may be BOX_2D rather than GEOMETRY. In that case this will read the wrong vector type and can crash or produce incorrect bboxes. Please either restore the generic bbox expression path for the probe side, or branch on probe_side_key->return_type (BOX_2D vs GEOMETRY) and extract bounds accordingly.

[Copilot]

This test now asserts st_astext(geom) fails to bind on the legacy v1.0.0 database. However, this PR also introduces an implicit cast from legacy BLOB-alias GEOMETRY to native GEOMETRY (see spatial_functions_cast.cpp) and adds legacy_geometry_cast.test that relies on ST_AsText(geom) succeeding. These two tests appear inconsistent—either the cast should make st_astext work here, or the cast/test should be adjusted. Please align the expected behavior and update this test accordingly.

[Copilot]

The new STR pre-sort loads all RTreeEntry values into an in-memory vector<RTreeEntry> all_entries before sorting. For large indexes this can cause significant memory spikes (doubling storage vs the underlying ManagedCollection) and may negate the benefit of external buffering. Consider using an external sort / in-place sort on the managed collection, or gating this full materialization behind a size threshold (fall back to the previous approach when rtree_size is large).