mortie.arrow.from_wkbs: the pyarrow skin over the WKB batch (issue #163) - #167
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #167 +/- ##
==========================================
+ Coverage 95.43% 95.53% +0.10%
==========================================
Files 12 12
Lines 1644 1681 +37
==========================================
+ Hits 1569 1606 +37
Misses 75 75
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| ImportError | ||
| If pyarrow is not installed. | ||
| ValueError | ||
| Fail-fast naming the **lowest-index** offending blob, in the logical |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] The "lowest-index" guarantee stated here is false when a null and a malformed blob are both present: the null wins, whatever its index.
The null scan runs over the whole column inside _wkb_blobs_from_arrow, and that completes before the core parses blob 0 — so a null anywhere preempts a parse failure anywhere, including one at a lower index.
Reproduced on 0feac3b (w163 worktree, rebuilt with maturin develop --release, mortie.__file__ in the worktree):
b = corpus(30)
b[3] = truncated_blob() # bad blob at logical 3
col = pa.array(b[:20] + [None] + b[21:]) # null at logical 20
marrow.from_wkbs(col, order=6)
# ValueError: blob 20: null entry in WKB column; ...
core_from_wkbs(b[:20] + [b""] + b[21:], order=6) # same rows, null as b""
# ValueError: blob 3: truncated WKB: 1 rings declared at offset 9 with only 0 bytes leftSame across chunks (bad at 3 in chunk 0, null at 25 in chunk 2 → blob 25). The reverse order is fine (null 3 + bad 25 → blob 3), and the within-class ordering is correct everywhere I probed — I re-ran test_the_rust_chunk_boundary_does_not_renumber_an_arrow_column and the sliced-frame test green.
So what actually holds is "the lowest-index null, else the lowest-index blob the core rejects" — nulls are a separate, earlier pass over the full column. Two cheap ways out: (1) say so in this Raises entry ("nulls are refused before any blob is parsed, so a null preempts a lower-index malformed blob"), or (2) drop the eager scan and let the core report, which costs the null diagnosis this PR exists to add. (1) matches the PR's own honest-caveat posture.
Nothing pins the cross-class order today, which is why it slipped: every null test contains only nulls, every malformed test only malformed blobs.
There was a problem hiding this comment.
🤖 from Claude
Confirmed and documented rather than restructured — 5e7f823.
Reproduced exactly as written (null at 20 → blob 20: null entry; the same rows with the null as b"" through the core → blob 3: truncated WKB). The vectorised pre-pass is the right implementation — is_null() is one call per chunk, and making it per-blob to satisfy a flat "lowest index wins" claim would trade the vectorisation for a diagnostic nicety. Restructuring is not cheap either: dropping the scan costs the null diagnosis this PR exists to add, and keeping the diagnosis and the flat ordering would need per-blob null flags handed into rust_wkbs_coverage_mocs, i.e. a Rust signature change — well outside this PR.
So the contract now says what actually holds, in a Notes section on mortie.arrow.from_wkbs, framed as the same two-gate shape the core's own from_wkbs Notes already use for its input-contract pre-pass:
Two ordered gates, as on
mortie.from_wkbsitself: nulls are screened by a vectorised pre-pass over the whole column, and only then are the blobs parsed and covered. Each gate reports its own lowest-index offender, so a null preempts a malformed blob at a lower index — a null at row 20 is raised ahead of a truncated blob at row 3. Within each class the lowest index wins.
The Raises entry no longer claims "lowest-index" flatly and points at the Notes, and _wkb_blobs_from_arrow's Raises says the null scan is a whole-column pre-pass that fires before any blob is parsed.
Pinned so it cannot drift, which is the part that was missing:
test_a_null_preempts_a_lower_index_malformed_blob[flat|chunked]— your exact fixture (bad blob at 3, null at 20), assertingblob 20: null entryfrom the skin andblob 3:from the core on the same rows with the null spelledb"";test_the_lowest_index_wins_within_each_failure_class— null 3 + bad 25 →blob 3: null entry; bad 3 + bad 25 →blob 3:.
docs/coverage_methods.md picked up the one-line version too (ad6794c).
| base = 0 | ||
| for chunk in chunks: | ||
| large = pa.types.is_large_binary(chunk.type) | ||
| if not (large or pa.types.is_binary(chunk.type)): |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] A ChunkedArray with zero chunks is never type-checked, so a non-binary column is silently accepted and answered as empty instead of refused.
The type check lives inside the chunk loop, so it never runs when chunks is empty:
for t in (pa.float64(), pa.int64(), pa.string(), pa.binary(2)):
marrow.from_wkbs(pa.chunked_array([], type=t), order=6)
# -> (array([], dtype=uint64), array([0])) for all four; no TypeErrorThe one-chunk spellings do raise, so the two existing tests miss each other exactly here: test_a_non_binary_column_is_refused_by_type[chunked_double] uses one chunk, and test_an_empty_column_keeps_the_contract[no_chunks] uses the zero-chunk shape but only with pa.binary().
[low] because the input is degenerate and the answer is empty rather than wrong — but the documented contract is "refused by type", and a ChunkedArray carries .type whether or not it has chunks, so the check can be hoisted to the column and the per-chunk one dropped (chunks of a ChunkedArray are all its .type by construction).
There was a problem hiding this comment.
🤖 from Claude
Fixed — 158d9ca. Reproduced first: all four of pa.chunked_array([], type=t) for float64 / int64 / string / binary(2) came back (array([], dtype=uint64), array([0])) with no TypeError.
The check is hoisted to the column exactly as you suggest, and the per-chunk one is gone:
# Checked on the column, not per chunk: a ``ChunkedArray`` carries its
# ``.type`` with zero chunks (where a per-chunk check never runs and a
# non-binary column would be answered as empty), and every chunk is that
# type by construction.
large = pa.types.is_large_binary(column.type)
if not (large or pa.types.is_binary(column.type)):
raise TypeError(...)New test for the crossed case the two existing shapes missed — test_a_non_binary_column_with_zero_chunks_is_refused_by_type, parametrized over your four types, asserting len(column.chunks) == 0 and column.type == type_ first so the shape itself stays pinned. test_an_empty_column_keeps_the_contract[no_chunks] still passes on the binary spelling, and [chunked_double] still refuses.
One follow-on the hoist made free: test_a_chunked_geoarrow_extension_column_is_unwrapped now also covers a zero-chunk extension column, which type-checks off the column's .type for the same reason.
| if not (large or pa.types.is_binary(chunk.type)): | ||
| raise TypeError( | ||
| "WKB column must hold binary or large_binary values; got " | ||
| f"{chunk.type}" |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] Open question (2): refusing an extension-over-binary column breaks this PR's own target column under a plausible install, and the stated blocker — "untestable without adding a test dependency" — is false.
The ATL03 column carries ARROW:extension:name = geoarrow.wkb in its field metadata (confirmed reading data/atl03_v007/atl03_v007_full.parquet directly). It arrives as plain binary only because nothing in the process registered the extension. The moment anything imports geoarrow-pyarrow — a normal thing for a geoparquet consumer to end up with transitively — the same file reads back as an ExtensionArray and this raise fires:
TypeError: WKB column must hold binary or large_binary values; got extension<geoarrow.wkb<...>>
i.e. englacial/zagg#400 breaks on an unrelated import, with a message that does not say what to do about it.
And it is testable with no new dependency — pa.ExtensionType is in pyarrow itself:
class WkbType(pa.ExtensionType):
def __init__(self):
super().__init__(pa.binary(), "geoarrow.wkb")
def __arrow_ext_serialize__(self):
return b""
@classmethod
def __arrow_ext_deserialize__(cls, storage_type, serialized):
return WkbType()
ext = pa.ExtensionArray.from_storage(WkbType(), pa.array(corpus(3)))
marrow.from_wkbs(ext.storage, order=6) # verified == core(corpus(3), order=6)
marrow.from_wkbs(ext, order=6) # TypeError todayThat is the whole fixture, and I ran it on 0feac3b: .storage round-trips byte-identically through the skin, so the unwrap is provably correct rather than speculative.
Recommend unwrapping when pa.types.is_binary/large_binary(chunk.type.storage_type), with that fixture as the test. If it stays refused, the message should at least name the escape hatch (... ; unwrap a geoarrow extension column with .storage) so the failure is self-solving.
There was a problem hiding this comment.
🤖 from Claude
You are right on both halves, and the unwrap is implemented — 87b1d63 (docs in ad6794c). The stated blocker was false: pa.ExtensionType is pyarrow's own, so the fixture costs nothing, and open question (2) is rewritten in the PR body to record that rather than leave the wrong reason standing.
The unwrap, in _wkb_blobs_from_arrow alongside the hoisted type check:
col_type = column.type
extension = isinstance(col_type, pa.BaseExtensionType)
if extension:
col_type = col_type.storage_type
if not (pa.types.is_binary(col_type) or pa.types.is_large_binary(col_type)):
raise TypeError(
"WKB column must hold binary or large_binary values; got the "
f"extension type {column.type.extension_name} over {col_type}"
)and chunk = chunk.storage at the top of the chunk loop, so a sliced extension array keeps its own offset (verified: ext.slice(4, 3).storage reports offset == 4, len == 3) and trap 2 survives the unwrap unchanged.
pa.BaseExtensionType rather than pa.ExtensionType deliberately: it catches C++-side canonical extension types (pa.uuid(), pa.json_()) as well as the Python-subclassed ones geoarrow-pyarrow registers, so a non-binary extension is refused by name in both worlds instead of falling through to chunk.buffers().
Tests — no new dependency, a module-level _WkbExtType(pa.ExtensionType) standing in for geoarrow.wkb:
test_a_geoarrow_extension_column_is_unwrapped_to_its_storage[binary|large_binary]— parity with the core, and byte-identical tofrom_wkbs(column.storage, ...);test_a_chunked_geoarrow_extension_column_is_unwrapped— the shape a geoparquet read actually produces (chunked and typed), plus the zero-chunk extension column;test_a_sliced_geoarrow_extension_column_reads_its_own_rows;test_a_null_in_a_geoarrow_extension_column_is_still_refused_by_index— the null pre-pass is not lost to the unwrap;test_an_extension_over_non_binary_storage_is_refused_by_its_name—geoarrow.pointoverint64→TypeError: ... got the extension type geoarrow.point over int64.
The englacial/zagg#400 caveat is recorded in the PR body and is now moot on this branch: the same file covers the same whether or not geoarrow-pyarrow is registered.
| :func:`mortie.arrow.from_wkbs` (issue #163) — and that is what to call: | ||
| ``marrow.from_wkbs(column, order=...)`` returns exactly this pair. Do | ||
| not improvise the extraction. Four traps sit between a column and its | ||
| blobs, each of which yields wrong data or silently-empty geometries |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] This rewritten paragraph still claims all four traps yield "wrong data or silently-empty geometries rather than an error" — the PR body's own honest caveat says trap 4 does not, and the code confirms it.
Measured on 0feac3b:
core_from_wkbs([blob, b"", blob], order=6)
# ValueError: blob 1: truncated WKB: 1 more byte(s) needed at offset 0, 0 remainIdentical with tolerance=0.5 and with max_cells=8, and the scalar from_wkb(b"", order=6, ...) raises the same for every moc / tolerance / max_cells combination I tried. A zero-byte blob can never become a cover, so in this function's contract a null-as-empty-blob is a misleading diagnosis, not a silent wrong answer — which is precisely what the PR body says the change should not claim.
Three shipped surfaces still claim it, and this one is the public docstring mkdocstrings renders:
mortie/geometry.py:725(this line) — "yields wrong data or silently-empty geometries rather than an error", with the null trap listed at line 730 ("arrives as an empty blob instead of being refused");mortie/arrow.py:430— "The four ways a hand-rolled version of this goes silently wrong";mortie/tests/test_arrow_wkb_batch.py:6— "four separate traps, each of which yields wrong data with no error".
docs/coverage_methods.md already gets this right — it names only offset / chunk boundaries / offset width as the silent three and lists nulls separately. The same split works here: three traps silent, the fourth a wrong diagnosis. Worth fixing because this paragraph is the one phase 2 deliberately rewrote to be accurate post-merge.
There was a problem hiding this comment.
🤖 from Claude
Fixed on all three surfaces — 9ce19af — matching docs/coverage_methods.md's split (three silent, the fourth a wrong diagnosis).
mortie/geometry.py (this paragraph, the one mkdocstrings renders):
Four traps sit between a column and its blobs. Three are silent — they yield different, valid-looking data rather than an error: … The fourth is a wrong diagnosis: a null entry spans zero bytes, so it arrives as an empty blob and this function reports it as a truncated geometry rather than as a missing one.
mortie/arrow.py — _wkb_blobs_from_arrow's list is now "the first three are silent … the fourth is a wrong diagnosis rather than wrong data", and from_wkbs's summary no longer sweeps the null into "different data with no error"; it names it separately as "an empty blob reported as a truncated geometry rather than as a missing one".
mortie/tests/test_arrow_wkb_batch.py:6 — "the first three of which yield wrong data with no error, the fourth a wrong diagnosis", with item 4 restated as "sails through as an empty blob and the core reports it as a truncated geometry, not as a missing one".
docs/coverage_methods.md was already right and is untouched on this point (ad6794c only adds the geoarrow and pre-pass sentences from the other two threads).
…/163-arrow-from-wkbs
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
🤖 from Claude Un-stacked and ready. PR #158 merged, its branch was deleted, and this PR is now based on The un-stack was done by merging Only this PR's work; no #158 content reintroduced. Resolution rule: #158-owned files took One flagged trap turned out not to be one: Post-merge verification: the four traps and the geoarrow |
1 participant
Closes #163
What
mortie.arrow.from_wkbs(column, order=..., ...)— the pyarrow skin over the WKBbatch. A geoparquet / STAC geometry column goes in as it comes off the file
(
binaryorlarge_binary, chunked or not, sliced or not) and the core's ragged(values, out_offsets)pair comes back, every scalar parameter forwardedunchanged. It is #154's core/skin split: lazily imported and pyarrow-gated
exactly like
mortie.arrow.polygons_to_morton_mocs, so a numpy-only install isuntouched and the
arro3-no-pyarrowleg is unaffected (that leg runs onlytest_arrow_cdata.py, and nothing added here imports pyarrow at module scope).This is a correctness change, not a performance one. As the
rescope comment
recorded, the memory argument evaporated when #158's phase-3 fold byte-capped the
chunk: arrow buffer slices already measure 1.07× there. What is left — and what
the follow-up demonstrated
— is that hand-rolling the extraction is a footgun with four traps, each of which
returns wrong data or a wrong diagnosis rather than an error.
Approach
One private helper,
_wkb_blobs_from_arrow, turns a column into one zero-copymemoryviewper row and hands that list to the core. There is no secondassembly path and no second memory posture: the core's byte-capped chunk loop
(2048 blobs / 64 MiB,
e6621f4+2774d82) does all the work, so the peak boundand the fail-fast ordering are the core's, unchanged.
The four traps, and where each is handled
ChunkedArrayhas no.buffers()— and that is the default shape, since a parquet column reads back chunked (the ATL03 column comes back as 90 chunks)hasattr(col, "buffers")isFalse; the recipe's first line simply does not existcol.chunksin order. Notcombine_chunks(), which copies the whole column and defeats the pointbuffers()are the original array'sslice()/takeare zero-copy metadata —pa.array([b"AAA",b"BBBB",b"CCCCC",b"DDDDDD"]).slice(2,2)reads back as[b"AAA", b"BBBB"]with no error. This is live for englacial/zagg:filter_bboxgoes throughtable.takechunk.offset + i, per chunk (each chunk carries its own offset)large_binaryoffsets areint64, plainbinary's areint32pa.types.is_large_binary[b"AA", None, b"BBB"]has offsets[0, 2, 2, 5]Fail-fast frame
Indices are the logical column's, always. The skin flattens the chunks into
one sequence in column order before the core sees them, so a malformed blob in
the third arrow chunk reports its column index, and the Rust chunk loop (which
chunks on its own, unrelated boundaries) does not renumber it either. A slice is
its own column: an offender at logical row 3 of a slice reports
blob 3.The null decision — refuse, with the index named
ValueError: blob 23: null entry in WKB column; a null is the absence of a geometry, not an empty one, and covering nothing is not a cover. This followsthe issue's recommendation and the core's fail-fast posture, and matches how
_ragged_from_arrowalready treats a null polygon (polygon 3: null polygon in batch).Honest caveat on this one. Unlike traps 1–3, trap 4 is not a silent-wrong-data
bug in the WKB context: a zero-byte blob cannot parse, so without the guard the
core raises
blob 23: truncated WKB: 1 more byte(s) needed at offset 0, 0 remain— the right index, but a misleading diagnosis (a corrupt geometry, when what
happened is that the row had no geometry). The guard converts a wrong diagnosis
into the right one. The silent-empty-geometry framing in the issue holds for
consumers doing something other than parsing; it does not hold here, and the PR
should not claim it does.
Testing
mortie/tests/test_arrow_wkb_batch.py(534 lines, 83 tests, all green;full suite 1334 passed, 16 skipped on the merged tip).
Each trap has a test that fails without its guard — verified by reverting the
guard locally, watching the test fail, and restoring:
ChunkedArraybranch removedtest_a_chunked_column_has_no_buffers_and_is_walked_chunk_by_chunkchunk.offset→0test_a_sliced_column_reads_its_own_rows_not_the_first_ones,test_a_sliced_and_chunked_column_offsets_every_chunk,test_a_malformed_blob_in_a_sliced_column_uses_the_slice_frameint32test_large_binary_offsets_are_read_as_int64and everylarge_binarydialect casetest_a_null_spans_zero_bytes_and_is_refused_by_index,test_a_null_is_named_in_the_column_frame_not_the_chunk_frame,test_a_null_in_a_sliced_chunk_is_named_by_its_logical_indexThe trap tests pin the trap itself as well as the fix, so they stay honest if
pyarrow changes:
assert not hasattr(chunked, "buffers"), the naive offset-blindrecipe asserted to yield
[b"AAA", b"BBBB"],int64vsint32reads assertedunequal, and the
[0, 2, 2, 5]null offsets asserted directly.Shape matrix:
BinaryArray,LargeBinaryArray,ChunkedArray(including areal parquet round-trip, which is where the chunked shape comes from), sliced,
sliced-and-chunked (each chunk with its own offset), zero-length chunks
interleaved between populated ones, single-element, and five spellings of empty
(
binary,large_binary, one empty chunk, zero chunks, a zero-length slice).Parity: per-blob against the scalar
from_wkb(..., moc=True); whole-columnagainst the core fed the same blobs. Plus the dialect fixtures the ATL03 corpus
lacks — holes, multipart, antimeridian, south and north pole, Z, M — each ×
both byte orders ×
binary/large_binary(32 cases), and a mixed-endianness +EWKB/SRID column.
Forwarding:
order(checked at both range ends),tolerance,max_cells(including the budget warning still naming
blob 0), andnormalize— the lastasserted to actually change the cover, so the parameter cannot be dropped and
still pass.
Refusals: non-binary Arrow columns by type (
string,int64,fixed_size_binary, chunkeddouble) and non-Arrow inputs by name.Real-corpus parity
The 555,867-blob ATL03 v007 WKB column
(
data/atl03_v007/atl03_v007_full.parquet, 276.7 MiB of WKB, 90 chunks), atorder 6: the skin's
valuesandoffsetsare byte-identical to the core fedcolumn.to_pylist()—sha256(values)[:16] = f17ca97bba7fe963,sha256(offsets)[:16] = a15cfc3a6ca66a31, identical across all three paths(skin /
to_pylist/ the hand-rolled offset-correctedmemoryviewrecipe).Memory — measured, and the headline claim confirmed with a correction
The claim to check was: this removes the ~305 MB Python-
bytesterm(englacial/zagg#408) while holding the core's ~1.07× peak.
Confirmed on the first half, with a term the claim omitted. Two independent
measurements agree; the second (adversarial review, same ATL03 column, order 6)
is the one quoted here because it is the tighter method —
tracemallocoff, andpeak sampled rather than read off
ru_maxrss:to_pylist()mortie.arrow.from_wkbsmemoryviewrecipeto_pylist()term is the ~305 MB / ~322 MB the issue andRust WKB reader: backend-free geometry ingest, plus from_wkbs (issue #157) #158's own docstring name (276.7 MiB of payload + ~17.5 MiB of
bytesheaders + 4.4 MiB of list). It is gone.
exactly:
sys.getsizeof(memoryview) = 184 B × 555,867 = 97.5 MiB, plus4.5 MiB of list. The issue's rescope comment already flagged these as "cheap
each, not free".
to_pylistterm and −41%end-to-end (487.1 → 286.9 MiB). Anyone quoting "removes 305 MB" should
quote "removes 305 MB and adds back 108 MB" instead.
costs ~1.5 MiB over doing it by hand correctly — that is the price of the
four guards, and it is noise against 198.7 MiB.
The cover-phase multiplier is 1.07×, not the 1.10× first reported here. A
5 ms RSS sampler gives 1.07× for the skin,
to_pylistand the recipe alike— the same figure the core publishes for arrow buffer slices, so the chunk
bound is intact and the earlier 1.10× was measurement noise. The sampler also
confirms the "0.62× is an artifact" diagnosis:
ru_maxrssis a process-lifetimehigh-water mark and simply cannot see growth underneath an earlier peak, where
the sampler can.
Methodology note, worth keeping. The review's first pass read +363.6 /
+164.9 MiB for the two paths — inflated because
tracemalloc's ownbookkeeping added ~57 MiB over 555,867 traced allocations. Measure this column
with tracing off; the numbers above are all tracing-off.
Bench — no throughput win, and that is expected
Same column, order 6, five repeats for the build step:
to_pylist()→ coremortie.arrow.from_wkbsThe build step is ~17% faster and is ~3% of the call; the cover dominates and is
byte-for-byte the same work. Read honestly: there is no speed win here, and
the full-call spread is run-to-run noise on a loaded machine, not a regression.
The win is the 200 MiB and the four traps. As the issue's rescope states, the
per-chunk byte copy stays regardless — releasing the GIL needs owned bytes.
Docs
mortie.arrow.from_wkbsadded todocs/api/arrow.mdand described indocs/coverage_methods.md.70b021d(on the base branch)ships a paragraph saying "mortie has no typed entry point for it yet (tracked
as mortie.arrow.from_wkbs: arrow skin over the WKB batch, so a geoparquet column skips Python bytes objects #163) … Until then the correct call is
from_wkbs(column.to_pylist(), ...)". That becomes false the moment thismerges, so it now points at
mortie.arrow.from_wkbsand keeps the four trapsas the reason, with the ~322 MB / ~112 MB comparison in place of the
one-sided figure.
Phases
_wkb_blobs_from_arrow+mortie.arrow.from_wkbs,the four traps, the 70-test module, and the docs pages (
5b48bd6)(
0feac3b)158d9ca,87b1d63,5e7f823,9ce19af,ad6794c)Review fold
5e7f823). The null pre-pass is one vectorisedis_null()per chunk; making it per-blob to satisfy a flat claim would trade the vectorisation for a diagnostic nicety, and keeping both the null diagnosis and the flat ordering needs per-blob null flags insiderust_wkbs_coverage_mocs— a Rust signature change.mortie.arrow.from_wkbsnow carries aNotessection stating the real contract (two ordered gates; a null preempts a lower-index malformed blob; within each class the lowest index wins), and two new tests pin the cross-class order and the within-class order so it cannot driftChunkedArrayis never type-checked158d9ca): the declared type is checked on the column, before the chunk loop; the per-chunk check is gone. Newtest_a_non_binary_column_with_zero_chunks_is_refused_by_type, parametrized overfloat64/int64/string/binary(2)— the crossed case the two existing tests missed87b1d63, docsad6794c) — see the rewritten open question (2) below9ce19af) inmortie/geometry.py,mortie/arrow.pyand the test module docstring, matchingdocs/coverage_methods.md's split: three silent, the fourth a wrong diagnosisUn-stacking (
06b2d78)The branch picked its base up by merging throughout, never rebasing — the
fold commits' parent was already pushed, and rebasing published commits would
need a force push, which this repo forbids outright. Two such merges are in the
history:
5ab6d38took the base tip (5ca54d4, #157 phase 4) while #158 wasopen, and
06b2d78takesmainafter #158 landed.06b2d78conflicted on six files, all of them the branch's pre-#158 copy of afile #158 owns. Resolution rule: #158-owned files take
main's versionwholesale —
mortie/tests/test_wkb_basins.py,test_wkb_batch_memory.pyandtest_wkb_reader.py(add/add) andmortie/__init__.py, none of which this PRtouches;
mortie/geometry.pyanddocs/coverage_methods.mdstart frommainand re-apply only this PR's edits (
0feac3b,9ce19af,ad6794c) on top.mortie/arrow.pyandmortie/tests/test_arrow_wkb_batch.pyare byte-identicalto what the branch had.
git diff origin/main...HEAD --name-onlyis now exactlydocs/api/arrow.md,docs/coverage_methods.md,mortie/arrow.py,mortie/geometry.py,mortie/tests/test_arrow_wkb_batch.py— no #158 contentin the diff, and
src_rust/is byte-identical tomain.Gates
Re-run on the merged tip
06b2d78:cargo fmt --checkclean;cargo clippy8 warnings, all pre-existing —
src_rust/is byte-for-bytemain's, sonone are new;
cargo test --lib317 passed, 1 ignored;pytest1334passed / 16 skipped (the growth over the 1258 reported earlier is #158's
phases 4–5 arriving with
main, not new tests here);flake8 mortie --count --select=E9,F63,F7,F820, and the non-blocking--max-line-length=88pass reports nothing new (see below);numpydoc lint mortie/*.pyclean. No Rust was changed by this PR.The four trap guards and the geoarrow unwrap were re-checked explicitly after
the merge: the 34 tests covering
ChunkedArray-has-no-.buffers(),chunk.offset,large_binary/int64offsets, null-refused-by-index and theextension-storage unwrap all pass, as does the full 83-test module.
flake8 --max-line-length=88(non-blocking) reports two findings inmortie/arrow.py—364:89 E501and65:1 C901 '_build_type' is too complex (11)— and both are already onmainat the identical lines (0ad6f9f,"fold review: document the batch entry points", issue #153). This PR adds zero
style findings. Left alone per the "don't fix unrelated pre-existing failures"
rule; happy to reflow the E501 if wanted.
Module sizes:
mortie/arrow.py555 → 770 (aim ~1,000, fine);mortie/tests/test_arrow_wkb_batch.py534 (new);mortie/geometry.py1,661 → 1,664 (+3, all docstring prose) — see the question below.
Questions for review
geometry.pyis at 1,664 lines, well past this repo's ~1,000 aim. I didnot push it there — it arrives that way from Rust WKB reader: backend-free geometry ingest, plus from_wkbs (issue #157) #158, now on
main— and thisPR changes it by +3 lines of docstring. Flagging rather than acting: does
that want a split issue against Parse WKB in Rust: backend-free geometry ingest, plus the plural from_wkbs batch #157/Rust WKB reader: backend-free geometry ingest, plus from_wkbs (issue #157) #158, or is it already known?
geoarrow-typed columns — resolved, and the original reason was wrong.
untestable without adding a test dependency:pa.ExtensionTypeships inpyarrow itself, so the fixture is a dozen lines and no new dependency. The
review demonstrated the risk concretely — the ATL03 column carries
ARROW:extension:name = geoarrow.wkbin its field metadata and reads back asplain
binaryonly because nothing registered the extension, so themoment any transitive dependency pulls in
geoarrow-pyarrow, the same filewould have started raising
TypeErrorat batch mortie coverage in ShardMap.build (issue #396) englacial/zagg#400._wkb_blobs_from_arrownow unwraps an extension type whose storage isbinary/large_binaryto that storage, and refuses any other extension byname (
... got the extension type geoarrow.point over int64). The detectionuses
pa.BaseExtensionType, so it catches pyarrow's own canonical extensiontypes as well as the Python-registered ones. Tested with a locally defined
pa.ExtensionTypeacross the flat, chunked, zero-chunk, sliced, null andnon-binary-storage cases (
87b1d63).zagg adoption caveat, now moot on this branch. Before the unwrap,
batch mortie coverage in ShardMap.build (issue #396) englacial/zagg#400 would have had to either pin the absence of a geoarrow
registration or defensively call
col.storage if hasattr(col.type, "storage_type") else colat the boundary.With
87b1d63neither is needed: the same file covers the same whether ornot geoarrow is installed.
The 106.6 MiB of
memoryviewobjects is the last removable term, and itcannot go without a Rust binding that takes
(values_buffer, offsets)directly instead of
Vec<Bound<PyAny>>—rust_wkbs_coverage_mocsmaterializesevery entry as a Python object regardless of how lazily the sequence produces
them, so no Python-side cleverness helps. Worth a follow-up issue on mocs_to_orders: ragged batch moc_to_order — plus an API sweep for bulk-by-default operators #156's
phase-5 roster, or is 200 MiB of the 307 enough?
binary_view/large_binary_view(pyarrow ≥ 16) have a different physicallayout and are refused by type here. Deliberate — a view array is a different
extraction problem, and no producer in this stack emits one. Confirm that is
the right line?