Skip to content

Updates 202606 - #90

Open
nmandery wants to merge 10 commits into
mainfrom
updates-202606
Open

Updates 202606#90
nmandery wants to merge 10 commits into
mainfrom
updates-202606

Conversation

@nmandery

@nmandery nmandery commented Jun 24, 2026

Copy link
Copy Markdown
Owner

@regislon

regislon commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Hi @nmandery — I just took the opportunity of having Fable 5 back to dig a little into this PR and the migration work you did here. First of all: really nice job, this is a big update and it reads very cleanly — and CI is green across the board. While going through it I spotted a few small things that might be worth a look, roughly by severity:

1. cells_to_wkb_polygons output changed from LargeBinary to Binary (i32 offsets).
In geoarrow-array 0.8, WkbArray is an alias for GenericWkbArray<i32>, so the new let out: WkbArray = ... (and the to_wkb_polygons call it forces) produces i32 offsets, while main used WKBArray<i64>. Verified by building this branch vs 0.22.0 from PyPI:

cells = geometry_to_cells(shapely.Point(8.9, 47.2), 8)
cells_to_wkb_polygons(cells).type
# 0.22.0:  arro3.core.DataType<LargeBinary>
# PR readthedocs/readthedocs.org#90:  arro3.core.DataType<Binary>

Besides the dtype change, WKB payloads > 2 GiB (~16M hexagon polygons) would panic on offset overflow, and it's inconsistent with cells_to_wkb_points / vertexes_to_wkb_points / directededges_to_wkb_linestrings, which still use <i64>. Suggested fix: GenericWkbArray<i64> (or the LargeWkbArray alias).

2. Empty geometries now yield nulls instead of empty cell lists. geo_traits::to_geo::ToGeoGeometry::try_to_geometry returns None for empty geometries, and from_geoarrow.rs maps that to Ok(None) → null entry, where main returned a valid empty list (and some POINT EMPTY-style WKB previously raised an error). If intentional, it may deserve a CHANGES.rst note since wkb_to_cells(..., flatten=False) list semantics change.

3. The .expect("wkbarray") calls are now reachable. The WKB conversion error type changed from Infallible to a real error, so the pre-existing expects in the four *_to_wkb_* functions became live panic paths — worth converting to into_pyresult()?.

4. Minor: the WKB output field name changed from "geometry" to "" (extension_field()data_type().to_field("", true)); geozero is bumped but appears to have no remaining usages and could be dropped; rasterh3 0.12 (2026-06-26) was released just after this branch and contains antimeridian/resolution-search fixes; crates/h3arrow/CHANGES.md has no entry for the 0.5.0 bump.

If you don't mind, I can propose these improvements directly in the PR, and you can validate them later on.

@nmandery

nmandery commented Jul 3, 2026

Copy link
Copy Markdown
Owner Author

Hi @regislon Thanks for looking into this - these are all valid findings. Please just go ahead and start a PR to address these.

For some of the issues like point 3 and the rasterh3 upgrade I also have started to prepare fixes, but so far that is not finished.

@keewis keewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some small nits, otherwise this looks good to me (and tests pass). Shall we merge this now and follow up with smaller PRs afterwards?

Comment on lines 16 to +17
use nom::IResult;
use nom::Parser;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use nom::IResult;
use nom::Parser;
use nom::{IResult, Parser};

Comment thread crates/h3arrow/Cargo.toml
ahash = "0.8"
arrow = { workspace = true }
geoarrow = { git = "https://github.com/geoarrow/geoarrow-rs", rev = "3ecf7dfc1816261b84f813eaf2a0174f2b5752d8", optional = true }
#geoarrow = { git = "https://github.com/geoarrow/geoarrow-rs", rev = "3ecf7dfc1816261b84f813eaf2a0174f2b5752d8", optional = true }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

Suggested change
#geoarrow = { git = "https://github.com/geoarrow/geoarrow-rs", rev = "3ecf7dfc1816261b84f813eaf2a0174f2b5752d8", optional = true }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Append a feature to contact a profile

3 participants