Repository navigation
perf: optionally dictionary-encode geog_id in the Arrow mapping output - #39
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 an optional dictionary_geog_id flag to expand_geohash_mapping_arrow to reduce Arrow output size by dictionary-encoding the geog_id column while preserving the existing default schema for current callers.
Changes:
- Added
dictionary_geog_id(defaultfalse) to optionally emitgeog_idasDictionary<Int32, Utf8>in the ArrowRecordBatchoutput. - Refactored the parallel expansion path to avoid threading
geog_idthrough Rayon work items (re-associating via preserved result order). - Added Python tests and updated the Python type stub to cover/describe the new optional argument and schema behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/lib.rs |
Implements optional dictionary encoding for geog_id and refactors parallel expansion result handling. |
tests/test_arrow_ops.py |
Adds test coverage for dictionary-encoded geog_id equivalence, schema typing, size reduction, and edge cases. |
geohash_polygon/__init__.pyi |
Updates the Python stub signature and docstring to include dictionary_geog_id. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let mut keys: Vec<i32> = Vec::with_capacity(total_out); | ||
| for (i, group) in expanded.iter().enumerate() { | ||
| keys.extend(std::iter::repeat_n(i as i32, group.len())); | ||
| } |
There was a problem hiding this comment.
Agreed, fixed in bbd2a02. expand_geohash_mapping_arrow now checks the group count against i32::MAX before building the keys and raises a ValueError naming the limit and the way out (dictionary_geog_id=False, or split the input). A wrapped negative key would silently repoint rows at the wrong geog_id, which is the worst kind of failure here.
07381ea to
f53ff32
Compare
e3e88de to
bbd2a02
Compare
bbd2a02 to
d532897
Compare
a33d93a to
d27b1ce
Compare
71f3a5b to
cae83cd
Compare
d27b1ce to
f43b43f
Compare
cae83cd to
3cfb8b4
Compare
expand_geohash_mapping_arrow emits one row per (geog_id, geohash) pair, so every row of a group repeats that group's id. The geog_id column is almost entirely duplicate bytes. Add dictionary_geog_id, which emits Dictionary<Int32, Utf8> instead: one copy of each id in the dictionary, 4 bytes of index per row. On 20,000 geographies of 300 cells each — 6M rows, with short 11-character ids: geog_id column 114.0 MB -> 24.3 MB 4.7x whole batch 204.0 MB -> 114.3 MB 1.8x call 362 ms -> 294 ms 1.2x Longer ids widen the gap; the dictionary column's size does not depend on the id length at all beyond the one stored copy. Defaults to false, so the existing schema is unchanged for current callers. Run-end encoding would compress this far harder still — the rows are already grouped, so 6M rows would reduce to 20,000 runs — but support outside pyarrow is patchier than for dictionaries, so it is not offered here. Also stops threading the geog_id String through the parallel expansion, which never needed it: results come back in input order, so results[i] belongs to geog_ids[i]. Both output paths are built from that. Squashed with: - fix: refuse a dictionary too large for its Int32 keys - fix: deduplicate the geog_id dictionary values Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3cfb8b4 to
b82dda3
Compare
Problem
expand_geohash_mapping_arrowemits one row per (geog_id, geohash) pair, so every row of a group repeats that group's id. Thegeog_idcolumn is almost entirely duplicate bytes.Fix
Add
dictionary_geog_id, which emitsDictionary<Int32, Utf8>instead: one copy of each id in the dictionary, 4 bytes of index per row.Defaults to
false, so the existing schema is unchanged for current callers.Benchmarks
20,000 geographies × 300 cells = 6M rows, with short 11-character ids:
geog_idcolumnLonger ids widen the gap — the dictionary column's size does not depend on id length beyond the one stored copy.
Considered
Run-end encoding would compress far harder still (the rows are already grouped, so 6M rows is really 20,000 runs), but support outside pyarrow is patchier than for dictionaries, so it is not offered here.
Also
Stops threading the
geog_idStringthrough the parallel expansion, which never needed it: results come back in input order, soresults[i]belongs togeog_ids[i].