Repository navigation
perf: add Arrow-native WKB and EWKB output - #37
Conversation
|
This change is part of the following stack:
Change managed by git-spice. |
There was a problem hiding this comment.
Pull request overview
This PR adds Arrow-native bulk decoding APIs to output fixed-width WKB/EWKB polygon bounding boxes as Arrow LargeBinary, minimizing Python boundary overhead for large columns.
Changes:
- Added
decode_many_to_wkb_arrow/decode_many_to_ewkb_arrowPyO3 entry points that acceptUtf8orLargeUtf8Arrow arrays and produceLargeBinaryoutput. - Refactored bbox serialization into an in-place writer (
write_bbox) and reused it for the existing list-based API viaserialize_bbox. - Added Rust + Python tests and updated Python type stubs to cover the new Arrow APIs.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib.rs |
Implements Arrow string-column abstraction, fixed-width parallel WKB/EWKB column builder, new PyO3 functions, and in-place bbox writer. |
tests/test_arrow_ops.py |
Adds PyArrow-focused tests ensuring Arrow output matches the list API, preserves nulls, supports slicing, and validates inputs. |
geohash_polygon/__init__.pyi |
Adds stubs/docs for the new Arrow decoding functions. |
Cargo.toml |
Adds arrow-buffer dependency to construct buffers/offsets/nulls for Arrow output. |
Cargo.lock |
Locks the new arrow-buffer dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let width = wkb_width(srid); | ||
| let rows = hashes.len(); | ||
| let mut values = vec![0u8; rows * width]; |
There was a problem hiding this comment.
Not fixing this one. width is 93 or 97, so rows * width needs rows above ~1.9e17 to wrap a 64-bit usize — an Arrow array of that length cannot exist, since its own offset buffer would already be orders of magnitude past addressable memory. The vec! below would OOM long before arithmetic became the problem.
| put(&x.to_le_bytes()); | ||
| put(&y.to_le_bytes()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Agreed, added in de54668 (now 0dcf5a8). write_bbox asserts its final write position against out.len(), so adding a field without bumping wkb_width fails a test instead of silently truncating or bleeding into the next row's slot.
bad9038 to
3b99627
Compare
24025e5 to
0dcf5a8
Compare
3b99627 to
7662f4f
Compare
1554d79 to
98b8997
Compare
7662f4f to
0f68dc4
Compare
For 500k precision-7 geohashes, the pure Rust core of decode_many_to_wkb runs in under 4 ms, while the function called from Python takes 45 ms. Over 90% of the wall time is the boundary: converting list[str] into Vec<String> on the way in, and building half a million Python bytes objects on the way out. That is also why the num_threads knob does nothing here — it parallelises the 4 ms. Add decode_many_to_wkb_arrow and decode_many_to_ewkb_arrow, following expand_geohash_mapping_arrow. Strings are read straight from the input's Arrow buffers and the output is built into Arrow buffers, so no Python object exists per row. N = 500,000, medians of 15 runs decode_many_to_wkb (list -> list[bytes]) 45.0 ms decode_many_to_wkb_arrow (arrow -> arrow) 3.1 ms 14.5x decode_many_to_ewkb (list -> list[bytes]) 51.8 ms decode_many_to_ewkb_arrow (arrow -> arrow) 3.2 ms 16.2x Even when the data starts as a Python list and has to be converted, list -> pa.array -> wkb_arrow is 10.5 ms, still 4.3x. Output is byte-identical to the list API in every case. Every row serializes to the same width — 93 bytes for WKB, 97 for EWKB — so the values buffer is allocated once for the whole column and filled across threads in place via par_chunks_exact_mut, with offsets computed from the row width. That removes the per-row Vec the list API still pays. Supporting changes: - serialize_bbox now wraps write_bbox, which writes into a caller-provided slice. The allocating form stays for the existing list API. - StringColumn accepts either Utf8 or LargeUtf8 input, so callers do not have to match the offset width the library happens to prefer. - Null inputs produce null outputs rather than being rejected or silently decoded as an empty string. The list-returning functions are untouched. Squashed with: - test: assert write_bbox fills its slot exactly - test: put the n_hops_for tests back under their section Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0f68dc4 to
2633fa6
Compare
98b8997 to
3e7745c
Compare
Problem
For 500k precision-7 geohashes, the pure Rust core of
decode_many_to_wkbruns in under 4 ms, while the function called from Python takes 45 ms. Over 90% of the wall time is the boundary:list[str]→Vec<String>on the way in, and half a million Pythonbytesobjects on the way out.That is also why the
num_threadsknob does nothing here — it parallelises the 4 ms.Fix
Add
decode_many_to_wkb_arrowanddecode_many_to_ewkb_arrow, following the existingexpand_geohash_mapping_arrow. Strings are read straight from the input's Arrow buffers and the output is built into Arrow buffers, so no Python object exists per row.Every row serializes to the same width — 93 bytes for WKB, 97 for EWKB — so the values buffer is allocated once for the whole column and filled across threads in place via
par_chunks_exact_mut, with offsets computed from the row width. That removes the per-rowVecthe list API still pays.Benchmarks
N = 500,000, medians of 15 runs:
decode_many_to_wkbdecode_many_to_ewkbEven when the data starts as a Python list and has to be converted,
list → pa.array → wkb_arrowis 10.5 ms — still 4.3×. Output is byte-identical to the list API in every case.Notes
Utf8orLargeUtf8, so callers need not match whichever offset width the library happens to prefer.serialize_bboxnow wrapswrite_bbox, which writes into a caller-provided slice. The allocating form stays for the existing list API.