Repository navigation
Fixes for the updates-202606 migration (follow-up to #90 review) - #94
Merged
Merged
Conversation
geoarrow-array 0.8's `WkbArray` is an alias for `GenericWkbArray<i32>`, so the migration silently changed the output of cells_to_wkb_polygons from large_binary to binary. This was inconsistent with the other *_to_wkb_* functions (which kept <i64>) and would panic on i32 offset overflow for WKB payloads over 2 GiB. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ToWKB* error type changed from Infallible to a real error in the
geoarrow 0.8 migration, turning the pre-existing .expect("wkbarray")
calls into reachable panic paths. Convert them into Python exceptions.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The migration replaced extension_field() with data_type().to_field("", true),
changing the arro3-visible field name from "geometry" to "".
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Picks up the antimeridian and resolution-search fixes released on 2026-06-26, plus iterator perf improvements. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Allows using the enum as a dict key / in sets, as expected from a Python enum. It was unhashable in 0.22 as well (eq without hash removes the default __hash__). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- all *_to_wkb_* functions return large_binary (i64 offsets) - WKB output field is named "geometry" and carries the geoarrow.wkb extension metadata - wkb_to_cells: flatten/list consistency, per-geometry compact round-trip via uncompact, containment mode ordering, null entry preservation - document the pre-existing compact+flatten 'heterogeneous resolution' failure (present in 0.22.0 as well) as xfail Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
keewis
reviewed
Aug 17, 2026
keewis
left a comment
keewis
left a comment
Collaborator
There was a problem hiding this comment.
this looks good to me, I just have some small nits.
Comment on lines
+254
to
+255
| // LargeWkbArray (i64 offsets) to match the other WKB-producing functions and the | ||
| // pre-0.23 behavior, and to avoid i32 offset overflow on large outputs. |
Collaborator
There was a problem hiding this comment.
can you remove the comparison ("pre-0.23 behavior"), please? This kind of comment tends to age really badly, so I'd stick this in the changelog
Comment on lines
+63
to
+65
| # regression test: with geoarrow-array 0.8, the `WkbArray` alias uses i32 | ||
| # offsets. All WKB-producing functions must keep returning large_binary | ||
| # (i64 offsets) as in h3ronpy 0.22. |
Collaborator
There was a problem hiding this comment.
on the other hand, this is fine (but quite possibly we don't need the "as in h3ronpy 0.22")
Comment on lines
+112
to
+113
| reason="compact+flatten compacts the already per-geometry-compacted (mixed resolution) " | ||
| "cells a second time and fails with 'heterogeneous resolution'. Present in 0.22.0 as well.", |
Collaborator
There was a problem hiding this comment.
Suggested change
| reason="compact+flatten compacts the already per-geometry-compacted (mixed resolution) " | |
| "cells a second time and fails with 'heterogeneous resolution'. Present in 0.22.0 as well.", | |
| reason=( | |
| "compact+flatten compacts the already per-geometry-compacted (mixed resolution) " | |
| "cells a second time and fails with 'heterogeneous resolution'. Present in 0.22.0 as well." | |
| ), |
nmandery
approved these changes
Aug 18, 2026
nmandery
left a comment
nmandery
left a comment
Owner
There was a problem hiding this comment.
Looks good to me - thank you for the fixes.
Merged
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
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.
Follow-up to the review comments on #90, as discussed there. All changes are confined to the
h3ronpycrate (Python bindings) — nothing incrates/h3arrowis touched.Fixes
cells_to_wkb_polygonsreturnslarge_binaryagain — geoarrow-array 0.8'sWkbArrayalias isGenericWkbArray<i32>; the function now usesLargeWkbArray(i64 offsets) like the other*_to_wkb_*functions, restoring the 0.22 output dtype and avoiding i32 offset overflow on > 2 GiB payloads.ToWKB*error type is no longerInfallible, so the four.expect("wkbarray")calls became reachable panic paths. (You mentioned you had started on this one — feel free to drop commit5d52ea4in favor of your version.)"geometry"again (was""after theextension_field()→to_field("", true)change).rasterh30.11 → 0.12 — picks up the antimeridian and resolution-search fixes from 2026-06-26; compiles without code changes. (Same here — dropb3626baif your version is further along.)ContainmentModeis now hashable (hash, frozenon the pyclass) — it was unhashable in 0.22 as well; noticed while writing the containment-mode tests.Tests added (
tests/arrow/test_vector.py)*_to_wkb_*functions returnlarge_binarygeometryand carries theARROW:extension:name = geoarrow.wkbmetadatawkb_to_cells: flatten/list-array consistency, per-geometrycompactverified by round-tripping throughuncompact, containment-mode count ordering, null entries preservedNot addressed here (out of scope for the bindings crate)
crates/h3arrow/src/array/from_geoarrow.rs(try_to_geometry()returningNonefor empty geometries) — left for you to decide whether to restore the empty-list behavior or document the new semantics.wkb_to_cells(..., compact=True, flatten=True)fails withValueError: heterogeneous resolution— the flatten path compacts the already per-geometry-compacted (mixed-resolution) cells a second time (from_geoarrow.rs,ToCellIndexArray::to_cellindexarray). This is pre-existing — 0.22.0 from PyPI fails identically — so not a regression from this migration. Documented as anxfailtest for now.geozerodependency and the missingcrates/h3arrow/CHANGES.mdentry, for the same reason.Verified locally on macOS arm64 / Python 3.12:
cargo test --workspace,cargo clippy,cargo fmt --check, and the full pytest suite (50 passed, 1 xfailed).🤖 Generated with Claude Code