Refactor/shared coord helpers#4
Merged
Merged
Conversation
Pulls coordinate storage construction, coord I/O (struct + FSL with slice offsets), WKT parsing, and JSON decoding into shared helpers (coord.go, wkt.go, jsoncoord.go) so new geometry types compose rather than copy from point/polygon. Also adds interleaved (FixedSizeList) storage support to PolygonType, renames the typo'd GeometryTypeID constants (MultiPointMIDID -> MultiPointMID etc.; no callers existed), adds Dimension.String().
- Move interleavedStorage/interleavedFieldName from point.go to coord.go so all coord-storage primitives live together. - Drop the boolean-flag coordStorage(dim, bool) wrapper; callers use coordStructStorage / interleavedStorage directly. - Hoist storage type-switch above the per-coord loop in readCoordsRange and appendCoords; FSL read uses a single copy() over Float64Values(), struct read/write caches *Float64 / *Float64Builder per field. - readCoordAt uses len(dst) (caller is source of truth) instead of re-deriving stride from the FSL type. - Combine wktBody + wktPrefix into one wktSplit that returns both, and avoid uppercasing the entire WKT body (only the prefix is scanned now). - Simplify the redundant hasZM check in dimFromWKTPrefix. - Drop decodeCoordArray 2-line wrapper; rename decodeFloatsUntilClose to decodeFloatsAfterOpen for symmetry with decodeOpenOrNull. - Trim narrative godocs per CLAUDE.md rules.
WKT:
- wktSplit now accepts dimensioned EMPTY ("POLYGON Z EMPTY", "POLYGON ZM
EMPTY") and enforces a word boundary on the type keyword so "POLYGONFOO("
no longer matches.
- dimFromWKTPrefix returns an error when the prefix's dim marker conflicts
with the observed vertex stride (e.g. "POINT Z(1 2)").
- splitTopLevelGroups returns an error on unbalanced parens instead of
silently truncating.
- formatCoord + formatCoordRing extract the duplicated f-format vertex
loop from point/polygon String().
Storage:
- nestedListStorage + unwrapNestedLists generalize List-wrapping/unwrapping
for nested geometry storage. polygonStorage / polygonCoordStorage are
rewritten on top of them; this also turns polygon Deserialize's
previously unguarded type assertions into errors. Adds a regression test.
- Move DimensionFromStorage, DimensionFromStructType,
DimensionFromInterleavedType, checkCoordStructFields,
checkCoordInterleaved from point.go into coord.go so future geometry
types don't depend on point.go.
Tests:
- TestPolygonInterleavedRoundTrip now also exercises WKT (ValueStr) and
JSON (Marshal/Unmarshal) round-trips per dimension.
- TestPolygonDeserializeRejectsBadStorage covers the new error path.
Cleanup:
- decodeFloatsAfterOpen drops its unused dst parameter.
- Trim narrate-what doc on PolygonType.
- gofmt -w geometry.go (constant alignment) fixes SG-001 / SG-002. - Add godoc on every exported symbol added or touched on this branch (SG-020): Dimension and its constants/methods, GeometryTypeID constants block, PointType / PolygonType, all PointValue / PolygonValue methods, the NewPoint*/NewPolygon* constructors, the *With* option functions, and the PointArray / PointBuilder / PolygonArray / PolygonBuilder aliases. - Document why DimensionFromStructType, DimensionFromInterleavedType, and DimensionFromStorage are exported (SG-023): they are the entry points downstream geometry types (LineString, MultiPolygon, etc.) use to introspect coord storage. - Annotate the four "unsupported type" panics in coord.go as unreachable given checkCoordStorage validation; PointValue.GeometryType and PolygonValue.GeometryType likewise (SG-053).
paleolimbot
approved these changes
May 24, 2026
paleolimbot
left a comment
paleolimbot
left a comment
Contributor
There was a problem hiding this comment.
Feel free to merge, but did you want to add any tests here? (Apologies if I missed them). If you use geoarrow-data as a submodule you can do some quick roundtrip tests with the example data to ensure that claude doesn't roll thorough and break stuff (and ensuring that future Claudes that roll through copy the existing code quality).
Collaborator
Author
|
@paleolimbot - no added tests, but there were a fair amount before and this is just a refactor. I'll add some tests on the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is a major refactor to prepare for addition of the remaining geometry types using common helpers. Most of this was done via Claude Code. I do plan to add a AI/Agent contributor guide to this repo soon, but thought the scope of that merited a deeper discussion.
What changed
New shared helpers
appendCoord / appendCoords, and the exported DimensionFrom{StructType,InterleavedType,Storage} (moved from point.go).
Hot-path improvements
*Float64 / *Float64Builder per field. This removes O(n_verts) repeated type assertions in polygon read/write.
Correctness fixes
TestPolygonDeserializeRejectsBadStorage.
New feature
JSON / Deserialize for all four dimensions.
Misc
invariant