Rewrite NumGeometries and GeometryN behavior for TIN and PolyhedralSurface, add NumPatches and PatchN functions #252 #812
Conversation
… to NumPatches and PatchN TIN and PolyhedralSurface are now treated as unitary geometries: - ST_NumGeometries() returns 1 for non-empty TIN/PolyhedralSurface - ST_GeometryN() returns the whole geometry for index 1 - ST_NumPatches() preserves original patch counting behavior - ST_PatchN() provides access to individual patches - Added lwgeom_has_patches() helper function
📝 WalkthroughWalkthroughThe changes introduce and implement patch-level operations for TIN and PolyhedralSurface geometries in PostGIS. New C functions and SQL bindings for counting and extracting patches are added, with corresponding updates to documentation, regression tests, and geometry classification logic. The changelog is updated to reflect these breaking and feature changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SQL
participant C_Functions
participant Geometry
User->>SQL: CALL ST_NumPatches(geometry)
SQL->>C_Functions: LWGEOM_numpatches(geometry)
C_Functions->>Geometry: Check if geometry has patches
alt Geometry supports patches
C_Functions->>Geometry: Count patches
Geometry-->>C_Functions: Patch count
C_Functions-->>SQL: Return patch count
SQL-->>User: Patch count
else Not supported
C_Functions-->>SQL: Error
SQL-->>User: Error
end
User->>SQL: CALL ST_PatchN(geometry, n)
SQL->>C_Functions: LWGEOM_patchn(geometry, n)
C_Functions->>Geometry: Check if geometry has patches
alt Geometry supports patches
C_Functions->>Geometry: Extract nth patch
Geometry-->>C_Functions: Patch geometry or NULL
C_Functions-->>SQL: Return patch geometry
SQL-->>User: Patch geometry
else Not supported
C_Functions-->>SQL: Error
SQL-->>User: Error
end
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
NEWS (1)
18-19: Update the placeholder issue number.The changelog entry correctly describes the breaking change, but "XXXXX" should be replaced with the actual issue or PR number before merging.
- - XXXXX, ST_NumGeometries/ST_GeometryN treat TIN and PolyhedralSurface as unitary geometries, + - #812, ST_NumGeometries/ST_GeometryN treat TIN and PolyhedralSurface as unitary geometries,postgis/postgis.sql.in (1)
5495-5498: Add cost hint for consistency.The function implementation looks correct, but consider adding a cost hint for consistency with
ST_NumPatches. Since this is a simple geometry extraction operation,_COST_LOWwould be appropriate.CREATE OR REPLACE FUNCTION ST_PatchN(geometry, integer) RETURNS geometry AS 'MODULE_PATHNAME', 'LWGEOM_patchn' - LANGUAGE 'c' IMMUTABLE STRICT PARALLEL SAFE; + LANGUAGE 'c' IMMUTABLE STRICT PARALLEL SAFE + _COST_LOW;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
NEWS(2 hunks)liblwgeom/liblwgeom.h.in(1 hunks)liblwgeom/lwgeom.c(3 hunks)postgis/lwgeom_ogc.c(2 hunks)postgis/postgis.sql.in(1 hunks)regress/core/polyhedralsurface.sql(1 hunks)regress/core/polyhedralsurface_expected(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: macOS
- GitHub Check: Analyze (c-cpp)
- GitHub Check: CI (pg14-geos39-gdal33-proj71, tests)
- GitHub Check: CI (pg13-geos38-gdal31-proj71, tests)
- GitHub Check: CI (latest, tests)
- GitHub Check: CI (pg15-clang-geos311-gdal35-proj90, usan_clang)
- GitHub Check: CI (pg12-geos38-gdal30-proj611, tests)
- GitHub Check: CI (pg16-geos312-gdal37-proj921, coverage)
- GitHub Check: CI (pg15-geos311-gdal35-proj90, coverage)
- GitHub Check: CI (pg17-geosmain-gdal39-proj94, coverage)
- GitHub Check: CI (pg13-clang-geos39-gdal31-proj71, usan_clang)
🔇 Additional comments (13)
NEWS (1)
40-40: Good formatting cleanup.The removal of the trailing space improves consistency in the changelog formatting.
liblwgeom/liblwgeom.h.in (1)
910-913: Function declaration looks good.The
lwgeom_has_patchesfunction declaration follows proper C conventions and PostGIS coding style. The placement betweenlwgeom_is_unitaryandlwgeom_has_ringsis logical and maintains the organizational structure of the header file.regress/core/polyhedralsurface_expected (1)
17-23: Test expectations align with the behavioral change.The expected results correctly reflect the new behavior where PolyhedralSurface geometries are treated as unitary:
- Empty surfaces return 0 geometries
- Non-empty surfaces return 1 geometry (the surface itself)
- ST_GeometryN(surface, 1) returns the surface, other indices return empty
The format is consistent with existing test data patterns.
regress/core/polyhedralsurface.sql (1)
31-40: Comprehensive test coverage for the behavioral change.The test cases provide excellent coverage of the new unitary geometry behavior:
- Empty surface testing: Verifies
ST_NumGeometriesreturns 0 for empty PolyhedralSurface- Single/multi-patch testing: Confirms both return 1 geometry (treating surface as unitary)
- Index boundary testing: Tests valid index (1) and invalid indices (0, 2) for
ST_GeometryNThe test queries align perfectly with the expected results in
polyhedralsurface_expected, demonstrating that PolyhedralSurface geometries are now consistently treated as single units rather than collections of patches.liblwgeom/lwgeom.c (2)
1115-1117: Include PolyhedralSurface and TIN types as unitary geometries
Expandslwgeom_is_unitary()to treat these types as single‐component geometries forNumGeometries/GeometryN.
1174-1176: Classify TIN as a collection type
AddingTINTYPEtolwtype_is_collection()aligns it with other multi-geometry types, enabling collection operations (e.g., iteration) to include TIN.postgis/postgis.sql.in (1)
5488-5492: LGTM! Well-implemented C function integration.The migration from SQL conditional logic to a direct C function call is properly implemented with appropriate function attributes and cost hints.
postgis/lwgeom_ogc.c (6)
58-63: Function declarations look good.The new function declarations follow the established naming conventions and documentation style.
276-314: Implementation looks correct.Both functions properly use the helper function with appropriate flags and handle memory management correctly. The error handling for unsupported geometry types in
LWGEOM_numpatchesis appropriate.
316-369: Extraction logic is consistent with counting logic.The function properly handles all geometry types and the patches option. The index bounds checking and conversion from 1-based to 0-based indexing are correct.
371-399: Refactoring maintains backward compatibility correctly.The function properly treats TIN and PolyhedralSurface as unitary geometries and includes an optimization to avoid unnecessary copying when returning the original geometry.
401-436: New patch extraction function is well-implemented.The function follows the same robust pattern as
LWGEOM_geometryn_collectionwith appropriate type validation and error handling for non-patch geometries.
256-265:❓ Verification inconclusive
Verify the fallback logic for single-patch geometries.
The code attempts to cast patch geometries to LWCOLLECTION and falls back to returning 1 if the cast fails. This assumes single-patch TIN/PolyhedralSurface geometries might not cast successfully to collections.
🏁 Script executed:
#!/bin/bash # Description: Search for information about TIN and PolyhedralSurface structure and casting behavior # Look for lwgeom_as_lwcollection implementation ast-grep --pattern 'lwgeom_as_lwcollection($_) { $$$ }' # Search for TIN and PolyhedralSurface type definitions rg -A 10 "TINTYPE|POLYHEDRALSURFACETYPE" --type c # Look for lwgeom_has_patches implementation ast-grep --pattern 'lwgeom_has_patches($_) { $$$ }'Length of output: 71008
Confirm
lwgeom_as_lwcollectioncovers patch geometriesI wasn’t able to locate a definition of
lwgeom_as_lwcollectionin the repo. Before relying on the fallback (col == NULL ? 1), please verify that this cast:
- Returns non‐NULL for both TINTYPE and POLYHEDRALSURFACETYPE, and
- Is implemented (e.g. as an inline function or macro) so the fallback branch never executes at runtime.
• Check for its declaration/definition in headers like
liblwgeom/lwgeom_inline.h.
• Ensure it handles patch types the same waylwgeom_has_patchesdoes.
| int | ||
| lwgeom_has_patches(const LWGEOM *geom) | ||
| { | ||
| switch (geom->type) | ||
| { | ||
| case TINTYPE: | ||
| case POLYHEDRALSURFACETYPE: | ||
| return LW_TRUE; | ||
| break; | ||
| default: | ||
| return LW_FALSE; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
New lwgeom_has_patches helper for patch‐level support
Correctly identifies TIN and PolyhedralSurface for the new NumPatches/PatchN API. Please add a doxygen‐style comment above this function and ensure it is declared in the public header (liblwgeom.h) so it’s discoverable.
🤖 Prompt for AI Agents
In liblwgeom/lwgeom.c around lines 1125 to 1137, add a doxygen-style comment
above the lwgeom_has_patches function describing its purpose and behavior. Then
declare this function in the public header file liblwgeom.h to make it
accessible and discoverable by other code using the library.
cf https://git.osgeo.org/gitea/postgis/postgis/pulls/252
Opened here for the CI