Sharded vlen-bytes layout for ragged t-digest output (issue 209)

[espg]

Closes #209

Decision record

Adopts the sharded vlen-bytes layout for ragged fields, per the espg-ratified decision on issue 209: each kind: ragged field becomes ONE zarr array with the variable_length_bytes dtype and a [vlen-bytes, zstd(level=3)] inner codec chain riding inside a ShardingCodec, replacing the per-inner-chunk CSR subgroups (~1,792 objects/shard → 1). Grounding:

• Head-to-head measurement — the vlen-bytes probe won on both fixtures (sparse NEON o8 + dense RGT-1336 o9): 1 object/shard, byte-neutral stored size vs the CSR blob, 14–27× write-CPU drop, 2-GET single-cell reads, in unmodified zarr 3.2.1.
• Option-3 deep dive — why this storage layout is the typed-vlen-dtype extension's layout with an interim registered dtype, and the wire-format compatibility argument (numcodecs VLenArray framing).
• Winning prototype: branch claude/209-vlen-bytes-probe (be6535f); this PR implements it properly in the production write path.

Hard break, pre-1.0, no migration shims: existing ragged stores become debris.

Phases

• Phase 1 — writer + template + golden wire pin (477225f; review folds in c0f6462)
• Phase 2 — reader swap (bf85603: tdigest_tensor reads the vlen layout — attrs-driven decode, 2-GET single-cell read_cell, whole-store sweep over stored objects; CSR group-walk and zagg.csr deleted)
• Phase 3 — docs + benchmark-harness check + o8 re-test note + stale "CSR" docstring sweep (71213a0: new docs/ragged_layout.md wired into the mkdocs nav, notebook rewritten to the vlen reader API, config/worker/aggregate/streaming/grids/tdigest + shipped config-YAML docstrings reconciled; the benchmark harness carries no CSR/ragged references, so nothing to change there)

What phase 1 does

Template (src/zagg/grids/base.py::ragged_array_spec, wired in healpix.py/rectilinear.py::_group_spec):

• A ragged field is a variable_length_bytes array on the cell grid, fill b"". Each populated cell holds the raw little-endian bytes of its (n, *inner_shape) payload; the interpretation is self-describing via array attrs: attrs["ragged"] = {"element": {"dtype": "float32", "shape": [-1, 2]}} (key: RAGGED_ELEMENT_ATTR).
• Codec chain [vlen-bytes, zstd(level=3)] (level 3 matches the coverage-sidecar precedent). The chain sits inside ShardingCodec(chunk_shape=(cells_per_chunk,)) when the grid is sharded (outer chunk mirrors the dense arrays' issue-update benchmarks #133 object split) and across the whole leaf in a hive shard_spec — one object per shard on both. The unsharded flat layout stays a regular array chunked at the inner chunk (one object per inner chunk), because that path streams per-chunk writes (issue Stream-and-free chunk writes in the multi-chunk worker (don't accumulate all K) #91) and a shared shard object would be read-modify-written K times.
• A located field (issue High-resolution morton location channel for ragged/CSR fields #87) gets a sibling {field}_locations uint64 vlen array, row-aligned with the payload, and (phase 2) the payload array's attrs declare the linkage: attrs["ragged"]["locations"] = "". resolution: chunk ragged (issue Vector (and ragged?) chunk companions: resolution: chunk for non-scalar kinds #82) sits on the chunk grid (one payload per chunk), like the scalar/vector companions.
• The UnstableSpecificationWarning at creation is the dtype naming only (variable_length_bytes vs the registry's bytes, zarr-python#3517); suppressed message-scoped at the emit sites (vlen_dtype_warning_suppressed, the coverage.moc precedent).

Writer (src/zagg/processing/write.py):

• Sharded flat path: ragged fields now ride write_shard_to_zarr's per-object slab pass — the whole shard's payloads are assembled into one b""-filled object slab and written in ONE set_block_selection, exactly like the dense columns. Deleted: _write_ragged_fanout, the issue-ShardingCodec doesn't consolidate ragged (tdigest) writes — K serial CSR groups dominate shard write time #142 thread pool, the issue-Sharded-path parallel CSR writes race under real CPU parallelism (fails 3/3 on 6 GB workers, largest shard) #186 failure roll-call, _block_index_key, and the write_csr dependency — all existed only to manage the object storm this layout removes.
• Hive leaf path (hive.py::process_and_write_hive + new write_ragged_leaf_to_zarr): the streamed chunks' ragged payloads accumulate (leaf-local blocks) and are written as ONE object after the stream, preserving the pinned leaf order dense → ragged → coverage sidecar → stamp. The memory bound is quantified at the accumulation site (review fold): ~141 MB held payload / ~200 MB peak at the o8 t-digest scale, accepted under the 4 GB workers (issue Skip zarr metadata consolidation by default (opt-in) #193); the named escape valve is spilling back to per-inner-chunk writes.
• Unsharded per-chunk paths (runner + Lambda handler _write_chunk): write_ragged_to_zarr is re-cut to (ragged, store, grid=, chunk_idx=) — the same block addressing as write_dataframe_to_zarr — deleting the K==1-shard-label / K>1-block-key naming split (both call sites simplify).
• Empty cells keep the b"" fill and all-empty inner chunks are omitted from the shard index (sub-shard sparsity, same as dense).
• validate_config rejects an agg field/coordinate named {located_field}_locations (review fold — the silent template/slab collision).

What phase 2 does

Reader swap (src/zagg/readers/tdigest_tensor.py): the CSR group-walk (_shard_groups/_resolve_chunk_morton/_subgroup_words and the read_csr consumption) is gone.

• Whole-store sweep: one LIST of the array's stored c/ objects (never a probe of every chunk of a mostly-fill array), then per-read-chunk decode via the attrs element spec — the reader decodes what the writer declared, no hardcoded f32. One code path reads BOTH flat layouts (ShardingCodec'd and per-inner-chunk regular): the difference is self-describing in array metadata, per the round-1 Q1 verdict.
• Chunk identity: derived from the sibling morton per-cell coordinate (any written cell's word coarsened to the chunk order) — the same coverage-cell id the CSR subgroup names carried. Missing morton sibling or missing element attrs → pointed errors (no silent misinterpretation; no legacy-name fallback).
• Single-cell path: new read_cell(store, field, cell) indexes the vlen array directly — 2 ranged GETs on a sharded store, pinned by a counting-store test (index suffix size asserted exactly; inner-chunk GET asserted ≪ object). Works on the {field}_locations sibling too (it has its own element attrs).
• Locations linkage: read_locations binds the sibling through attrs["ragged"]["locations"] (writer emits it; no derived-name fallback — one convention, per the Q2 verdict).
• Deleted: src/zagg/csr.py + tests/test_csr.py — production-orphaned once the reader swapped (the last importers). Its write-side guards (per-cell inner-shape tiling, locations row-alignment, empty-on-empty) live on in _place-path checks in write.py, already under test.
• Tensor shape generalizes from hardcoded 64×64 to side = isqrt(cells_per_chunk) — identical output for the production chunk_inner configs (4096-cell chunks), and lets the tests exercise real grids at smaller orders.

Reader tests (tests/test_readers.py, rewritten): fixtures build REAL stores through the production template + writers (no synthetic CSR): round-trip tensors/raw-values/locations on the K==1 layout, a sharded-vs-regular layout parity test (identical tensors + chunk ids from the same logical data — the Q1 pin), locations alignment with read_raw_values, read_cell decode + 2-GET pin, missing-attrs and missing-morton pointed errors, and the issue #191 consolidation-parity test rebuilt on the vlen layout.

What phase 3 does

• docs/ragged_layout.md (new; wired into the mkdocs Design nav next to sharding.md/hive_layout.md): the vlen-bytes layout end to end — the on-disk arrays ({field} / {field}_locations / morton), the attrs["ragged"] contract (spec = "zagg-ragged/1", element, locations — exact key names), the golden-pinned wire framing (u32 cell_count then per cell u32 length + payload, little-endian numcodecs VLenBytes/VLenArray framing), sharded vs per-inner-chunk layouts and the Q1 rationale (unsharded flat K>1 keeps per-inner-chunk objects to preserve the Stream-and-free chunk writes in the multi-chunk worker (don't accumulate all K) #91 stream-and-free bound; both self-describing, one reader path), the hive-leaf reader contract (leaf carries the morton sibling + ragged attrs under the same group path), the two read paths (whole-store sweep; read_cell 2-GET random access), and the issue Typed vlen-array<float32> Zarr data type + codec (follow-up to the issue 209 vlen-bytes layout) #210 typed-vlen-array<T> dtype migration note (metadata-only, guaranteed by the golden framing pin).
• notebooks/tdigest_reader_example.ipynb rewritten to the new reader API: builds the synthetic store through the production template + write_ragged_to_zarr (was zagg.csr.write_csr), peeks a cell with read_cell, keeps its in-memory Binder-runnable data source, stable cell ids, outputs cleared; verified nbformat-valid, every code cell ast.parse-clean and executes end-to-end (the one matplotlib cell ast-parsed only where the extra is absent).
• Stale-CSR docstring sweep across src/: reconciled present-tense CSR references in config.py, processing/{worker,aggregate,streaming}.py, grids/{morton,healpix,base}.py, stats/tdigest.py, and the shipped config YAMLs (atl03_tdigest_healpix, atl03_tdigest_located_healpix, atl03_gain_bias_healpix) — "the CSR writer" → the ragged/vlen writer, "companion CSR array" → companion vlen array, "CSR subgroup names" (as a current external surface) → hive leaf ids, and the located config's stale {field}/{shard}/locations on-disk description → the {field}_locations sibling. Left standing (deliberate historical contrast, not stale): the "replaces the per-inner-chunk CSR subgroups" / "pre-issue-209 CSR stores are a hard break" / "the CSR layout named its subgroups by" / "write_csr enforced" notes in write.py, readers/tdigest_tensor.py, grids/base.py, grids/healpix.py, grids/rectilinear.py, which accurately describe what the layout replaced.
• Benchmark harness: .github/scripts/run_benchmark.py carries no CSR object-count / ragged-layout references (checked), so it needed no change; .github/workflows/ and deployment/aws/ untouched.

Questions for review — RESOLVED (round-1 verdicts adopted)

1. Unsharded flat K>1 keeps per-inner-chunk objects. The Stream-and-free chunk writes in the multi-chunk worker (don't accumulate all K) #91 stream-and-free write is the point of that path; collapsing would recreate the accumulation question or the K-RMW anti-pattern. Not a reader fork — the layout is self-describing, and the phase-2 reader reads both through one path (pinned by test_sharded_and_regular_layouts_read_identically). Docs note lands in phase 3 (docs/ragged_layout.md).
2. {field}_locations name stays. The collision guard is in validate_config (phase-1 fold), and the attrs-declared linkage (attrs["ragged"]["locations"]) landed with the phase-2 reader, which binds by metadata only.

How it was tested

• Phase-1 gates in tests/test_processing.py::TestRaggedVlenLayout: one-object-per-shard, bit-exact fresh-reopen round trip (variable lengths incl. n=0 payloads and empty cells), empty-inner-chunk omission against the shard-index sentinel, the golden framing vector (the wire pin for the typed-dtype migration), single-cell read = exactly 2 ranged GETs with tightened span assertions, flat-sharded vs hive-leaf parity, and (review fold) the located channel end-to-end through the sharded slab pass.
• Full suite: uv run pytest -q → 1765 passed, 28 skipped at 71213a0 (skips pre-existing / optional-dep-gated). ruff check / ruff format --check clean on every touched file; codespell clean on the changed docs and the touched lines. The phase-3 diff is docs/comment-only in src/ plus the new markdown page, the notebook, and the mkdocs nav — no runtime code changed.
• CI: phase-1 head 477225f fully green including test (3.13). Phase-2 head bf85603 running at last check.

Post-merge validation

• o8 order-sweep re-test. The o8 leg of the fleet order sweep hit the 900 s worker timeout under the old per-inner-chunk CSR fan-out; this layout removes the ~K×7 tiny-PUT storm the timeout traced to (0.44 s vs 12.1 s write on the sparse NEON o8 fixture from the issue t-digest ragged output bypasses the sharding codec (~1,792 objects/shard) — needs a sharding-codec-like path #209 head-to-head), so o8 is expected to clear. Re-running the o8/o9/o10 sweep (espg-side, credentialed) confirms it and feeds issue Benchmarks: full-AOI NEON runs, re-pin temporal to full catalog (→ 2026-03-15), reset live series, CONUS cost model #202's cost model its new per-order row — including the measured worker RSS, which checks the ~200 MB hive-accumulation envelope this PR documents and accepts.

Notes for review

• deployment/aws/lambda_handler.py is touched (phase 1: the _write_chunk ragged call + dropped imports, net −13 lines). It is the Lambda application source, routinely edited by prior issue PRs (main red after #170 merge + template hammer edits (test_apriori/test_streaming under the inline default; template pins) #175/Granule-level read concurrency: the worker's serial granule x group loop is the dominant read-wall term #180/Adopt morton decimal shard ids + hive-partitioned output layout (manifest, commit stamp) #199/Write store coverage MOC at end of run (dispatcher-side domain declaration) #200), not one of the §1 infra/deploy scripts.
• zstd on the ragged array deviates from the dense bytes-only/uncompressed policy deliberately: a vlen payload has no fixed-width raw layout to preserve, the CSR arrays it replaces were already codec-bearing, and the level is pinned (3) for cross-worker byte determinism.
• The reader's stored-object LIST uses zarr.core.sync.sync (zarr 3 exposes no public sync listing) — one semi-private import, commented at the use site.
• Pre-existing, unrelated, left standing: ruff check src N818 on src/zagg/registry.py (on main too); ruff format --check drift on __init__.py/catalog/sources.py/viz/crs.py/test_catalog.py/test_extract.py/test_integration.py (untouched here); codespell gir (grids/rectilinear.py:276-277) and anc (grids/morton.py:153-154) on lines this PR did not touch; and the mypy hook lacks types-PyYAML (a one-liner in .pre-commit-config.yaml, left for a human call since it edits the hook's dependency list).

[espg]

🤖 from Claude (review)

Fresh-context adversarial review of phase 1 (477225f), traced against the issue 209 decision record and the deep-dive layout argument, plus a local run of the new gates (TestRaggedVlenLayout + test_hive.py: 54 passed) and empirical checks in the repo venv.

No blocking-correctness findings

The data-loss surfaces I went after hardest all check out:

• Cell alignment: dense columns land via values.reshape(inner_shape) into region; ragged payloads land via row-major np.unravel_index(cid, grid.chunk_shape) into the same region view — consistent by construction for HEALPix (1-D) and rectilinear (2-D), on all three paths (sharded slab pass, per-chunk unsharded, hive leaf). The ragged array's shard geometry matches the dense arrays' exactly: cells_per_shard_object on HEALPix (issue #133 object split included), (chunk_h, chunk_w) on rectilinear, whole leaf in shard_spec.
• Empty vs n=0: both collapse to the b"" fill, identically on flat and hive (write_csr skipped empties too, so this is parity, not a change), and test_one_object_per_shard_and_bitexact_roundtrip pins it — payloads excludes the n=0 entries and the test asserts set(seen) == set(payloads), so a regression that started storing (or dropping) either side would fail.
• Golden wire pin is the real thing: test_golden_inner_chunk_framing parses the shard index footer manually (offset/nbytes pairs + sentinel), zstd-decodes the located inner chunk, and compares against a hand-built struct.pack vector — no re-encoding. u64 lengths, an omitted count, or a framing reorder would all fail it. This is exactly the pin the issue-210 typed-dtype migration needs.
• Deletions are clean: no stragglers for _write_ragged_fanout / _block_index_key / the #142 pool constants; fd_safe_max_workers survives only in its legitimate dispatcher uses; no dead imports in runner/handler (shard_label in runner.py is still used for status labels). The #186 failure roll-call was ragged-fanout-only — dense writes raise synchronously from set_block_selection and never depended on it. (The #186 S3 retry pacing in store.py is a separate, untouched aspect of that issue.)
• Hive: the dense→ragged→sidecar→stamp ordering pin survives and now covers write_ragged_leaf_to_zarr (test_stamp_is_the_final_write), and the torn-write test pins that a torn worker leaves no partial ragged data.
• Warning scope, verified empirically (zarr 3.2.1 in the repo venv): the suppression regex matches the actual #3517 message; creation without the context manager emits exactly 1 UnstableSpecificationWarning; write_shard_to_zarr + open_array + read emit zero — so leaving _set_ragged_block/readers unsuppressed is correct, and nothing else is swallowed.
• Conventions: lambda_handler.py edit is app-source with PR precedent and is net −13 as claimed; no new deps, no CI/workflow/infra files touched; write.py at 830 lines is under the §4 ceiling; CSR module + reader deliberately kept green for phase 2, as documented.

Findings (inline)

• should-fix — hive.py: the "small" memory claim for leaf ragged accumulation is unquantified; it's ~140 MB payload / ~200 MB peak at the o8 t-digest scale this fix exists to unlock, on the path built to be O(chunk) (#91/#143/#148). Accumulate-once is the right design; the bound should be stated and sanity-checked against the worker cap.
• should-fix — grids/base.py: no collision guard for the derived {field}_locations name; a colliding user field corrupts data silently in _accumulate_ragged_slabs. Includes my verdict on PR question 2 below.
• should-fix — grids/healpix.py: the located channel is never tested through the sharded template/slab pass.
• nit — tests: the 2-GET assertion accepts a ranged GET spanning the whole object; assert the spans are small relative to the object.

Verdicts on the PR questions

1. Unsharded flat K>1 — keep per-inner-chunk objects. The stream-and-free write (#91) is the point of that path, and collapsing to one object would force either whole-shard accumulation (recreating the hive memory question on a path that exists precisely to avoid it) or K read-modify-writes of a shared object (the anti-pattern this PR deletes). Crucially it is not a reader fork: the layout difference is fully self-describing in array metadata (regular chunk grid vs sharding_indexed), so the phase-2 reader uses the same zarr API on both. Since that variant exists only for the benchmark A/B, the inconsistency is contained; just say so in the phase-3 docs. Don't fold it into phase 2.
2. {field}_locations — keep the name, add the guard. A subgroup is impossible (array nodes can't have children — the same constraint that killed the CSR fourth array), and a reserved prefix buys nothing over a sibling. What's needed is a validate_config rejection of colliding field names plus an attrs-declared linkage so the reader binds by metadata, not convention (detail in the inline comment). Phase 2 is the right slot, before the reader ships.

One stale-docs note (phase 3 fodder, no inline anchor — files untouched by this diff)

config.py (lines ~675, 718–719, 777, 1356), worker.py:132/159/201, aggregate.py (several), streaming.py:129, and stats/tdigest.py:70 still describe the on-disk layout as "companion CSR array" / "the CSR writer". config.py's are user-facing schema descriptions that now misdescribe the store. Fine to sweep in phase 3 with the notebook, but they shouldn't be forgotten — the PR body's sweep list doesn't mention them.

CI state for 477225f

All concluded checks green: test (3.12) ✅, ruff ✅, lambda build x86_64/arm64 ✅, docs build/check ✅ (deploy/pr-preview skipped, as expected on a fork-less draft). test (3.13) still shows in_progress at review time (started with the push; likely a stale/stuck runner given 3.12 finished in ~8 min) — worth a re-run if it hasn't concluded by the next pass. Draft state is correct while it's outstanding.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

should-fix (quantify the memory claim): "small next to the dense slabs the sharded flat path already buffers" compares against the wrong baseline. This is the streaming path — the one issue #91/#143/#148 built to hold ~1 chunk of output, and on main the per-chunk CSR write freed each chunk's ragged payloads immediately. Accumulating ragged_chunks reintroduces an O(whole-shard-payload) term on exactly the path that was O(chunk):

• sparse NEON o8 fixture (the shard size this fix is meant to unlock, per the head-to-head): 17.6 M centroids × 8 B ≈ 141 MB raw payload, plus ~12–16 MB of per-bytes-object + slab-pointer overhead in write_ragged_leaf_to_zarr's object slab, plus the assembled ~60 MB shard object during the single set_block_selection — roughly ~200 MB peak added;
• dense RGT-1336 o9: 12.9 M centroids ≈ ~103 MB payload, ~150 MB peak added.

That's likely fine under the Lambda budget (the flat sharded path already buffers all K carriers), and accumulate-then-write-once is the right call for a one-object-per-shard layout (per-chunk writes would RMW the shard object K times, as the docstring says). But the bound changed on the tight-memory path, so it should be stated with numbers, not "small": suggest replacing the parenthetical with the rough figure at the o8 t-digest scale, so the next memory investigation doesn't have to rediscover it — and confirming the o8 hive worker (payload + dense chunk + read side) was actually profiled or at least back-of-enveloped against the worker's memory cap. Not blocking: correctness is unaffected and the ordering/torn-write semantics are pinned by tests.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

should-fix (name-collision guard; answers PR question 2): the derived {field}_locations name has no guard anywhere. A config that declares a located ragged field foo and a user field literally named foo_locations collides twice, both silently:

1. in _group_spec's members dict (healpix.py:724 / rectilinear.py:633) — last assignment wins, so one array's template silently replaces the other;
2. in _accumulate_ragged_slabs' slabs dict (write.py:596-598) — the user field's payload bytes and the location words interleave per cell into the same slab, i.e. actual data corruption, not just a template quirk.

Pre-existing coord names (cell_ids, morton, count) have the same theoretical hazard, but those are names the user can see; this PR mints a name the user never wrote, which makes the collision much easier to hit blind.

Verdict on the naming itself: keep {field}_locations — a subgroup is impossible for the same reason the CSR fourth array moved (an array node can't have children), and a reserved-prefix scheme (_locations/{field}) would put a group where the flat cell-grid arrays live for no gain. What's missing is (a) a validate_config rejection of any agg-field name that equals ragged_locations_name(other_located_field) — a 5-line check with a test — and (b) declaring the linkage in attrs (e.g. attrs["ragged"]["locations"] = "<sibling name>" on the payload array, or "location_of": "<field>" on the sibling) so the phase-2 reader binds by metadata rather than reconstructing the naming convention. Fine to land (a) as a phase-2 item since the reader hasn't shipped, but it should not slip past this PR series.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

should-fix (test gap): the located channel is never exercised through the sharded layout. test_end_to_end_located_write_then_read pins {field}_locations row-alignment on the unsharded K==1 path (write_ragged_to_zarr), and TestRaggedVlenLayout exercises the sharded slab pass with an unlocated field only — so this branch (the uint64 sibling created with shard_shape) and the located arm of _accumulate_ragged_slabs under write_shard_to_zarr's per-object regions run in no test. The alignment code is shared, so I traced it and believe it's correct (payload and locations use the same region/pos, and the n_rows check keeps them row-aligned), but per-cell payload↔locations alignment across an object-split shard is precisely the kind of invariant this PR's other gates pin byte-hard. One test — a located field in _sharded_setup-style hand-built chunk_results through write_shard_to_zarr, asserting per-cell len(locations) == n_rows(digest) after reopen — would close it.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

nit (loophole in the 2-GET assertion): byte_range is not None doesn't quite pin "never the whole shard" — a RangeByteRequest(0, len(object)) (or a suffix request spanning the object) is ranged and would pass while doing a whole-shard read under the hood. The GET count of exactly 2 is the strong half; the range assertion could be tightened cheaply, e.g. record the object's total size and assert each request's span is a small fraction of it (index suffix ≈ 16·K+4 bytes; the inner chunk ≪ object). As-is, a regression to "2 GETs that each fetch everything" would slip through.

---

Generated by Claude Code

[espg]

🤖 from Claude

Round-1 findings folded (c0f6462) and phase 2 landed (bf85603). Per-finding dispositions against the round-1 review:

Review dispositions

1. hive leaf memory — folded (documented, not re-engineered). The accumulation-site comment in hive.py now carries the arithmetic verbatim: ~141 MB held payload (17.6 M centroids × 8 B, the sparse-NEON-o8 point this fix exists to unlock) and ~200 MB peak through the single write once per-bytes-object overhead + the assembled ~60 MB shard object are counted; accepted under the 4 GB workers (issue Skip zarr metadata consolidation by default (opt-in) #193) because the dense side keeps its O(chunk) Stream-and-free chunk writes in the multi-chunk worker (don't accumulate all K) #91 bound and the accumulation is what deletes the ~K×7 PUT storm (~1/3 of shard wall at CONUS scale). The named escape valve — spill the ragged field back to per-inner-chunk writes against a regular-chunked vlen array — is recorded in the comment, not built. I looked for a cheap accumulation win and there isn't one: the sink already holds the worker's float32 arrays by reference (no copy), and encoding to bytes at accumulation time is byte-for-byte the same footprint; compressing early would trade CPU for a bound we're under. No profiled o8 hive run exists yet — the o8 re-test note is the phase-3 item, and its measured RSS will check this envelope.
2. {field}_locations collision — folded, both halves. (a) validate_config now rejects any agg field OR coordinate named ragged_locations_name(located_field) with a pointed message (config.py; tests cover the field collision, the coordinate collision, and that the suffix stays free for UNlocated fields). (b) The attrs-declared linkage landed with phase 2: the payload array carries attrs["ragged"]["locations"] = "<sibling name>" (writer: ragged_array_spec(locations=...); reader: read_locations binds through the declaration only — no derived-name fallback, one convention).
3. located channel through the sharded slab pass — folded. test_located_field_through_sharded_slab_pass drives hand-built located triples (empty payloads and empty chunks included) through write_shard_to_zarr, asserts one object per array (payload AND sibling), and after a fresh reopen checks per-cell len(locations) == n_rows(digest) plus bit-exact values on both channels.
4. 2-GET assertion loophole — folded. The counting store now records response sizes; the test asserts the first GET is exactly the 16·K+4-byte index suffix and the second ≤ a quarter of the object (one compressed inner chunk of ~12 similar-size populated chunks). A "2 GETs that each fetch everything" regression now fails. The same tightened pin guards the new reader-level test (test_two_ranged_gets_on_sharded_store).
5. Q1/Q2 verdicts adopted — PR body updated: unsharded flat K>1 keeps per-inner-chunk objects (and phase 2 pins that this is not a reader fork — see below); the _locations name stays with guard + linkage.

Phase 2 — reader swap (bf85603)

readers/tdigest_tensor.py now consumes the vlen layout; the CSR group-walk is deleted, and with its last importers gone, src/zagg/csr.py + tests/test_csr.py are deleted (its write-side guards — inner-shape tiling, locations row-alignment — live on in write.py's _place checks, already under test). Key shapes:

• Attrs-driven decode: _open_ragged reads attrs["ragged"]["element"] and decodes exactly what the writer declared; a vlen array without the declaration raises a pointed error (pinned by test_missing_element_attrs_is_pointed_error) instead of decoding under a guessed layout.
• Whole-store sweep: one LIST of stored c/<ordinal> objects, then per-read-chunk decode — the sweep visits only written data, and ONE code path reads both the ShardingCodec'd and the per-inner-chunk layouts (the Q1 claim, pinned by test_sharded_and_regular_layouts_read_identically: same logical data through both writers → identical tensors + chunk ids).
• Chunk identity: coarsened from the sibling morton per-cell coordinate (clip2order to the chunk order) — the same coverage-cell id the CSR subgroup names carried; missing sibling raises pointed (no name fallback, hard break).
• Single-cell: new read_cell indexes the vlen array directly — the 2-GET property, pinned reader-side with the tightened span assertions; works on the _locations sibling too.
• Fixtures rewrite: tests/test_readers.py now builds stores exclusively through the production template + writers (emit_template + write_ragged_to_zarr/write_shard_to_zarr); the issue Skip zarr metadata consolidation by default: ~70s/run finalize tax no reader uses #191 consolidation-parity test was rebuilt on the vlen layout.
• Sweep of other CSR consumers: index backends (src/zagg/index/) and zagg.temporal/viz touch no ragged data; the only remaining references are stale docstrings (config/worker/aggregate/streaming/tdigest — the review's inventory) and notebooks/tdigest_reader_example.ipynb, which now genuinely breaks with zagg.csr gone — both queued as phase 3.

Testing / CI

Full local suite 1756 passed, 28 skipped; ruff check + format --check clean on touched files under the dev ruff and the pre-commit-pinned 0.14.10; mypy delta vs main is −5 (deleted csr.py findings), 0 new. One flag: the mypy hook lacks types-PyYAML, so a pre-existing missing-stub error for yaml flaps between report sites as the module graph changes (now at config.py:12); the one-line fix edits .pre-commit-config.yaml's dependency list, so it's left for a human call.

CI: the round-1 note on test (3.13) at 477225f resolved itself — the run concluded green at 01:16Z (~15 min; slow runner, not stuck), so 477225f is fully green. The bf85603 run was in progress at last check.

Staying in draft for the next review round; phase 3 (docs + benchmark touch-ups + o8 re-test note) not started.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

Fresh-context adversarial review, round 2 — the two newest commits only: c0f6462 (round-1 folds) and bf85603 (phase 2: reader swap, csr.py deleted). Verified against the issue 209 decision record, with the new suites run locally (test_readers 36 passed; the fold tests in test_config/test_processing green) and the read paths probed empirically in the repo venv.

No blocking-correctness findings

The reader's math checks out where I attacked it hardest:

• One code path for both flat layouts, genuinely. _stored_chunk_spans derives the span from arr.shards or arr.chunks and sorts ordinals numerically (int(...) before sort — c/10 cannot misorder before c/2); test_sharded_and_regular_layouts_read_identically pins identical tensors + chunk ids from the same logical data. Empty cells (b"") decode to the zero-length (0, *inner_shape) element on both layouts; all-fill chunks are skipped identically (absent object on regular, index-omitted on sharded).
• Chunk identity is sound. I verified _chunk_word round-trips empirically: 256 order-12 children coarsen to exactly their order-8 parent word (clip2order at cell_order − log4(cells_per_chunk)), the same coverage-cell id the CSR subgroup names carried; missing morton / missing attrs raise the pointed errors the tests pin. (The depth-from-slice-length arithmetic is only safe because HEALPix chunk sizes are powers of 4 and array shapes are chunk multiples — a truncated slice would silently coarsen one order short, but no zagg grid can produce one.)
• read_cell matches the writer's rank convention (global index = block base + row-major cell id — pinned by test_decodes_declared_element), and the 2-GET pin is now exact (index suffix == 16·K+4).
• Deletion is clean. No zagg.csr / read_csr / iter_csr_cells imports survive in src/ or tests/ (the notebook is the declared phase-3 item); readers/__init__.py re-exports are consistent; the #191 consolidation-parity test still tests the same invariant — non-consolidated navigation byte-parity — now through all three vlen readers on a production-written located store, which is a strictly stronger rebuild.
• Conventions: modules at 585/830/843 lines; no new deps; no CI/workflow/deploy files in the diff; the types-PyYAML pre-commit gap correctly left for a human (it edits the hook config).

Round-1 fold verdicts — all four genuinely folded by c0f6462

1. Memory note (hive.py): folded; the arithmetic is right (17.6 M × 8 B ≈ 141 MB; ~200 MB peak with the ~60 MB assembled object) and the escape valve is named. One residual echo of the old claim survives in write.py:502 ("the (small) ragged payloads") — nit, inline.
2. Collision guard: folded properly — rejects both a colliding field and a colliding coordinate, and allows the _locations suffix for unlocated fields; grid-agnostic since it sits in validate_config. Three targeted tests.
3. Located channel through the sharded slab pass: folded with a real test — end-to-end write_shard_to_zarr, mixed n=0 payloads and empty chunks, asserting per-cell row alignment (w.shape == (v.shape[0],)) and exact payload/location array equality after fresh reopen.
4. 2-GET assertion: folded stronger than asked — exact index-suffix size, inner chunk bounded ≪ object.

Findings (inline, priority order)

• should-fix — read_cell: out-of-range or negative cell dies with ValueError: not enough values to unpack (zarr clamps the slice), not a pointed error; -1 neither wraps nor explains. Verified empirically.
• should-fix — the whole-store sweep re-fetches the shard-index suffix once per inner chunk (measured: 16 identical suffix GETs + 4 chunk GETs on a K=16 shard) — ~256 redundant sequential GETs per shard at production geometry. One slice per stored span fixes it at the cost of the already-accepted ~141 MB span bound.
• should-fix — coverage: the writer-side ragged guards that replaced write_csr's checks are real, but their raising branches are untested — the substance of 5–6 deleted test_csr.py tests has no equivalent at head, and the PR body's "already under test" claim is wrong on that half. (Everything else deleted is genuinely superseded; the 1768→1756 count reconciles exactly — detail inline.)
• question-for-author — the attrs["ragged"] contract carries no version marker ahead of the issue 210 typed-dtype migration; and a garbage element.dtype string surfaces as a raw numpy TypeError, not this module's pointed style. Both one-pass decisions for the phase-3 freeze.
• nit — the hive leaf-at-a-time reader contract is implicit (works, traced; but undocumented and untested through the readers) — phase-3 fodder alongside the notebook.

CI state for bf85603

ruff ✅, test (3.12) ✅, lambda build x86_64 + arm64 ✅, docs build ✅ (deploy / pr-preview skipped as expected on a draft). test (3.13) still in_progress at review time (started 01:50Z) — the same slow-3.13 pattern as phase 1, which eventually concluded green; mergeable_state: unstable reflects only that pending check. Draft state remains correct while phase 3 is open.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

should-fix (unpointed error on a bad cell index): read_cell promises "an absent/empty cell returns the zero-length element", but an out-of-range or negative cell crashes with an unrelated message — verified in the repo venv against a production-template store (shape 201,326,592):

read_cell(store, "12/h_tdigest", n)      # ValueError: not enough values to unpack (expected 1, got 0)
read_cell(store, "12/h_tdigest", n + 5)  # same
read_cell(store, "12/h_tdigest", -1)     # same (zarr's slice [-1:0] is empty — no numpy-style wrap)

zarr basic selection clamps the slice instead of raising, so the 1-tuple unpack is what fails. This module's own standard (which phase 2 explicitly adopts — missing attrs/morton are pointed errors, "no silent misinterpretation") argues for a bounds check: if not 0 <= cell < arr.shape[0]: raise IndexError(f"cell {cell} out of range for {field!r} with {arr.shape[0]} cells") — three lines plus a two-line test. Note the -1 case is the sharpest edge: a caller expecting numpy negative-index semantics gets neither the last cell nor a comprehensible error.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

should-fix (sweep re-reads the shard index once per inner chunk): results are correct (I verified sharded/regular parity), but slicing arr[start : start + cells_per_chunk] per inner chunk on a ShardingCodec'd array re-fetches the shard-index suffix on every slice — zarr caches nothing across __getitem__ calls. Measured with a counting store on a K=16 shard with 4 populated chunks: the sweep issued 20 GETs on one object — 16 identical SuffixByteRequest(260) index reads + 4 inner-chunk reads. At the production K=256 (the o9 CONUS geometry in the issue 209 head-to-head) that is ~256 sequential index GETs per shard, paid even for all-empty inner chunks — a whole-store sweep on S3 becomes latency-bound on redundant round trips, which cuts against the decision record's read-efficiency argument.

One-slice-per-stored-span fixes it: iterate _stored_chunk_spans and read arr[span_start:span_stop] once (1 index GET + populated-chunk GETs only; zarr's sharded partial read skips index-omitted chunks), then split into read chunks in memory. The cost is holding one span's decoded payload — ~141 MB at the o8 t-digest scale, i.e. exactly the bound process_and_write_hive now documents and accepts on the write side, and this is the client-side bulk reader, not the Lambda path. If the memory bound is instead the reason for per-chunk slicing, that tradeoff deserves the same named-and-quantified treatment the hive accumulation comment got in c0f6462.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

should-fix (coverage lost with test_csr.py — the guard error branches are untested): the PR body says the deleted write_csr guards "live on in _place-path checks in write.py, already under test". The guards live on; the "already under test" half is not accurate. I grepped the suite at head — none of these raising branches is exercised anywhere:

• values_list/cell_ids length mismatch (this line) — was test_csr.py::test_mismatched_lengths_raises
• locations_list/values_list length mismatch (L591) — was test_locations_list_length_mismatch_raises
• per-cell row-misalignment at L609, which also subsumes nonempty-locations-on-empty-payload (n_rows == 0 forces shape (0,)) — was test_per_cell_location_length_mismatch_raises + test_nonempty_locations_on_empty_payload_raise
• payload does not tile inner_shape (_ragged_payload_bytes, L556) — was test_inconsistent_inner_shape_raises
• located resolution: chunk rejection (_write_ragged_companions, L640) — new in this PR, also untested

These are exactly the malformed-worker-output paths that would otherwise surface as silent corruption or a confusing numpy error deep in the slab pass, and they were the substance of 5–6 of test_csr.py's 18 deleted tests (the other 12 — round-trips, empties, dtype — have genuine new equivalents in TestRaggedVlenLayout and the reader round-trips; the 6 deleted subgroup-name-recovery tests in old test_readers.py are superseded by the no-fallback design and its pointed-error tests, no loss there). One small pytest.raises class covering the five messages closes it; the PR-body claim should be corrected either way.

For the record, the 1768 → 1756 arithmetic reconciles exactly: −18 (test_csr.py) + 2 (test_readers.py 34→36 items) + 3 (collision-guard tests) + 1 (located sharded slab test) = −12.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

question-for-author (attrs contract shape ahead of issue 210): the contract itself is in good shape — built at this single choke point, so all three emit variants (sharded flat, hive leaf shard_spec, unsharded regular) and both grids carry identical attrs, and the reader binds strictly (missing/malformed element → pointed error, locations by declaration only). Two small choices worth settling before the phase-3 doc freezes it:

1. No version marker. Issue 210's typed-vlen-dtype migration will presumably drop or supersede attrs["ragged"]; a reader then has to discriminate by dtype name alone. Options: (a) stamp {"version": 1} into ragged_meta now (one line, and the migration bumps or removes it), or (b) declare the zarr data_type string the discriminator and say so in the phase-3 doc. Either works — which do you want on disk before stores proliferate?
2. Garbage dtype string decodes to a raw numpy error, not this layout's pointed style: np.dtype(str(element["dtype"])) in _open_ragged raises TypeError: data type 'x' not understood with no mention of the field or the attrs contract. Not garbage-in-garbage-out (nothing decodes wrong), but wrapping it in the same ValueError(f"{field!r} declares an unreadable element dtype ...") pattern would match the missing-attrs branch. Fine as a phase-3 rider.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

nit (hive contract is implicit — say it, and pin it once): I traced the hive question and the one-code-path claim holds: a hive leaf's ragged array is the same ragged_array_spec product (whole-leaf ShardingCodec → one stored span), the leaf carries the morton sibling and identical attrs, and write_ragged_leaf_to_zarr writes under the same grid.group_path — so pointing this reader at a leaf store works through exactly this sweep. But that contract is only implicit: the reader is store-scoped (no digit-tree traversal — same as the old CSR reader, which took one field prefix in one store), the docstring never says "for a hive product, open one leaf store and pass the same group path", and no test opens an actual hive leaf through a reader — the flat↔leaf parity pin (test_hive_leaf_parity_with_flat_sharded) lives on the write side and compares arrays, not reader output. Since the phase-3 checklist already owns docs + the notebook rewrite, fold two things into it: the explicit leaf-at-a-time sentence here, and one cheap test that runs read_tensors/read_cell against a written hive leaf store. Non-blocking for phase 2.

While in this file for phase 3: write.py:502's "collects the (small) ragged payloads" is the last echo of the unquantified memory claim that c0f6462 correctly replaced at the hive.py accumulation site — worth pointing that phrase at the quantified comment rather than re-asserting "small".

---

Generated by Claude Code

[espg]

🤖 from Claude

Phase 3 instructions (docs + inventory) — for the agent picking this phase up

Context: phases 1–2 are on this branch (claude/209-vlen-ragged); a fold review: read_cell bounds + single index fetch (issue #209) commit is landing shortly from the round-2 fold — start from the branch head after that commit is pushed (git fetch origin claude/209-vlen-ragged and confirm the fold commit is present before branching your work). Read CLAUDE.md at the repo root first and follow it exactly (title-only commit, no attribution in commits/PR text, ruff line-length 100, terse reviewable diffs). Continue on THIS branch — do not open a new PR. One commit: phase 3 of issue #209.

Scope:

1. Ragged-layout documentation under docs/ (place it the way docs/hive_layout.md is placed and linked — likely a sibling page or a section of it; wire into the mkdocs nav the same way): the vlen-bytes layout end to end — the wire framing (u32 count + per-element u32 length + payload, golden-pinned in tests/test_processing.py), the attrs contract (ragged.spec = "zagg-ragged/1", ragged.element, ragged.locations — exact key names as implemented after the fold commit), sharded vs per-inner-chunk layouts and when each applies (the Q1 resolution: unsharded flat K>1 keeps per-inner-chunk objects to preserve issue Stream-and-free chunk writes in the multi-chunk worker (don't accumulate all K) #91 stream-and-free; both are self-describing via array metadata), the hive-leaf reader contract (leaf carries the morton sibling + ragged attrs under the same group path), read paths (whole-store sweep; read_cell 2-GET random access), and the issue Typed vlen-array<float32> Zarr data type + codec (follow-up to the issue 209 vlen-bytes layout) #210 typed-dtype migration note (metadata-only, guaranteed by the golden framing pin).
2. notebooks/tdigest_reader_example.ipynb: update to the new reader API. Per CLAUDE.md §4: runnable on Binder, data must be publicly reachable or in-tree (keep the notebook's existing data-source approach), outputs cleared, nbformat-valid, every code cell ast.parse-clean, stable cell ids (the PR End-to-end sponsor example: catalog → shardmap → dual aggregation → cached reads #169 precedent).
3. Stale-CSR inventory: fix residual docstrings/comments in src/ that still describe the CSR-subgroup layout (flagged across the round-1/2 reviews; grep for CSR, read_csr, iter_csr_cells, subgroup in src/ and reconcile each hit — update or justify).
4. Benchmark harness check: if .github/scripts/run_benchmark.py (app-level harness — editable per the PR Morton decimal shard ids + hive-partitioned output layout (issue 199) #205 precedent) references CSR object counts or the old ragged layout, update it. Do NOT touch .github/workflows/ or deployment/aws/ build/standup scripts.
5. PR body: check phase 3 in the checklist; add a "Post-merge validation" heading noting the o8 re-test — the o8 900 s timeout observed in the order sweep should clear with this layout; re-running the o8/o9/o10 sweep (espg-side, credentialed) feeds the Benchmarks: full-AOI NEON runs, re-pin temporal to full catalog (→ 2026-03-15), reset live series, CONUS cost model #202 cost model its new row.

Definition of done: full local green — uv sync --extra test, uv run ruff check src tests, uv run ruff format --check src tests, uv run pytest -q (foreground), codespell via pre-commit run --files <changed> on docs, nbformat validity for the notebook. Push (plain push, never force). Post ONE PR comment starting 🤖 *from Claude* summarizing what phase 3 changed, with file links. Leave the PR in draft; do not mark ready-for-review; do not merge. A fresh-context adversarial review round follows the push — expect findings on doc-vs-implementation accuracy, and fold or answer them.

---

Separately, answering espg's thread-adjacent question about worker threading in this PR — see the clarification below (posted next).

---

Generated by Claude Code

[espg]

🤖 from Claude

Clarification (espg asked whether this PR removes "the k workers for the multithreaded shard agg/write") — there are three distinct "K/worker" concepts in the shard path, and this PR touches exactly one:

1. granule_workers (K=4, issues Granule-level read concurrency: the worker's serial granule x group loop is the dominant read-wall term #180/Granule-level read concurrency (K in flight, ordered fold) #183) — UNTOUCHED. The granule-level read/aggregation concurrency inside process_shard (threads fetching and processing granules in parallel, dispatcher-clamped per shard) is completely unaffected. Aggregation parallelism is exactly what it was.
2. The issue-ShardingCodec doesn't consolidate ragged (tdigest) writes — K serial CSR groups dominate shard write time #142 write-fanout thread pool — REMOVED, deliberately. That pool existed solely to parallelize the ~1,792 tiny per-inner-chunk CSR object PUTs at the end of a shard. With the ragged field now written as ONE slab into one sharded object, there is nothing to fan out — the pool, _write_ragged_fanout, and the issue-Sharded-path parallel CSR writes race under real CPU parallelism (fails 3/3 on 6 GB workers, largest shard) #186 failure roll-call (which existed to report per-object failures from that fanout) are all deleted. The write is now a single serial encode + one PUT, and it is much faster than the pool it replaces (0.44 s vs 12.1 s on the sparse NEON o8 fixture; 0.12 s vs 1.6 s dense — from the issue t-digest ragged output bypasses the sharding codec (~1,792 objects/shard) — needs a sharding-codec-like path #209 head-to-head).
3. K = chunk_inner inner chunks (256/shard) — RETAINED. Inner chunks still exist as the layout's random-access quantum; they just live inside the one sharded object (with its internal index) instead of being separate S3 objects.

So: aggregation stays multithreaded; the write-side threading disappears because the problem it solved disappeared.

---

Generated by Claude Code

[espg]

🤖 from Claude

Round-2 review findings folded in 97c788f (fold review: read_cell bounds + single index fetch (issue #209)). Per-finding disposition:

1. should-fix — read_cell bounds: now raises a pointed IndexError naming the valid cell range (no negative-index wrap, no bare unpack crash from zarr's slice clamping). Tested both directions.
2. should-fix — sweep index re-fetch: the whole-store sweep no longer re-fetches the shard-index suffix once per inner chunk (was ~K redundant sequential GETs/shard at production geometry); each stored span is read once at the already-accepted memory bound. Counting-store test pins the fixed GET count.
3. should-fix — untested writer-guard raising branches: the guards that replaced write_csr's checks now have tests exercising each raising branch (the earlier "already under test" claim was wrong on that half — corrected).
4. question folded as hardening — attrs version marker: added RAGGED_SPEC = "zagg-ragged/1" to the attrs["ragged"] block (writer emits it, reader version-gates on it with a pointed "newer writer's layout" error) — the coverage-envelope discipline, and the exact seam issue Typed vlen-array<float32> Zarr data type + codec (follow-up to the issue 209 vlen-bytes layout) #210's typed-vlen-array<T> dtype migration replaces (commented as such). A garbage element.dtype now raises a pointed error instead of a raw numpy TypeError.
5. nits: stale "(small)" memory wording fixed; hive-leaf reader contract (leaf carries the morton sibling + ragged attrs under the same group path) documented in the reader docstring.

Verified before push: affected suites and the full suite green (1780 passed, 13 skipped); ruff clean on all changed files (base.py, write.py, tdigest_tensor.py, test_processing.py, test_readers.py); the only ruff/format findings repo-wide are the pre-existing main ones (N818 in registry.py, format drift in test_extract.py/test_integration.py et al.), untouched.

Note: this fold commit was completed and verified but its original authoring session ended before pushing; it was pushed from the verified worktree state. Phase 3 (docs + inventory) is unstarted and picked up next per the instructions above. PR stays draft.

---

Generated by Claude Code

[espg]

🤖 from Claude

Phase 3 (docs + inventory) landed in 71213a0, on top of the round-2 fold 97c788f. What changed:

New narrative doc — docs/ragged_layout.md, wired into the mkdocs Design nav next to sharding.md/hive_layout.md. Covers the layout end to end: the on-disk arrays ({field} / {field}_locations / morton), the attrs["ragged"] contract (spec = "zagg-ragged/1", element, locations — the exact RAGGED_SPEC/RAGGED_ELEMENT_ATTR keys), the golden-pinned wire framing (u32 cell_count then per cell u32 length + payload, little-endian numcodecs VLenBytes/VLenArray framing, tied to test_golden_inner_chunk_framing), sharded vs per-inner-chunk layouts with the Q1 rationale (unsharded flat K>1 keeps per-inner-chunk objects for the #91 stream-and-free bound; both self-describing, one reader path), the hive-leaf reader contract, the two read paths (whole-store sweep; read_cell 2-GET), and the issue #210 typed-dtype migration note (metadata-only, guaranteed by the framing pin).

Notebook — notebooks/tdigest_reader_example.ipynb rewritten to the new reader API: builds the synthetic store through the production template + write_ragged_to_zarr (was zagg.csr.write_csr/read_csr/iter_csr_cells, all deleted), peeks a cell with read_cell, keeps its in-memory Binder-runnable data source and stable cell ids, outputs cleared. Verified nbformat-valid, every code cell ast.parse-clean, and executes end-to-end (the one matplotlib cell ast-parsed only where the extra is absent).

Stale-CSR sweep across src/ — reconciled present-tense CSR references in config.py, processing/{worker,aggregate,streaming}.py, grids/{morton,healpix,base}.py, stats/tdigest.py, and the shipped config YAMLs (atl03_tdigest_healpix, atl03_tdigest_located_healpix, atl03_gain_bias_healpix): "the CSR writer" → the ragged/vlen writer, "companion CSR array" → companion vlen array, "CSR subgroup names" (as a current external surface) → hive leaf ids, and the located config's stale {field}/{shard}/locations on-disk description → the {field}_locations sibling. Left standing as deliberate historical contrast (each accurately describes what the layout replaced, not current state): the "replaces the per-inner-chunk CSR subgroups" / "pre-issue-209 CSR stores are a hard break" / "the CSR layout named its subgroups by" / "write_csr enforced" notes in write.py, readers/tdigest_tensor.py, grids/base.py, grids/healpix.py, grids/rectilinear.py.

Benchmark harness — .github/scripts/run_benchmark.py carries no CSR object-count / ragged-layout references (grep-checked), so it needed no change; .github/workflows/ and deployment/aws/ untouched.

PR body — phase 3 checked; a Post-merge validation heading records the o8 900 s-timeout re-test (o8/o9/o10 sweep, espg-side credentialed, feeding issue #202's cost model and checking the ~200 MB hive-accumulation envelope).

Green locally: uv run pytest -q → 1765 passed, 28 skipped (skips optional-dep-gated / pre-existing); ruff check + ruff format --check clean on all touched files; codespell clean on the changed docs. Pre-existing findings left standing and named in the PR body (N818 in registry.py; format drift in six untouched files; codespell gir/anc on lines this PR did not touch; the types-PyYAML mypy-hook gap). PR stays draft.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

Fresh-context adversarial review of the newest commit 71213a0 (phase 3 of issue #209 — docs page + notebook rewrite + stale-CSR docstring sweep). Docs phase, so I checked every technical claim against the code at head. No blocking findings. The page is accurate to the implementation; the notebook runs end-to-end; the nav wires up and builds. One inline nit on the GET-count table plus the small items below.

What I verified (all check out)

• attrs contract — docs/ragged_layout.md claims spec/element/locations under the ragged key with spec == "zagg-ragged/1". Exact match: grids/base.py has RAGGED_ELEMENT_ATTR = "ragged", RAGGED_SPEC = "zagg-ragged/1", and ragged_array_spec emits {"spec": RAGGED_SPEC, "element": {"dtype": str(...), "shape": [-1, *inner_shape]}} (+ optional "locations") at the single choke point. Doc example (shape [-1, 2], "locations": "h_tdigest_locations") matches what the located config produces.
• Wire framing — doc's u32 cell_count then per-cell u32 length + payload (LE, VLenBytes/VLenArray) is byte-for-byte the golden vector in test_processing.py::TestRaggedVlenLayout::test_golden_inner_chunk_framing (struct.pack("<I", len(cells)) + b"".join(struct.pack("<I", len(c)) + c ...)). The #210 "metadata-only migration" claim follows correctly from that pin — a typed vlen-array<f32> frames u32 len + array.tobytes(), identical bytes to the current u32 len + ascontiguousarray(value).tobytes().
• read_cell / sweep GET claims — see inline. Sharded 2-GET is exact and test-pinned; unsharded 1-GET is analytically correct but not test-pinned. Neither count includes the array-open metadata read. Not blocking.
• Symbols the doc names all exist: _open_ragged strict-spec-gate (raises on unknown spec), _stored_chunk_spans, ragged_array_spec(shard_shape=…), write_shard_to_zarr / write_ragged_leaf_to_zarr, hive.shard_leaf_path, hive._ZSTD_LEVEL == 3. Shard footer "K (offset,nbytes) u64 pairs + crc32c" and the 2^64-1 empty-chunk sentinel match the sharding index codecs + the 16*K+4 suffix assertion.
• mkdocs nav — built the docs at 71213a0 in a throwaway worktree: ragged_layout/index.html renders, the in-page anchor #issue-210-typed-dtype-migration resolves to an existing id, and hive_layout.md renders to ../hive_layout/. No broken-link/nav warnings for the new page (the only build errors are pre-existing network-blocked objects.inv inventory fetches, unrelated).
• Notebook — ran all 10 code cells end-to-end under zagg[test,analysis] against the phase-2 runtime (phase 3 is docs/comment-only in src, so runtime is identical): builds a store via the production template + write_ragged_to_zarr(ragged, store, grid=, chunk_idx=), read_cell(store, FIELD, base+5), read_tensors, read_raw_values, all fit policies — every symbol it imports exists and every cell executes with sensible output. nbformat 4.5, unique stable cell ids, outputs cleared, all code cells ast.parse-clean. Binder claim holds: .binder/postBuild installs zagg[analysis,catalog,viz], and matplotlib>=3.10.8 is in the analysis extra; data source is in-memory rng, no credentialed calls.
• Config-YAML edits — accurate to what the writer produces: the located config's old {field}/{shard}/locations on-disk description is correctly replaced by "the {field}_locations sibling … row-aligned", and the three sharded:-block "K CSR writes" comments → "K per-inner-chunk writes".
• src diff is truly comment-only — every hunk in config.py/aggregate.py/streaming.py/worker.py/grids/{base,healpix,morton}.py/stats/tdigest.py is inside # comments or docstrings; no runtime line changed. No CI/workflow/deploy edits. The reconciled present-tense CSR references now read correctly.

Minor items (non-blocking)

• nit (notebook, markdown "read_tensors" section): the prose says the chunk morton id is "recovered from the sibling {field}/morton coordinate array". With FIELD = "12/h_tdigest", the backtick path {field}/morton reads as a child of the field array (12/h_tdigest/morton), which doesn't exist — the array is the field's sibling at {group}/morton (12/morton). The word "sibling" is right; only the path template contradicts it. The Summary cell gets it right ("the sibling morton coordinate"). Trivial wording fix.
• observation (stale-CSR sweep, out of this commit's diff): the "deliberately left" historical-contrast CSR mentions I spot-checked are genuinely historical and don't misdescribe current behavior — grids/base.py:67 ("the CSR layout's fourth in-group array cannot nest…"), readers/tdigest_tensor.py:355 ("the same parent cell the CSR layout named its subgroups by"), and the "replaces the per-inner-chunk CSR subgroups" notes are all accurate. The two write.py docstrings that credit an invariant to write_csr ("the guard write_csr enforced structurally", L551; "the per-cell length contract write_csr enforced", L576) are past-tense and don't mislead about current behavior, but write_csr was deleted in phase 2, so a maintainer greps a symbol that no longer exists while the live guard sits inline in the same function. Since write.py isn't in this commit's diff it's out of scope to fold here — noting it only for completeness; reword to point at the inline check if the file is touched again.

2-GET / 1-GET doc-claim verdict

Both directions of the table are factually correct. Sharded single-cell read_cell = exactly 2 ranged data-object GETs (shard-index suffix + one inner chunk), and this is hard-pinned by test_two_ranged_gets_on_sharded_store. Unsharded per-inner-chunk read_cell = 1 data-object GET (a regular vlen array indexes the single chunk holding the cell) — correct, but analytic, not test-pinned. Both counts exclude the one-time array-open metadata read. Recommend the two half-sentence caveats in the inline comment; no correctness problem.

CI state @ 71213a0

ruff success; build / build-x86_64 / build-arm64 success; deploy / pr-preview skipped; test (3.12) and test (3.13) in_progress at review time. The PR body reports the full suite green at this head (1765 passed / 28 skipped) with ruff/format/codespell clean on the changed docs and touched lines; the two test jobs were still running when I pulled check-runs.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

nit (GET-count table — one column is pinned, one is asserted): I verified both cells against the code. The sharded 2 GETs is exact and test-pinned — tests/test_readers.py::test_two_ranged_gets_on_sharded_store filters data_gets to /h_tdigest/c/ and asserts len == 2, with the index-suffix span == 16*chunks_per_shard + 4 and the inner-chunk read <= obj_size // 4. The 1 GET for the per-inner-chunk regular array is correct in principle (a regular vlen array indexes exactly the one chunk holding the cell → one data-object GET) but has no equivalent test pin — the only unsharded read_cell test (test_out_of_range_and_negative_raise_pointed) checks bounds, not GET count. Two half-sentences would keep the table honest: (a) note the 1-GET column is the analytic expectation rather than a counting-store assertion, and (b) note both counts are data-object GETs and exclude the one-time array-open metadata read (zarr.json/attrs in _open_ragged), so a reader tallying raw S3 requests isn't surprised. Non-blocking; the numbers are right.

---

Generated by Claude Code

[espg]

🤖 from Claude

Final-review nits folded in 788922c (fold review: doc GET-count caveat + notebook morton path (issue #209)):

1. nit — GET-count table (docs/ragged_layout.md): added the two caveats the reviewer asked for — the counts are data-object GETs only and exclude the one-time array-open metadata read (amortized across a store's cells), and the sharded 2-GET count is test-pinned (test_two_ranged_gets_on_sharded_store) while the unsharded 1-GET count is analytic.
2. nit — notebook morton path (notebooks/tdigest_reader_example.ipynb): the "read_tensors" markdown said "sibling" but wrote the path template {field}/morton, which with FIELD = "12/h_tdigest" reads as a child (12/h_tdigest/morton); corrected to {group}/morton (sibling to the field under the same group), matching the code's {grid.group_path}/morton. The two cells that already said "sibling morton" were correct and untouched.
3. observation (out of this diff) — the reviewer noted two write.py docstrings (L551/576) crediting an invariant to the now-deleted write_csr symbol: past-tense and not misdescribing current behavior, but referencing a symbol a maintainer can't grep. Left standing for a future touch (write.py isn't in the phase-3 diff) rather than expanding scope.

Verified: notebook nbformat-valid (4.5, 21 cells, all 10 code cells ast.parse-clean, ids unique, outputs cleared), no stray {field}/morton remains, codespell clean on both files. Docs/notebook-only change — no runtime code touched.

All three phases complete; three adversarial-review rounds folded. Applying waiting.

Standing for review (nothing blocking): the pre-existing main lint findings this PR deliberately doesn't touch (N818 in registry.py; ruff format drift in test_extract.py/test_integration.py + ~4 others; codespell gir/anc; the types-PyYAML mypy-hook gap), and the Post-merge validation in the PR body — the o8 900 s-timeout re-test (o8/o9/o10 sweep, credentialed, feeding #202's cost model), which is the real-world confirmation that collapsing the K×7 write storm clears the coarse-order timeout.

---

Generated by Claude Code