chore: delete the dead handbrake cover by mailletf · Pull Request #35 · nexmoov/rusty-polygon-geohasher

mailletf · 2026-08-25T00:43:26Z

Problem

polygons_to_geohashes_handbrake had no callers outside benches/bench.rs, where it served as the "oldfunc" comparison arm. It was also broken: a cell failing the envelope test was recorded in neither the inner nor the outer set and was not expanded from, so every neighbour that reached it queued it again — up to eight times the work.

Fix

Drop the function and its four benchmark arms rather than fix a variant nothing calls. That leaves several imports unused outside the test module: VecDeque and geohash::neighbors are no longer needed at all in the library, Centroid was the last thing the handbrake used, and Contains is now only reached by the brute-force oracle, so it moves into mod tests.

Also

expand_geohash_mapping_arrow reported a null geohash as "all geohashes in a group must have the same precision", because a null reads back as an empty string and so fails the length check. Detect the null first and name its position.

mailletf · 2026-08-25T00:43:48Z

This change is part of the following stack:

fix: don't drop multipolygon parts whose seed lands in an earlier part #30
- perf: cover polygons by descending the geohash tree #31
  - perf: expand geohash sets on packed integers instead of strings #32
    - fix: cap the expansion hop count instead of hanging near the poles #33
      - fix: release the GIL while covering a polygon #34
        
        chore: delete the dead handbrake cover #35 ◀
        
        chore: cargo fmt #36
        
        perf: add Arrow-native WKB and EWKB output #37
        
        perf: add Arrow-native encode and coordinate decode #38
        
        perf: optionally dictionary-encode geog_id in the Arrow mapping output #39
        perf: cover large polygons across Rayon workers #40

_{Change managed by git-spice.}

Copilot

Pull request overview

This PR removes an unused/broken legacy geohash expansion variant and its benchmark coverage, cleans up now-unused imports by scoping them to tests, and improves Arrow-based mapping expansion by reporting null geohashes with a targeted error message (covered by a new Python test).

Changes:

Removed polygons_to_geohashes_handbrake and deleted its benchmark comparison arms.
Adjusted src/lib.rs imports so Contains/neighbors are test-only, and removed unused library imports.
Updated expand_geohash_mapping_arrow to detect null geohashes within list elements and report their position; added a regression test.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
tests/test_arrow_ops.py	Adds a regression test ensuring null geohashes in list entries are reported as nulls (not precision mismatches).
src/lib.rs	Removes the dead handbrake function/imports; adds null detection in Arrow expansion; scopes geometry traits and `neighbors` to tests.
benches/bench.rs	Removes benchmark arms that depended on the deleted handbrake function.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

mailletf · 2026-08-25T14:46:47Z


-pub fn polygons_to_geohashes_handbrake<PI>(
-    polygons: PI,
-    precision: usize,
-    inner: bool,
-) -> Result<HashSet<String>, GeohashError>
-where
-    PI: IntoIterator<Item = Polygon>,
-{
-    let mut inner_geohashes = HashSet::new();
-    let mut outer_geohashes = HashSet::new();
-
-    for polygon in polygons {
-        let envelope = polygon.bounding_rect().unwrap();
-
-        let centroid = polygon.centroid().unwrap();
-        let centroid_geohash = encode((centroid.x(), centroid.y()).into(), precision)?;
-
-        let mut testing_geohashes = VecDeque::new();
-        testing_geohashes.push_back(centroid_geohash);
-
-        while let Some(current_geohash) = testing_geohashes.pop_front() {
-            if inner_geohashes.contains(&current_geohash)
-                || outer_geohashes.contains(&current_geohash)
-            {
-                continue;
-            }
-
-            let rect_bbox = decode_bbox(&current_geohash)?;
-            let current_geohash_polygon = rect_bbox.to_polygon();
-
-            let condition = if inner {
-                envelope.contains(&rect_bbox)
-            } else {
-                envelope.intersects(&rect_bbox)
-            };
-            if !condition {
-                continue;
-            }
-
-            if inner {
-                if polygon.contains(&current_geohash_polygon) {
-                    inner_geohashes.insert(current_geohash.clone());
-                } else {
-                    outer_geohashes.insert(current_geohash.clone());
-                }
-            } else {
-                if polygon.intersects(&current_geohash_polygon) {
-                    inner_geohashes.insert(current_geohash.clone());
-                } else {
-                    outer_geohashes.insert(current_geohash.clone());
-                }
-            }
-
-            if let Ok(rez) = neighbors(&current_geohash) {
-                for neighbor in [rez.sw, rez.s, rez.se, rez.w, rez.e, rez.nw, rez.n, rez.ne] {
-                    if !inner_geohashes.contains(&neighbor) && !outer_geohashes.contains(&neighbor)
-                    {
-                        testing_geohashes.push_back(neighbor.to_string());
-                    }
-                }
-            }
-        }
-    }
-    Ok(inner_geohashes)
-}
-
 /// Walk a `__geo_interface__` coordinate ring (list of [x, y] pairs) into a LineString.


Intentional. The function was dead and broken — deleting it is the point of the PR — and there are no downstream Rust consumers of the rlib; the crate is consumed through the Python extension. A deprecated wrapper forwarding to polygons_to_geohashes would not preserve behaviour anyway, since the two do not agree. The version bump is handled at release time for the stack as a whole.

mailletf · 2026-08-25T14:46:48Z

+    // Used only by the brute-force oracle, which deliberately avoids the
+    // prepared-geometry path the library itself takes.
+    use geo::Contains;
+    // The crate's own neighbour lookup, kept as the oracle for ghbits.


Fixed in f5d7af3 — the comment now says the import is the geohash crate's lookup and that it is the oracle the ghbits::neighbors tests check against.

polygons_to_geohashes_handbrake had no callers outside benches/bench.rs, where it served as the "oldfunc" comparison arm. It was also broken: a cell failing the envelope test was recorded in neither the inner nor the outer set and was not expanded from, so every neighbour that reached it queued it again — up to eight times the work. Drop the function and its four benchmark arms rather than fix a variant nothing calls. That leaves several imports unused outside the test module. VecDeque and geohash::neighbors are no longer needed at all in the library; Centroid was the last thing the handbrake used; and Contains is now only reached by the brute-force oracle, so it moves into `mod tests` alongside the comment explaining why the oracle avoids the prepared-geometry path. Also: expand_geohash_mapping_arrow reported a null geohash as "all geohashes in a group must have the same precision", because a null reads back as an empty string and so fails the length check. Detect the null first and name its position. Squashed with: - docs: say which crate the neighbours oracle comes from Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

mailletf requested a lite review from Copilot August 25, 2026 00:51

Copilot started reviewing on behalf of mailletf August 25, 2026 00:52 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

mailletf force-pushed the chore/remove-dead-code branch from ae1ed30 to f5d7af3 Compare August 25, 2026 14:45

mailletf force-pushed the fix/release-gil-polygon branch from 86f9cb8 to 5ca65fb Compare August 25, 2026 14:45

mailletf force-pushed the fix/release-gil-polygon branch from 5ca65fb to 61106b2 Compare September 1, 2026 18:19

mailletf force-pushed the chore/remove-dead-code branch from f5d7af3 to 5d882aa Compare September 1, 2026 18:19

mailletf force-pushed the fix/release-gil-polygon branch from 61106b2 to 6d1578c Compare September 1, 2026 18:33

mailletf force-pushed the chore/remove-dead-code branch from 5d882aa to 2acbe28 Compare September 1, 2026 18:33

mailletf force-pushed the chore/remove-dead-code branch from 2acbe28 to db95577 Compare September 1, 2026 18:54

mailletf force-pushed the fix/release-gil-polygon branch from 6d1578c to 32ab316 Compare September 1, 2026 18:54

mailletf merged commit a3e3d69 into fix/release-gil-polygon Sep 16, 2026
32 checks passed

Navigation Menu

Repository navigation

chore: delete the dead handbrake cover - #35

chore: delete the dead handbrake cover#35
mailletf merged 1 commit into
fix/release-gil-polygonfrom
chore/remove-dead-code

mailletf commented Aug 25, 2026 •

edited

Loading

Uh oh!

mailletf commented Aug 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

mailletf Aug 25, 2026

Uh oh!

mailletf Aug 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mailletf commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Also

Uh oh!

mailletf commented Aug 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

mailletf Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

mailletf Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mailletf commented Aug 25, 2026 •

edited

Loading