Repository navigation
feat(enrichment): S4 (#89) — numpy/pyarrow keyed gather, drop pandas merge - #210
Merged
Merged
Conversation
Polichinel
force-pushed
the
feat/s4-pyarrow-keyed-gather
branch
from
August 3, 2026 00:16
d1f48c0 to
9065dd3
Compare
…das merge Rewrite GaulLookupEnricher's lookup side to be pandas-free (epic #85): - Read the 888 KB GAUL lookup with pyarrow; hold it as numpy arrays + a sorted key index. Attaching metadata is a keyed `searchsorted` gather, not a pandas merge — absent gids gather to None so the downstream null gate still fails loud. The artifact is no longer materialised as a pandas frame, and the join no longer needs it to carry pandas index metadata (which unblocks S5/#90's pyarrow-native builder). - pandas becomes a TYPE_CHECKING-only import (it's in the enricher's interface — callers hand it DataFrames — not its implementation). - test_enrichment: the `lookup` fixture loads the artifact directly (the enricher no longer exposes a private pandas frame), tolerant of both the current pandas-index shape and S5's index-free one. - test_doc_accuracy: the pandas-importer guard now checks by AST, asserting ZERO runtime importers + type-only in enrichment.py (a regex couldn't tell a TYPE_CHECKING guard from a real import). Recovered from an uncommitted working tree on this (correctly-named) branch — complete, its 31 tests pass. (The repo's 5 wire shard/sidecar byte-parity + toolchain-pin failures are pre-existing and unrelated to this change.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… what the dtype change costs
Four findings from the diff review, applied to the commit that preceded them.
An empty lookup raised IndexError from inside the gather. The guard read
`(len(self._keys) > 0) & (self._keys[clipped] == wanted)`, which LOOKS like a
guard and is not one: `&` evaluates both operands, so the index happened anyway.
Guard where the condition is knowable — __init__ now refuses an empty lookup with
a message about the lookup, rather than letting the delivery later complain about
missing metadata.
The parity proof passed check_dtype=False, check_categorical=False and compared
after reset_index — three flags that hide exactly what a merge-to-assign rewrite
is most likely to change. Re-checked without them. Two results worth having:
- name columns went category -> object. Measured, not guessed: on a 200-row
output, category is 1,795,191 bytes and object is 60,061, because a
categorical carries the artifact's full 64,742-entry dictionary whatever the
output size. Lighter for the frames this object actually sees. Recorded in the
code beside the C-32 note it might otherwise be read as contradicting.
- on EMPTY input the old merge replaced the input's RangeIndex with an
object-dtype Index; the gather leaves it alone. The only behavioural
difference found, and it is in the direction the CIC already claimed.
The CIC is corrected — fourth story running where it lagged the change, which is
the pattern ADR-014 §5 was written for, recurring in the first story after the ADR
landed. It now states the gather, the dtype change, the empty-lookup refusal, and
that pandas is interface-only.
Also dropped a dead alias and a duplicated row count.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…uards the claims needed
Six findings from /code-review max (five parallel reviewers), all reproduced before
fixing.
The blind `np.asarray(gids, dtype=np.int64)` was wrong in three ways, one of them a
regression against development:
- a NaN gid became INT64_MIN with only a RuntimeWarning, and then the warning line
itself raised `ValueError: cannot convert float NaN to integer` because it
iterated the PRE-cast array. The path whose job is to report the problem
gracefully was the path that crashed. A missing gid is ordinary data quality.
- a non-integral float truncated SILENTLY: 54220.000000001 -> 54220, matching a
real, different cell. Verified against the pandas merge this replaced — it
returns null there, and even warns. A silent wrong match is worse than a crash.
- pd.NA raised a bare TypeError with no log and no contract-shaped error.
Replaced by `_as_cell_ids`, which DECLARES what a cell id is and marks anything else
unusable rather than coercing it. Unusable ids gather to null exactly as unknown
gids do, so the downstream gate sees a hole rather than a guess. Verified case by
case against the old implementation: NaN, drifted float and object-with-None now
match it exactly, and pd.NA is strictly better (old crashed).
The lookup side had the same hole: a null key would take the same sentinel, collide
with an unusable query id, and report FOUND — handing back another cell's metadata.
That is the fabricated value this module's docstring forbids. __init__ now refuses
it.
The warning then named cell 0 for an unusable id — a cell nobody asked about. It now
names unknown cells and separately counts rows that carried no usable id, because an
id that is not an id has none to report.
The AST guard added in this same PR had the defect it was written to catch. It
walked the whole `if TYPE_CHECKING` node including `else:`, and substring-matched
the test, so `not TYPE_CHECKING` read as a guard. Both evasions reproduced, both now
caught, and pinned by a test rather than a comment — the `&`-not-short-circuiting
shape, committed twice in one pull request.
The CIC's guarantees were prose resting on a throwaway script. Committed: empty and
null-key refusals, the four unusable-id behaviours, row/order/index preservation
across five shapes plus a non-default index, and the object dtype. 29 tests.
ADR-012 updated — my own test docstring said "ADR-012 names the seam, so update it
rather than letting the claim drift", and the first draft did not. Three claims in
the CIC that PR #200 retired in the manager's CIC were still standing here.
Registered C-75: this class has no production caller and now holds a second copy of
the delivery path's keyed gather. Tier 3 — the verification path and the delivery
path implement the same algorithm twice, and the tests run through the copy that
does not ship.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng, prove the mask Round 1 removed one silent coercion and introduced two more. Round 2 found them. The object branch used `float(value)`, which PARSES. It accepted the string "54220" as a cell id — where the pandas merge this replaces raised "You are trying to merge on object and int64 column". A string gid column is a declaration error and the old path said so; parsing it is inference, and it is the drifted-float defect pointing the other way. It now accepts only values that already ARE integers. `bool` is excluded for the same reason: True is not cell 1. Neither branch bounded int64. `uint64` max wrapped to -1 and `1e30` to INT64_MIN, both flagged VALID — an id nobody wrote, marked usable. Harmless only because real gids are 1..259,200, which is luck of the key range rather than a property the code enforced. Both branches now bound explicitly. `& usable` in `_gather` was the one new guard that survived its own removal: unusable ids substitute to 0, and 0 is not a real gid, so the key comparison already failed. It looked like decoration. It is not, and the case is now committed: with a lookup containing gid 0, a NaN row silently receives cell 0's geography without the mask. That is the fabricated value the docstring forbids, produced from an input the caller never wrote. The out-of-range test I wrote in round 2 built every case with dtype=object, so all of them entered the element-wise branch and the int and float bounds it claimed to cover were never executed — removing them left the suite green. Each case now enters the branch it tests. Mutation matrix after: dropping `& usable`, the float bound, the int bound, or the isinstance check each fails the suite. `np.isfinite` is the exception and is now documented as such rather than left to be rediscovered: inf fails the upper bound, NaN fails `arr == rint(arr)`, so the check is subsumed and defensive, not load-bearing. Saying so is cheaper than the next person mutation-testing it and wondering. 37 tests in test_enrichment.py; 337 in the suite. Wire untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…LP, and my mutation proof was a replica Second /code-review max pass. Three correctness defects and a vacuous test, all reproduced before fixing. The float upper bound admitted exactly what it was added to exclude. _INT64_MAX is 2**63-1, which is odd and NOT representable in float64, so comparing a float against it promotes and rounds UP to 2**63. float(2**63) passed isfinite, integral and both bounds, then wrapped to INT64_MIN and was flagged valid. Now bounded strictly below 2.0**63, which IS exact. _INT64_MIN needed no change: -2**63 is a power of two. timedelta64 was accepted as a cell id, because isinstance(np.timedelta64(5,'ns'), np.integer) is True — a numpy quirk, and datetime64 is not, so the hole was specific to one type. A duration is not a cell id any more than True is cell 1. The test I wrote to prove the AST guard bites was itself a replica. It defined a local copy of the classifier and asserted against that; reintroducing the polarity bug into the REAL guard left all 16 tests green. That is the defect S4 of epic #181 retired from test_validation.py two days ago — 43 tests against a function the file defined itself — committed again by me, in the PR whose message boasts about catching its cousin. The classifier is now one module-level function with two callers, and both regressions now fail it. And the fix for that had a sting: routing the out-of-range cases through the public API, as the review recommended, LOST the proof. Publicly, "refused" and "accepted but wrapped to an absent id" both produce null, so reverting the float bound left the test green. The public assertion is the caller's guarantee; the mask assertion is the mutation proof; both are needed and both are now present, with a comment saying why. _gather returned four values, three of which existed only to build one log message. The warning moved inside it, matching this file's own convention where __init__ co-locates detection and logging at every guard. It returns one dict. My own error message credited the builder with refusing null keys. It does not: its null check runs after the key becomes the index, and DataFrame.isna() does not inspect an index. A null key is unreachable there only because an earlier astype('int64') raises — protection by accident. Registered as C-76, along with the empty-output case, which IS reachable and writes cells=0 without complaint. Also: the pandas error message was misquoted, ADR-012 cited a path that does not exist (unfao/wire/ -> contract/wire/), the CIC claimed eight input shapes where the pinned class has six and named one it does not exercise, the artifact size was wrong by 2% in four places, and a register quote was singular where C-40 is plural. C-75 gained the owner ADR-014 §4 requires and its first draft omitted while citing §4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Polichinel
force-pushed
the
feat/s4-pyarrow-keyed-gather
branch
from
August 3, 2026 01:35
dfaceb5 to
b406ad6
Compare
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #89 (epic #85, S4).
What
Rewrites
GaulLookupEnricher's lookup side to be pandas-free: read the GAUL lookup with pyarrow, hold it as numpy arrays + a sorted key index, and attach metadata by a keyedsearchsortedgather instead of a pandasmerge. Absent gids gather toNone, so the downstream null gate still fails loud. pandas drops to aTYPE_CHECKING-only import (it's in the enricher's interface — callers pass DataFrames — not its implementation). Unblocks S5 (#90)'s pyarrow-native builder.Tests
test_enrichment:lookupfixture loads the artifact directly (the enricher no longer exposes a private pandas frame), tolerant of both the current pandas-index shape and S5's index-free one.test_doc_accuracy: the pandas-importer guard now checks by AST (zero runtime importers + type-only inenrichment.py) — a regex couldn't distinguish aTYPE_CHECKINGguard from a real import.Provenance / honesty
This was recovered from an uncommitted working tree on this (correctly-named) branch — one of the parallel sessions implemented it and never committed. Reviewed end-to-end before committing: complete, coherent, on-topic for #89.
The repo currently has 5 failing tests in
test_wire_shard.py/test_wire_sidecar.py(byte-parity +test_toolchain_is_pinned). Verified they fail on the clean branch without this change too — likely fallout from the views-frames 1.10.2 bump (#209) changing wire bytes, or a Python-version/toolchain drift. Flagging separately — not this PR's to fix, but someone should look.Leaving the merge to whoever owns #89.