feat(enrichment): S4 (#89) — numpy/pyarrow keyed gather, drop pandas merge

docs/ADRs/012_revised_ontology.md

@@ -47,7 +47,7 @@ unfao/       who a delivery is FOR         — one partner's product and manager
 | **Delivery Invariants** | Representation-free rules over primitives that a delivery must satisfy: coverage, no-collapse, gid parity, observed-range, provenance. Live in `delivery/` — **nothing there imports pandas or views_frames**. *Forecast identity was one of these until 2026-07-31 — see the amendment below.* | Authoritative — they define what a valid delivery is | Stable — changes are governance decisions |
 | **Representation Seam** | `contract/frame_extraction.py` — turns a `views_frames` frame into the primitives the invariants consume. **One seam.** Its pandas sibling `unfao/extraction.py` was deleted in #151 once the pandas delivery was retired; the two ran as deliberate WET siblings through the migration. | Derived — isolates the representation so invariants stay representation-free | Evolving |
 | **Wire Mechanism** | `contract/wire/` — the ADR-013 contract: header, shard, sidecar, run manifest, sink, source selection. Partner-neutral: it takes its consumer name and collapse floor as **arguments** (#153). | Authoritative — the contract with the consumer | Stable — changes are contract amendments |
-| **Enrichment Asset** | The precomputed GAUL lookup (`data/gaul_lookup.parquet`), its identity in `contract/gaul_lookup.py`, its schema in `contract/gaul_schema.py`, and the pandas merge that joins it (`contract/enrichment.py`, the build/verification path). | Authoritative for geographic metadata | Stable — rebuilt only when the producer releases new GAUL data |
+| **Enrichment Asset** | The precomputed GAUL lookup (`data/gaul_lookup.parquet`), its identity in `contract/gaul_lookup.py`, its schema in `contract/gaul_schema.py`, and the keyed gather that joins it (`contract/enrichment.py`, the build/verification path — numpy/pyarrow since #89; see register **C-75** on whether that class should survive at all). | Authoritative for geographic metadata | Stable — rebuilt only when the producer releases new GAUL data |
 | **Artifact Builders** | `contract/historical.py` — turns a frame plus the lookup into the partner-facing artifact. | Derived | Evolving |
 | **External Facts** | Facts read from systems this repo does not own: the producer's (`contract/source_metadata.py` — `last_valid_month_id`, D-07) and the store's (`contract/store_metadata.py`). | Authoritative (the owning system is the source of truth) | Evolving |
 | **Launch Declarations** | `contract/launch_config.py` — the delivery mode the launcher must declare. Omitting a key is **refused by name**, never inferred (ADR-003, register C-63). | Authoritative | Stable |
@@ -60,7 +60,7 @@ unfao/       who a delivery is FOR         — one partner's product and manager
 636 lines holding two of everything, and it called `unfao/extraction.py` *"the **single**
 pandas-aware module"* when pandas lived in three. Both drifted the same way: the ADR described
 the intended end state of a migration that then stopped one step short. Both are now true —
-pandas has exactly one importer again (`contract/enrichment.py`) — and the load-bearing ones are
+pandas has **zero runtime importers** (#89 made `contract/enrichment.py`'s a type-only import under `if TYPE_CHECKING` — pandas is in that class's interface, not its implementation) — and the load-bearing ones are
 **mechanically checked** by `tests/test_doc_accuracy.py`, so the next drift fails CI instead of
 waiting for an audit.
 
@@ -117,7 +117,7 @@ looking for deleted code.
 **The rule is not weaker — it is enforced from better evidence.** The retired
 `assert_forecast_identity` compared one selected store document's `name`/`loa` against the
 configured ensemble. Since the ADR-013 contract path became the only path (#149), the same
-guarantee is enforced in `unfao/wire/source_selection.py:73-81`: `TargetLease.load()`
+guarantee is enforced in `contract/wire/source_selection.py:73-81`: `TargetLease.load()`
 checks **every shard header's declared `provenance.ensemble`** against the launched
 ensemble, and refuses the run on a mismatch. Identity now comes from the artifact's own
 declared content rather than from a metadata field on a single document, and it is checked

docs/CICs/GaulLookupEnricher.md

@@ -25,23 +25,45 @@ views-datafactory area-majority join), so this class does only a table join.
 - This class does **not** build the lookup table (that is
   `scripts/build_gaul_lookup.py`, run offline).
 - This class does **not** fill, impute, or invent metadata for unmatched cells.
-- This class does **not** validate the result — null/coverage validation is the
-  manager's `_validate()` responsibility.
+- This class does **not** validate the result. Null/coverage enforcement lives on the
+  delivery path — `contract/historical.assert_metadata_complete` at artifact build and
+  `delivery/coverage.py` for the region contract. It is **not** the manager's
+  `_validate()`, which stopped null-gating in #149 and now asserts only that the read
+  resolved. (Corrected 2026-08-02; PR #200 retired the same claim in the manager's CIC
+  and this one was left standing.)
 - This class does **not** read from the datafactory, viewser, or Appwrite.
 
 ---
 
 ## 3. Responsibilities and Guarantees
 
-- Loads exactly one lookup Parquet at construction and verifies it carries the 9
-  contract columns; missing columns raise at construction.
+- Loads exactly one lookup Parquet at construction and verifies it carries the key
+  plus the 9 contract columns; missing columns raise at construction, and so does an
+  **empty** lookup — every cell would gather to null and the delivery would then
+  complain about missing metadata rather than about a missing lookup (S4 / #89).
 - Returns the input frame augmented with exactly the 9 columns of
-  `gaul_schema.METADATA_COLS`, with their dtypes preserved from the lookup
-  (codes numeric, coordinates float, names/iso categorical).
+  `gaul_schema.METADATA_COLS`: codes numeric, coordinates float, **names and iso as
+  `object`**.
+  *Changed in S4 (#89).* They were `category`, inherited from the pandas merge that
+  read the artifact's dictionary encoding. The gather that replaced it assigns plain
+  values. Measured on a 200-row output: category 1,795,191 bytes, object 60,061 —
+  a categorical carries the artifact's full 64,742-entry dictionary whatever the
+  output size. The builder still writes the artifact with dictionary-encoded names
+  (`# names/iso categorical (C-32 memory)`); that governs the file, not this output.
 - A cell id present in the lookup is enriched with that cell's metadata.
 - A cell id **absent** from the lookup yields **null** metadata for that row —
   never a sentinel, never a fabricated value (fail-loud downstream).
-- Row count and row order of the input are preserved (left merge).
+- Row count, row order **and index** of the input are preserved. Pinned by
+  `tests/test_enrichment.py::TestFramePropertiesPreserved` across six shapes —
+  ordered, reversed, duplicated, single, empty, and a non-default index.
+  (An earlier draft of this line claimed *eight* shapes "including unknown gids",
+  counting a throwaway development script rather than the committed suite, and naming
+  a shape that class does not exercise. Unknown gids are covered, for null-value
+  correctness, by `TestFailLoud` — a different guarantee.)
+  On **empty** input this is now *more* true than before: the pandas merge replaced the
+  input's `RangeIndex` with an object-dtype `Index`, where the gather leaves it
+  untouched. The only behavioural difference found, and it is in the direction the
+  guarantee above already claimed.
 
 ---
 
@@ -60,7 +82,12 @@ views-datafactory area-majority join), so this class does only a table join.
 ## 5. Outputs and Side Effects
 
 - Output: the input frame (or, with `only_metadata=True`, just `pg_id_col` +
-  `time_id_col`) left-merged with the 9 metadata columns.
+  `time_id_col`) with the 9 metadata columns attached by **keyed gather**.
+  *Changed in S4 (#89):* this was a pandas left-merge on the lookup's index. The
+  lookup is now read with pyarrow, sorted once, and addressed by `np.searchsorted`.
+  Attaching metadata to a frame is a keyed gather, not frame algebra, and it never
+  needed a merge — which is also what frees the builder to stop writing pandas index
+  metadata (S5 / #90).
 - Public attribute: `lookup_version` — a short, stampable id read from the
   lookup's **declared** `lookup_version` metadata key at construction
   (`<region>@<8-char source digest>`). Delegates to
@@ -84,16 +111,21 @@ views-datafactory area-majority join), so this class does only a table join.
   column.
 - **Raises** `ValueError` if `pg_id_col` is not in the input.
 - **Does not raise** on unmatched cells — it surfaces them as nulls and logs a
-  WARNING. This is deliberate: the manager's `_validate()` null gate is the
-  single enforcement point, so a coverage hole fails loudly there (one place),
-  not in two. Passing a sentinel for unmatched cells would be a **bug** (it would
+  WARNING naming the unknown cells, and separately counting rows that carried no
+  usable cell id at all. This is deliberate: enforcement is a single point on the
+  delivery path (`historical.assert_metadata_complete`), so a coverage hole fails
+  loudly there, not in two places. Passing a sentinel for unmatched cells would be a **bug** (it would
   bypass that gate). Aligns with ADR-003 (fail loud on semantic ambiguity).
 
 ---
 
 ## 7. Boundaries and Interactions
 
-- Allowed to depend on: pandas, `gaul_schema`, and a local Parquet file.
+- Allowed to depend on: numpy, pyarrow, `gaul_schema`, and a local Parquet file.
+  **pandas is interface-only** — callers hand this class DataFrames and get one back,
+  but nothing here constructs, reads or joins one, and its import is under
+  `if TYPE_CHECKING` (S4 / #89). `tests/test_doc_accuracy.py` asserts by AST that the
+  package has **zero** runtime pandas importers.
 - Must **not** depend on: geopandas/shapely, the runtime mapper, the
   datafactory, viewser, Appwrite, or any network resource.
 - Treats the lookup table as an opaque, trusted artifact produced by the build
@@ -112,7 +144,12 @@ out = enricher.enrich_dataframe_with_pg_info(
 # out has the 9 metadata columns; unmatched cells are null.
 ```
 
-Drop-in for the manager's existing call (same method name and key kwargs).
+Signature-compatible with the runtime mapper it replaced (same method name and key
+kwargs), which is why the mapper-only kwargs are still accepted and ignored.
+**The manager does not call this class** — it has no `enrich` reference at all, and
+reads the lookup directly via `gaul_lookup.load()` (register C-66). This example is a
+build/verification-path usage. Whether the class should survive that is register
+**C-75**.
 
 ---
 

reports/technical_risk_register.md

@@ -5,8 +5,8 @@
 | Project           | views-postprocessing                 |
 | Owner             | Dylan Pinheiro / PRIO MD&D Team      |
 | Last Updated      | 2026-08-02                           |
-| Total Concerns    | 74                                   |
-| Open Concerns     | 15                                   |
+| Total Concerns    | 76                                   |
+| Open Concerns     | 17                                   |
 | Resolved Concerns | 59                                   |
 
 ---
@@ -532,6 +532,52 @@ Cross-refs: **C-25** (whose resolution this corrects), **C-40** (the inherited p
 
 ---
 
+### C-75: `GaulLookupEnricher` has no production caller, and now implements a second copy of the delivery path's keyed gather
+
+| Field | Value |
+|-------|-------|
+| ID | C-75 |
+| Tier | 3 — no correctness impact today: the class is off the delivery path, so a defect in it cannot reach the UN FAO. The cost is that **the verification path and the delivery path now implement the same algorithm twice**, and the tests that check the artifact run through the copy that does *not* ship. A fix applied to one and not the other makes the verification stop verifying what ships — quietly, because both would still pass their own tests. |
+| Source | `code-review max` (2026-08-02) — PR #210, five parallel reviewers; two reached this independently |
+| Trigger | When a bug is fixed in `contract/historical.py`'s gather (the one that ships), check whether `contract/enrichment.py`'s copy needs the same fix — nothing links them. Also fires at **S5 (#90)**: once the builder is pyarrow-native, the enricher's pandas interface is the last one in the package, and the question "does this class survive?" has to be answered rather than deferred again. |
+| Owner | Whoever takes **#90** — the keep-or-retire decision is theirs to make and record, not to defer a third time. Added 2026-08-03: the first draft of this entry named two triggers and no owner, while citing ADR-014 §4 in its own body. This register had already learned that twice — *"a deferral needs an owner and a trigger, not just a reason"* (Cluster L) and *"a decision awaiting an owner, not a task awaiting effort"* (epic #181 closeout). |
+| Location | `views_postprocessing/contract/enrichment.py` (the whole class; `_gather` specifically); the shipping twin is `views_postprocessing/contract/historical.py:54-68` |
+
+**Verified, not inferred (2026-08-02):** `grep -rn "GaulLookupEnricher\|enrich_dataframe_with_pg_info"` across the package finds **zero** production callers — the two hits are docstring mentions in `gaul_lookup.py`. The manager calls `gaul_lookup.load()` directly and has zero `enrich` references. **C-66**'s resolution already said this plainly: *"the pandas enricher leaves the delivery path entirely."*
+
+**What PR #210 did, and why that raises the question.** S4 (#89) rewrote this class's lookup side from a pandas merge to a numpy/pyarrow keyed gather: a measured dtype analysis, an empty-lookup guard, a mutation-proven bug fix, a corrected CIC, and five reviewers' attention. All of it spent on a method with no reachable caller outside its own test suite. The engineering is sound; what is missing is anyone having **decided** that the class should exist.
+
+**The duplication is the concrete consequence.** `_gather`'s `argsort → searchsorted → clip → equality-mask` is the same shape as `historical.py:54-68`. The policies differ deliberately — `historical` **raises** on an absent gid (*"geography must never silently vanish"*), the enricher returns nulls for the downstream gate to catch — so extracting a shared helper would mean parameterising the failure policy, which is the guessed abstraction **WET before DRY** exists to prevent. Two copies that are understood is the right call *today*. The trigger above is what stops "today" lasting indefinitely, per **ADR-014 §4**.
+
+**The precedent is C-45**, `unfao/frames.py`: an unused adapter carried on no live path, resolved by deleting it. This is the same shape with a different module, and the same question — keep it as the declared verification/reference implementation, or retire it and let the fidelity suite test `historical.py` directly.
+
+**Deliberately NOT registered from the same review** (defects in unmerged code, all fixed in #210 before merge rather than tracked): a NaN gid crashing the warning path, the unvalidated int64 coercion at both ends, the AST guard's `else`-branch blind spot, ADR-012's stale pandas-merge claim, and three CIC claims retired elsewhere by #200. The register tracks standing risk; a defect fixed before it ships is not one. They are recorded in the PR.
+
+Cross-refs: **C-45** (RESOLVED — the same shape, resolved by deletion), **C-66** (RESOLVED — established the enricher left the delivery path), **C-40** (which calls `enrichment.py` and `extraction.py` together *"the retired-in-place `enrichment.py`/`extraction.py` legacy seams"*), **#89** / **#90** / epic **#85**, ADR-014 §4.
+
+---
+
+### C-76: `build_gaul_lookup.py` will write an empty lookup without complaint
+
+| Field | Value |
+|-------|-------|
+| ID | C-76 |
+| Tier | 4 — no silent corruption. A zero-row artifact fails downstream at `historical.build_historical_table`, which raises on cells absent from the lookup. The cost is that it fails **late and confusingly**: the message names missing geography rather than an empty lookup, and the artifact is committed by then. |
+| Source | `code-review max` (2026-08-03) — PR #210 second pass, while checking whether the consumer's new guards duplicated a producer guarantee. They do not. |
+| Trigger | When `build_gaul_lookup.py` is next run with a new or renamed `--region`, or against a datafactory whose `gaul_admin` parquets have changed shape — check the printed `cells=` count is non-zero before committing the artifact. Nothing else will tell you. |
+| Owner | Whoever next runs the builder. It is a two-line guard in a script one person runs by hand, not a scheduling decision. |
+| Location | `scripts/build_gaul_lookup.py` — the invariant block at `:246-268` and the write at `:284` |
+
+The builder's invariant block is thorough about what it checks: index uniqueness (C-59), nulls in the metadata columns, `-1` sentinels in the code columns (C-35). It does not check that any rows survived. A `--region` argument that filters every cell out, or an upstream join that produces nothing, writes a zero-row parquet and prints `cells=0` as though that were a result.
+
+**Verified 2026-08-03, and the neighbouring worry is NOT real.** The same review asked whether the builder also fails to reject a null key, since `df.isna().sum().sum()` runs *after* `priogrid_gid` becomes the index and `DataFrame.isna()` does not inspect the index. It does not check it — but the null key is unreachable anyway: `df.index.astype("int64")` raises `IntCastingNaNError` two lines earlier. Protection by accident rather than by declaration, which is worth knowing, but not a defect to fix. **Only the empty case is reachable.**
+
+**Why this was found now.** PR #210 added consumer-side refusals for both an empty lookup and a null key to `GaulLookupEnricher.__init__`, and the review challenged them as duplicating a producer guarantee. Checking established the opposite: for the empty case there is no producer guarantee to duplicate, and for the null key the producer's protection is incidental. The consumer guards stay, and this entry records the producer-side half rather than quietly assuming someone will notice.
+
+Cross-refs: **C-59** and **C-61** (RESOLVED — the invariant block this sits beside, and the reason it is otherwise thorough), **C-35** (the `-1` defect class it does check for), **C-75** (the consumer whose guards prompted the check), #210.
+
+---
+
 ## Disagreements
 
 ### D-12: Post-Run-0 infrastructure & naming intents — repo rename, internal-store transport, compute co-location