feat(wkb): add MultiPolygon support to WKB parser#44
Conversation
* Import from apache arrow root level * Fix lint * Consolidate imports to use the main one
56b3c02 to
0149696
Compare
a0a7f2b to
30c751d
Compare
|
pushed 30c751d to make the linter happy |
30c751d to
da33f57
Compare
kylebarron
left a comment
There was a problem hiding this comment.
I think the WKB parser really needs a wholesale re-design. Or, perhaps the design is fine, but there are a lot of considerations
Perhaps you'd be able to point Claude at this folder in geoarrow-rs and enough test cases and it would be able to create a WKB -> GeoArrow parser.. but I'm skeptical that a claude-created version would be possible to maintain without a lot of additional input effort.
I do think the Rust implementation there is quite stable and cleanly-designed, but it's reasonably verbose. You can read, say, the wkb parsing/encoding from cast.
I think you'd need something like https://github.com/developmentseed/geotiff-test-data: a considerable corpus of test data for WKB - GeoArrow interop to have confidence in the code.
That said I don't really have the time to review big changes here at the moment.
| "@loaders.gl/schema": "*", | ||
| "@loaders.gl/wkt": "*", |
There was a problem hiding this comment.
I definitely wouldn't want to depend on loaders.gl from geoarrow-js.
| | MultiPoint | ||
| | MultiLineString | ||
| | MultiPolygon; | ||
| export type WKB = Binary; |
There was a problem hiding this comment.
You'd want a type for LargeWKB too
|
Thanks @kylebarron. Agreed on the bigger picture. This PR is just adding the missing MultiPolygon case to the existing parser (one function + tests), because I want to use that elsewhere. Happy to convert to draft if you'd rather revisit the whole thing at once. |
Adds
WKBType.MultiPolygoncase toparseWkb(). Based on the WKB parser from thekyle/wkbbranch, adding one more nesting level for geom-to-polygon offsets usingpolygonIndicesfrom loaders.gl'sBinaryPolygonGeometry.MultiPolygon is a core OGC Simple Features type. The types (
MultiPolygonData,WKBType.MultiPolygon) and type guards already exist in this library, only theparseWkb()case was missing.Changes
src/io/wkb.tsrepackMultiPolygons()+inferMultiPolygonCapacity()src/io/wkb.test.tssrc/type.ts,src/data.tsLargeWKB/LargeWKBDatatype aliasesTests verify offset arrays at every nesting level and spot-check coordinate values. WKB hex generated via Shapely. All 8 tests pass (4 new + 4 existing).
Note
Based on
kyle/wkb. stac-map depends on smohiudd/geoarrow-js (a fork of that branch) because it was never merged upstream.AI (Claude) supported my development of this PR.