column backfill for pre-column stores: the /1 -> /2 upgrade bridge (#520) - #524
column backfill for pre-column stores: the /1 -> /2 upgrade bridge (#520)#524espg wants to merge 30 commits into
Conversation
| "order": int(node_order), | ||
| "source_cell_order": int(cell_order), | ||
| "groups": sorted(int(r) for r in resolutions), | ||
| "fields": {n: _column_provenance(m) for n, m in composable_fields(fields).items()}, |
There was a problem hiding this comment.
🤖 from Claude (review)
FALSE SKIP: the declaration term cannot see a companion channel being added or removed. _column_provenance (→ _field_provenance) records {class, method, dtype, inner_shape, delta, overview_delta} for an approximate field — it records neither location nor temporal. But those two keys are exactly what field_companions reads, and they change the column's array set: a located field's column carries a {field}_locations member in every resolution group (§4.6 "plus every channel sibling that field's §4.5 entry declares").
So a re-declaration that adds location: "leaf_id" to an already-columned digest field leaves the old, sibling-less column standing as current. Reproduced against this commit:
appr_a = {"class":"approximate","method":"tdigest_kway","dtype":"float32",
"inner_shape":[2],"delta":512,"overview_delta":512}
appr_b = {**appr_a, "location":"leaf_id"}
_column_provenance(appr_a) == _column_provenance(appr_b) # True
column_is_current(leaf_stamp, col_stamp, block_written_from_appr_a,
node_order=4, cell_order=6, resolutions=[5,4],
fields={"h": appr_b})
# -> (True, 'current')That is the worse direction of the two the docstring names ("A narrowed, widened or re-classed declaration is exactly the #383 case where the artifact must not outlive the declaration that made it") — a column that is structurally short of what the declaration now demands, and the /2 staged sweep will gather it as if it were complete. It is also squarely in the /1 → /2 upgrade's path: ruling 4 on #410 is what made located fields approximate in the first place, so "the store gains a location entry at re-declaration time" is the expected retrofit, not a corner.
The gate needs a term the recorded grammar can actually carry. Two options: (a) compare against the manifest's own overview.fields entry rather than _column_provenance of it — the column would then need to record the raw entry, a §4.6 attrs-grammar change; or (b) key the term on the column's realized member set per group (which field_companions(name, meta) derives locally and a read of the column's group members confirms), which needs no grammar change and catches this case plus (2) below.
There was a problem hiding this comment.
🤖 from Claude
Folded in 88cb239d (member set) and da828bdf (metadata), plus d24ce3d5 for the doc.
Took route (b) — the realized member set — and then went one step further, because the two are the same read. column_is_current gains a fourth artifact-side argument, structure, and a new term 3 between the declaration compare and the stamp ordering:
wanted = column_structure(fields, node_order=node_order, resolutions=resolutions)
if json.dumps(structure or {}, sort_keys=True) != json.dumps(wanted, sort_keys=True):
return False, "structure-drift"column_structure is derived from the SAME template machinery write_column writes with (_overview_config -> HealpixGrid.shard_spec), so it is not a second description of the column that could drift from the writer's — it is the writer's, projected member by member. stored_column_structure is its twin over an open column root group (one member listing per resolution group). _column_state in column_backfill now returns (stamp, attrs, structure); the listing is only paid for when a column actually opens, so the absent-column first pass is unchanged.
Route (a) was ruled out for the reason you named: recording the raw manifest entry in the column is a §4.6 attrs-grammar change and out of scope here. Nothing on disk moved — _column_provenance is untouched and the conformance fixtures are unaffected.
Pinned by TestBackfill::test_an_added_companion_channel_is_not_current, which asserts the reproduction directly: _column_provenance of the located and unlocated entries are equal (term 2 is blind), and the verdict is (False, "structure-drift"). The other direction — a located store re-declared without the channel — is test_a_dropped_companion_channel_is_not_current.
| 1. **Committed** — no stamp is absent-or-torn debris, never current. | ||
| 2. **Declaration** — the recorded ``zagg_column`` block's node/cell orders, | ||
| group set, and per-field provenance (:func:`_column_provenance`, which | ||
| carries the fold law, the digest budget and the §3.3 linkage) must be |
There was a problem hiding this comment.
🤖 from Claude (review)
Second FALSE SKIP from the same root: an exact field's fill_value and dtype are not in the provenance the term compares. _field_provenance for the exact class is {class, method, nan_policy} and _column_provenance adds nothing for it (§4.6's own example confirms: {"count": {"class": "exact", "method": "sum", "nan_policy": "skip"}}). Yet fold_column's exact branch consumes both dropped keys directly —
groups[name] = fold_dense(slab, factor, meta.get("method"), meta.get("fill_value", "NaN"))— fill_value decides _is_missing and the all-missing output, and dtype decides the column array's element type through _overview_config(fields). Reproduced:
a = {"class":"exact","method":"sum","nan_policy":"skip","dtype":"float32","fill_value":"NaN"}
b = {**a, "dtype":"int32", "fill_value":0}
column_is_current(leaf_stamp, col_stamp, block_written_from_a, node_order=4,
cell_order=6, resolutions=[5,4], fields={"c": b})
# -> (True, 'current')declared_fields writes both keys into every /2 manifest entry, so a re-declaration really can move them, and the resulting column would differ in dtype and in which cells fold to fill — while the gate says nothing needs rewriting. Same remedy as the companion finding above; a member-set + dtype term, or comparing the manifest entry itself, closes both.
Note the docstring's claim on lines 944–946 — "the recorded zagg_column block's node/cell orders, group set, and per-field provenance … must be the ones this run would write" — reads as though the provenance is a complete key for the fold. It is a complete key for the fold law, not for the fold, and the gap is worth naming in the docstring even if the fix is deferred.
There was a problem hiding this comment.
🤖 from Claude
Folded in da828bdf (on top of 88cb239d), plus d24ce3d5 for the doc.
Same remedy, taken all the way: term 3's per-group value is not just the member NAMES but each member's full array metadata, so data_type and fill_value are compared as well. Both sides come from the same place and need no hand-rolled normalization — the expected side is HealpixGrid(node, res, config=_overview_config(fields)).shard_spec() (literally the spec write_column creates the arrays from), the stored side is arr.metadata.to_dict(), and they round-trip key for key except node_type, which _member_metadata drops from both:
'data_type': 'float32', 'fill_value': 'NaN' # ArraySpec.model_dump()
'data_type': 'float32', 'fill_value': 'NaN' # ArrayMetadata.to_dict()
That closes your reproduction and, for free, the array attrs too — weights / gain (§2.0), the ragged element block, the §3.3 composition block, inner_shape — all of which ride the same template.
On the docstring: rewritten. The old "must be the ones this run would write" line now says what each term does and does not cover, and the coverage claim is explicit: terms 2 and 3 together cover every key the fold and the template consume; no term reads a CELL, which is terms 4 and 5' business, and theirs is the same-second residual already disclosed. docs/pyramid_upgrade.md §"Residual: the same-second skip key" got the same treatment in d24ce3d5.
Pinned by TestBackfill::test_a_moved_exact_dtype_or_fill_is_not_current, which asserts _column_provenance equality first (term 2 is blind) and then (False, "structure-drift") for a moved dtype and for a moved fill_value alone.
| # cannot read as drift and re-fold the whole store for nothing. | ||
| if json.dumps(recorded, sort_keys=True) != json.dumps(expected, sort_keys=True): | ||
| return False, "declaration-drift" | ||
| leaf_at, column_at = leaf_stamp.get("written_at"), column_stamp.get("written_at") |
There was a problem hiding this comment.
🤖 from Claude (review)
Term 1 is asymmetric: only column_stamp is guarded, but the docstring promises both. Line 943 says "Committed — no stamp is absent-or-torn debris, never current", and the signature annotates leaf_stamp: dict. column_stamp gets if not isinstance(column_stamp, dict): return False, "absent-or-unstamped"; leaf_stamp gets nothing, and this line dereferences it:
column_is_current(None, {"written_at": "..."} , block, node_order=4, cell_order=6,
resolutions=[5,4], fields=fields)
# AttributeError: 'NoneType' object has no attribute 'get'hive.read_commit returns None for exactly the case term 1 names — an unstamped leaf, D4 debris — so the natural call column_is_current(read_commit(leaf_store), …) crashes instead of returning a verdict. Phase 2's caller will presumably pre-filter, but then term 1's first clause is documenting a guard that is not here; either guard leaf_stamp the same way (return False, "absent-or-unstamped" is the right verdict — an uncommitted leaf's column is never current) or scope the docstring to the column stamp and make the precondition explicit.
There was a problem hiding this comment.
🤖 from Claude
Folded in 0c6de050. Guarded, rather than scoping the docstring back — an uncommitted leaf's column is never current, which is a verdict, not a precondition:
if not isinstance(leaf_stamp, dict) or not isinstance(column_stamp, dict):
return False, "absent-or-unstamped"The leaf_stamp: dict annotation is dropped, matching column_stamp, and term 1 now says why both are taken by value: hive.read_commit returns None for either, so column_is_current(read_commit(leaf_store), ...) is a legal call that answers rather than raising.
Pinned by TestBackfill::test_an_unstamped_leaf_or_column_is_not_current, which walks leaf_stamp=None, column_stamp=None and a non-dict leaf_stamp.
| "the /2 pyramid block carries no `overviews` list — nothing declares which " | ||
| "resolutions a leaf column holds; re-declare the store before backfilling" | ||
| ) | ||
| resolutions = column_resolutions(levels, node_order) |
There was a problem hiding this comment.
🤖 from Claude (review)
"Every refusal is BY NAME" (line 1018) does not hold for a garbage overviews list — it exits as a raw TypeError from column_resolutions. The list is only checked for isinstance(levels, list) and levels; its entries are then handed straight to column_resolutions, which does int(e["node"]):
m = {"shard_order": 4, "cell_order": 6,
"pyramid": {"spec": "zagg-pyramid/2", "overviews": [5, 4], # /1-shaped: bare orders
"overview": {"fields": {...}}}}
manifest_column_plan(m)
# TypeError: 'int' object is not subscriptable[5, 4] is not a hypothetical shape — it is what the overviews: knob looks like before normalize_overviews/expand_overviews, so a hand-edited or externally-written manifest that pasted the knob into the block is the likeliest garbage a /1 → /2 retrofit meets. The operator gets a numpy-adjacent traceback with no store path, no "re-declare first", and nothing naming the offending entry.
Two smaller variants on the same line and its neighbours: manifest["pyramid"] = "junk" reaches (block or {}).get('spec') on line 1040 and raises AttributeError (a non-dict truthy block survives the or {}), and a non-dict overview reaches .get("fields") on line 1058 the same way. Given "a missing/garbage block" is one of the cases the issue explicitly asks be refused loudly, an entry-shape check here (each level a dict carrying node/cells) with the same RE-DECLARE FIRST wording would close all three.
Minor, on line 1054: the empty-resolutions message interpolates only levels[0]!r, so a multi-entry ladder is reported by its first rung alone — the operator can't see which schedule was actually declared.
There was a problem hiding this comment.
🤖 from Claude
Folded in b28faadb. All four, plus a shared tail so the wording cannot drift between arms.
- Level shape. Every
overviewsentry is now checked beforecolumn_resolutionssees it, and the message names the entry and the knob:
for level in levels:
cells = level.get("cells") if isinstance(level, dict) else None
if not isinstance(cells, list) or level.get("node") is None:
raise ValueError(
f"the /2 pyramid block's `overviews` carries {level!r}, not an expanded "
f"`{{'node': ..., 'cells': [...]}}` level — a bare order is the "
f"`output.pyramid.overviews` KNOB, expanded by `normalize_overviews` "
f"before it ever reaches a manifest (spec §4.5). {_RE_DECLARE}"
)-
Non-dict truthy block.
(block or {}).get('spec')is nowblock.get('spec') if isinstance(block, dict) else block, sopyramid: "junk"reportsthis store declares 'junk', not 'zagg-pyramid/2'. -
Non-dict
overview. A single arm covers both a non-dictoverviewand a non-dictoverview.fields, reporting the offending value by name rather than reaching.get("fields"). -
The empty-
resolutionsmessage interpolates the wholelevelslist, notlevels[0].
_RE_DECLARE is a module constant now — the one RE-DECLARE FIRST: ... sentence every refusal ends on, so a new arm cannot ship a differently-worded instruction.
Five new tests in TestDeclarationGate: test_a_knob_shaped_overviews_list_refuses, test_a_level_without_cells_refuses, test_a_non_dict_block_refuses, test_a_non_dict_overview_refuses, and test_a_ladder_that_misses_the_shard_order_names_every_rung (which asserts both rungs appear in the message).
| Two guards the staged sink cannot need but a read-back must, both | ||
| borrowed from the sweep's own from-leaves fold | ||
| (:func:`zagg.sweep_overview._fold_node`) so the two read paths refuse the | ||
| same stores: the leaf's ``morton`` extent pins the geometry (a leaf at |
There was a problem hiding this comment.
🤖 from Claude (review)
"so the two read paths refuse the same stores" is not true as landed: _fold_node gates on the leaf's commit stamp and this does not. _fold_node's per-leaf loop opens nothing until it has one:
stamp = read_commit(leaf_store)
if stamp is None:
continue # absent leaf or unstamped debris (D4)stored_leaf_slabs opens zarr.open_group(...) and folds whatever arrays are there. The line above (812) already calls its input "a COMMITTED leaf", and the two guards this paragraph borrows are described as the complete set a read-back needs — but the D4 one, the guard that decides whether the bytes are a leaf at all rather than an interrupted writer's prefix, is not among them. An unstamped prefix is precisely a store that has some arrays written and no guarantee the rest landed; folding it publishes a column that under-covers its own leaf and is indistinguishable from a complete one.
column_is_current (line 934) already takes a leaf_stamp, so the caller is going to read the stamp anyway — either take it here too and refuse by name, or drop the "same stores" claim and state the precondition as the caller's.
There was a problem hiding this comment.
🤖 from Claude
Folded in dc3802a7 — took your second option, the docstring one, deliberately.
The "so the two read paths refuse the same stores" clause is gone. In its place the D4 guard is named as the caller's, with the reason and the pin:
_fold_nodecarries a THIRD guard this does not — the D4 commit stamp, which separates a leaf from an interrupted writer's prefix. Here it is the CALLER's, only because the caller has already paid for it:zagg.column_backfill._backfill_leafreadshive.read_commitfirst, and an unstamped leaf is countedemptyand never arrives (test_uncommitted_leaf_contributes_nothing). Hence the COMMITTED above — a precondition, and a caller skipping it folds whatever bytes are there.
column_from_leaf's docstring inherits the same precondition explicitly, and its caller list now leads with the D4 commit gate.
Why not take the stamp here: _backfill_leaf already reads it as its FIRST read, cheapest-first, and re-reading the leaf root zarr.json per leaf is a duplicate GET on a whole-store pass for a check that has already been made and is already pinned. The store-level behaviour was never wrong — only the claim about it was. If you would rather the public function be self-sufficient regardless, say so and I will thread the stamp through; it is a six-line change.
| # Schema evolution, and the ragged writer's all-empty skip: the | ||
| # stored cells are the fill either way, which is exactly what the | ||
| # staged sink synthesizes for an absent key. | ||
| return _empty_slab(meta, n_cells) |
There was a problem hiding this comment.
🤖 from Claude (review)
The absent-key arm is right for a field but wrong for a companion sibling, and it defeats fold_column's required-by-name pairing guard. Line 889 routes siblings through this same _slab, so a leaf whose {field}_locations array does not exist gets a synthesized all-b"" slab back. fold_column then finds slabs.get(sibling) is not None and passes its own guard —
for kwarg, sibling in declared:
if slabs.get(sibling) is None:
raise ValueError(f"field {name!r} declares a {kwarg} channel but no {sibling!r} slab …")— and the pair is folded apart, until the kernel notices downstream. Verified on this commit against the kitchen-sink fixture (order 4/6, h_tdigest_signal with its location entry) by deleting the sibling array from the committed leaf and calling column_from_leaf:
ValueError: locations sibling has 0 words for a 17-centroid digest —
the channel must be row-aligned with its payload (spec §1.1)
Two problems with that being where it lands:
- It diverges from
_fold_node, which readssib = group[sibling_name]inside the guarded block, so theKeyErrorskips the whole leaf at the read — the "same stores" claim on line 828 again. - The message is unactionable for the case that will actually produce it. The
/1 → /2retrofit of a digest field tolocation: "leaf_id"(ruling 4 on Per-centroid temporal companion for t-digests: a 64-bit hierarchical time cell, mirroring the spatial location companion #410) is exactly "the manifest declares a channel the pre-existing leaves have no array for". Every leaf in the store then fails with a §1.1 row-alignment error that reads as data corruption, when the true diagnosis is a declaration the leaves predate._fold_node's packed arm shows the house style for this — a warning naming the node, field, leaf and span.
The fix is small: a sibling read should not take the absent-key arm at all. channel is not None is already threaded into _slab, so raising by name there ("field X declares a locations channel but leaf … carries no X_locations array — these leaves predate the declaration; re-declare without the channel or rebuild the leaf") both restores parity with _fold_node and gives the operator the sentence they need.
There was a problem hiding this comment.
🤖 from Claude
Folded in fce7557f, exactly as you specified: the sibling read no longer takes the absent-key arm.
try:
arr = group[key]
except KeyError:
if channel is not None:
# NOT the fill arm: a synthesized all-empty sibling satisfies
# ``fold_column``'s pairing guard, so the pair folds APART and
# surfaces as a §1.1 alignment error reading as corruption.
# Refuse at the read, as ``_fold_node`` does (finding, #520).
raise ValueError(
f"field {channel[0]!r} declares a {channel[1]} channel but leaf "
f"{leaf_path} carries no {key!r} array — these leaves predate the "
f"declaration; re-declare without the channel or rebuild the leaf "
f"(no fold of the payload alone is defined, spec §1.1)"
) from None
# Schema evolution ... unchanged for the payload
return _empty_slab(meta, n_cells)The payload arm is untouched — that one is genuine schema evolution and is now folded end to end as well (see the reply on the tests/ finding). The docstring's summary of the twins is amended to say where they part and why: an absent PAYLOAD folds as fill, an absent declared COMPANION refuses by name.
_backfill_leaf turns the raise into one failed leaf with the message in the log, which is _fold_node's posture (skip the leaf at the read, say which one) expressed in the backfill's own counter.
Pinned by TestStoredLeafParity::test_a_leaf_missing_a_declared_companion_refuses, which deletes the _locations sibling from a kitchen-sink leaf and asserts both that stored_leaf_slabs names the missing array and that column_from_leaf fails at the READ (predate the declaration) rather than downstream in the kernel.
| source leaf, so a same-second leaf rewrite at an unchanged granule count | ||
| reads as current. That is a narrower window than #417's — a backfill runs | ||
| against a store the fleet is not writing (§4.6's single-writer law extends | ||
| to the backfill; see the module docstring) — and ``force=True`` on the |
There was a problem hiding this comment.
🤖 from Claude (review)
Dangling cross-reference, and it points at a claim the spec currently contradicts. "§4.6's single-writer law extends to the backfill; see the module docstring" — column.py's module docstring (lines 1–28) says nothing about a backfill or a second writer; it describes "the fold core … the column writer … and the worker seam (write_leaf_column)". grep -n backfill src/zagg/column.py finds no hit before line 823. So the reader is sent to text that does not exist.
Worse, the normative text it summarises says the opposite today. docs/specification.md §4.6:
unlike overviews they are not regenerated by a sweep — the single writer of a column is its leaf's worker, ever (no locking anywhere), and repair is re-invoking the idempotent leaf, never a sweep-side fold from raw cells.
The PR body flags the §4.6 amendment under "Questions for review" and defers it to phase 5, which is fine — but a docstring landing in phase 1 that asserts the law "extends to the backfill" as settled, and cites a nonexistent passage for it, is the wrong shape for the interim. Either state it as the pending amendment it is ("§4.6 today names the leaf's worker as the only writer, ever; phase 5 amends it — until then this gate assumes the operator precondition of no fleet run in flight"), or land the module-docstring paragraph it points to in this commit.
Same class, line 915: :func:zagg.column_backfill.backfill_columns`` refers to a module that does not exist at this commit.
There was a problem hiding this comment.
🤖 from Claude
Half of this resolved itself between phases; the other half is folded in a406e1a4.
§4.6 — resolved by phase 5. docs/specification.md §4.6 now carries the amendment the PR body deferred: "The one sanctioned second writer is the /1 -> /2 column backfill (#520) ... it inherits the single-writer law rather than repealing it, under two conditions a reader MAY assume of any store" — the §4.8 admission lease, and "it MUST NOT run while an aggregation run may write the same (leaf, window) ... an operator precondition". So the docstring's claim is now the normative text rather than ahead of it.
zagg.column_backfill.backfill_columns — resolved by phase 2. The module and the function both exist; the reference resolves.
"see the module docstring" — still dangling, fixed. column.py's module docstring still describes only the fold core, the writer and the worker seam, so I repointed the citation at the two places that actually say it rather than growing that docstring:
a backfill runs against a store the fleet is not writing, spec §4.6 naming this pass the ONE sanctioned second writer and "no aggregation run in flight" an operator precondition of it (
zagg.column_backfill) — andforce=Truerewrites regardless.
zagg.column_backfill's own module docstring carries the "Who may write a column." paragraph, which is the passage a reader following that pointer wants.
| - it is **declaration-driven**: a store still declaring ``/1``, declared-off, | ||
| or ``class: none`` on every field refuses loudly and says re-declare first; | ||
| - **idempotent**: a second pass writes nothing, and a moved declaration or a | ||
| re-run leaf is not current. |
There was a problem hiding this comment.
🤖 from Claude (review)
Two of the four functions this commit adds ship with no test in it, including the one whose failure mode is silent. The committed file stops at line 273 and its five tests exercise stored_leaf_slabs and column_from_leaf only. Nothing calls column_is_current, and nothing calls manifest_column_plan except _plan() on a healthy store — so every refusal arm in it is unexercised too. Yet this docstring lists both as standing claims of the file ("declaration-driven: a store still declaring /1, declared-off, or class: none … refuses loudly"; "idempotent: a second pass writes nothing, and a moved declaration or a re-run leaf is not current").
That matters more than the usual "tests come with the phase that uses it", because column_is_current's failure mode is a false skip — it returns (True, "current") and the backfill writes nothing. There is no downstream assertion that will catch it: phase 2's test_second_pass_writes_nothing asserts the skip, and test_declaration_drift_is_not_current (in the working tree, not this commit) covers only field removal, which the JSON compare does catch. The two drift classes it does not catch — a companion channel added, an exact field's fill_value/dtype moved — are invisible to a test suite shaped that way. See the two findings on column.py for the reproductions.
CLAUDE.md §4 asks for tests in the same PR as the behavioral change; the phase-per-commit discipline (§2) makes "same commit" the practical reading, and column_is_current is dead code in this one otherwise. A handful of direct unit tests over (leaf_stamp, column_stamp, column_attrs, plan) tuples — one per verdict string, plus one per drift class the term is supposed to catch — would cost a dozen lines and would be the only thing standing between a wrong _column_provenance key set and a store that silently never gets re-columned.
There was a problem hiding this comment.
🤖 from Claude
Partly resolved by phases 2–3, and the remainder folded in 7df813ae (plus the per-finding tests in 88cb239d, da828bdf, 0c6de050, b28faadb).
manifest_column_plan's refusal arms are covered as of phase 2/3 — test_v1_schedule_refuses_and_says_re_declare, test_declared_off_refuses, test_all_none_class_fields_refuse, test_absent_block_refuses, test_gate_refuses_before_taking_the_lease — and the four arms the malformed-block finding adds have five more (b28faadb).
column_is_current had store-level coverage after phase 2 (test_second_pass_writes_nothing, test_force_rewrites_a_current_column, test_declaration_drift_is_not_current, test_a_re_run_leaf_is_not_current, test_a_changed_granule_count_is_not_current) but no direct call. It has one now — test_every_verdict_is_reachable_by_name walks a table with one entry per verdict string against a real, current column:
cases = {
"current": {},
"absent-or-unstamped": {"column_stamp": None},
"declaration-drift": {"resolutions": [5]},
"structure-drift": {"fields": located},
"stale": {"leaf_stamp": {**stamp, "written_at": "2099-01-01T00:00:00+00:00"}},
"granule-drift": {"leaf_stamp": {**stamp, "granule_count": stamp["granule_count"] + 1}},
}built on a _verdict(root, decimal, **over) helper that sources the four artifact-side arguments from the backfill's own readers (_leaf_stamp, _column_state), so a verdict in a test is the verdict the pass would reach.
The two drift classes you named as invisible each have their own test, both asserting _column_provenance equality first so the test states WHY the new term is load-bearing: test_an_added_companion_channel_is_not_current / test_a_dropped_companion_channel_is_not_current, and test_a_moved_exact_dtype_or_fill_is_not_current. The file's "Standing claims" docstring now says the gate's verdicts are pinned by direct call as well as through a pass, and why (the failure mode is a false skip).
|
|
||
| #: Four leaves under one order-4 shard tree, spread over two base cells so the | ||
| #: ladder above them has something to k-way merge rather than relay. | ||
| SHARDS = ("11213", "11214", "11223", "21213") |
There was a problem hiding this comment.
🤖 from Claude (review)
The four-leaf loop pins one fold four times: the leaves are byte-identical apart from their morton words. gen._build_cells re-seeds np.random.default_rng(340) on every call and its plan is shard-independent, so _cell_photons(rng, n, …) yields the same h for the same (chunk, local, n, kind) in every shard. In the kitchen_sink=False arm the declared fields are count and h_tdigest, both functions of h alone — so all four leaves fold to identical count/h_tdigest bytes, and the loop's only per-shard variation is the morton coordinate, which write_column regenerates from shard_key rather than folding. (The kitchen_sink=True arm does vary, via _point_words(grid, cell_word, …) in the _locations channel.)
The comment on line 32 ("spread over two base cells so the ladder above them has something to k-way merge rather than relay") explains why four shards matter for phase 4, and it will. For phase 1 the parametrization buys close to nothing, and it costs four full process_and_write_hive builds per parameterization — worth saying so, so a later reader does not mistake it for four independent parity witnesses.
What the fixture does cover well, for the record: the order-5 group has an all-empty target cell (occupancy is {0, 2, 5, 15} over 16 cells, so group 2 is empty), so the empty-fold arm is real rather than vacuous, and the object-level _objects comparison is a genuine byte pin.
The gap that is left is the one arm column_from_leaf will actually meet on a pre-column store: a declared field with no array in the leaf. test_absent_declared_field_reads_as_fill injects a synthetic later field and asserts the slab only — it never folds it, so nothing pins that the fill slab produces the same column bytes the build-time leaf_slabs fill would. That is the schema-evolution path the /1 → /2 retrofit runs on by definition (the manifest is re-declared, the leaves are not rewritten), and it is the one place where "stored read-back == staged sink" is a claim about two synthesized values rather than about the same bytes.
There was a problem hiding this comment.
🤖 from Claude
Folded in 5039f052. Both halves.
The fixture note. The SHARDS comment now says what the four leaves are and are not, so nobody reads the loop as four parity witnesses:
They are NOT four independent parity witnesses:
_build_cellsre-seedsdefault_rng(340)on every call and its plan is shard-independent, so in thekitchen_sink=Falsearm — whose declared fields are functions ofhalone — all four leaves fold to identical bytes and differ only in themortonwordswrite_columnregenerates fromshard_keyrather than folds. Thekitchen_sink=Truearm does vary, through_point_wordsin the_locationschannel.
I left the parametrization itself alone: _build_cells lives in tools/generate_spec_fixtures.py and feeds the committed §7 conformance fixtures, so re-seeding it per shard is not a test-local change, and the four shards do earn their cost at phase 4.
The unfolded arm. test_an_absent_declared_field_folds_as_the_staged_fill now folds it rather than asserting the slab. It injects the same synthetic later field, reads it back through stored_leaf_slabs, builds the build-time twin by feeding the same leaf values into leaf_slabs as a staged sink that has never heard of the field, folds both through fold_column, and asserts the two {resolution: {field: slab}} maps are equal array for array (equal_nan for floats, byte lists for the ragged ones) — then that later folds to its declared fill at every resolution. If the read-back fill and the staged fill ever diverged the other fields would diverge with them, so the test is self-checking on its own wiring.
Noted on the fixture's strengths — the empty order-5 target cell and the object-level _objects pin both survive unchanged.
| _backfill_leaf(store_root, decimal, window, plan, counts, store_kwargs, force=force) | ||
| seen += 1 | ||
| if held is not None and seen % HEARTBEAT_EVERY == 0: | ||
| held = heartbeat_lease(store_root, held, store_kwargs=store_kwargs) |
There was a problem hiding this comment.
🤖 from Claude (review)
The heartbeat is counted in LEAVES, so nothing bounds the wall-clock gap between beats — the lease can expire mid-pass while this run keeps writing columns. HEARTBEAT_EVERY = 64 against sweep_lease.DEFAULT_TTL_S = 900 silently assumes a leaf costs under ~14 s. Nothing enforces that, and the target this issue names does not obey it: column.write_leaf_column's own memory note measures the node-order k-way merge at ~2.0 GB / ~17.6M centroids, and this pass adds a full leaf READ (every cell array plus the ragged digests) plus write_column's ~39 object PUTs on top. One slow run of 64 leaves on S3 blows the TTL.
What follows is not a clean abort — it is the state §4.8 exists to prevent:
- beat N lands; leaves 1..64 take > 900 s;
- the intent is now claimable, and any other sweep takes it — including the operator's own retry, which
acquire_lease's refusal text explicitly invites ("claim after the heartbeat expires"); - this run keeps writing columns the whole time — it does not learn it lost the lease until leaf 128;
- beat N+1 raises
SweepRefusedError, and by then two writers have been PUTting into the same(leaf, window)prefixes.
The finally is right (release_lease refuses to delete a foreign intent, so nothing is stolen), but the window itself is the chimera column the lease is for — and it makes the normative claim this PR adds at docs/specification.md:1158, "a reader MAY assume ... it is serialized against every other sweep by the §4.8 admission lease", untrue in exactly the case that matters.
The repo already has the fix, in the only other lease holder. sweep_stages beats on the CLOCK, not on a count:
def _maybe_beat(*_args):
if time.monotonic() - last_beat[0] >= ttl_s / 3:
heartbeat_lease(store_root, lease, store_kwargs=store_kwargs)
last_beat[0] = time.monotonic()fired per dispatch node AND per tuple, so "a beat must land well inside the TTL however long a tuple (or one node's fold) runs, without one PUT per node on a large store" — which is precisely the tradeoff HEARTBEAT_EVERY's docstring is reaching for, and a time.monotonic() per leaf costs nothing. As landed, the constant's stated justification ("a store large enough to outlive the TTL is ordinary") argues for a beat, never for 64 leaves being a safe interval.
There was a problem hiding this comment.
🤖 from Claude
Folded in cc5c9c8 — the beat is now thrown by the wall clock, mirroring sweep_stages._maybe_beat:
beat_after = int((held or {}).get("ttl_s") or DEFAULT_TTL_S) / HEARTBEAT_FRACTION
last_beat = time.monotonic()
...
if held is not None and time.monotonic() - last_beat >= beat_after:
held = heartbeat_lease(store_root, held, store_kwargs=store_kwargs)
last_beat = time.monotonic()HEARTBEAT_EVERY = 64 is gone; HEARTBEAT_FRACTION = 3 is the ttl_s / 3 throttle, and its docstring now says why the interval is a clock and not a count (nothing bounds a leaf, so a count is an unenforced assumption about seconds per leaf). The TTL comes off the held lease rather than the constant, so a caller that took a non-default ttl_s beats to its own.
Test: TestBackfill.test_the_heartbeat_is_thrown_by_the_clock_not_a_leaf_count swaps column_backfill.time for a fake clock that jumps a full DEFAULT_TTL_S per read and runs a two-leaf pass — 2 beats now, 0 under the count-based form.
docs/specification.md §4.6 corrected in the same commit: the bullet now says the lease is heartbeated on the wall clock and that §4.8's expiry rule is the residual — a holder that stalls past its ttl_s without beating is claimable like any other, so what a reader MAY assume is the lease's guarantee, not a stronger one.
| the cost of one leaf-reading pass, paid once. | ||
|
|
||
| It is a **sweep family** (``columns``, registered in :mod:`zagg.sweep`), not | ||
| a mode of its own: the family registry is the already-Lambda-wired |
There was a problem hiding this comment.
🤖 from Claude (review)
"the already-Lambda-wired mode: "sweep" transport ... with no handler change" is not true as landed: the handler cannot select a family, so columns is unreachable worker-side. deployment/aws/lambda_handler.py's sweep arm is, in full:
summary = run_sweep(event["store_path"], leaves, store_kwargs=store_kwargs)grep -n 'families\|partition' deployment/aws/lambda_handler.py returns exactly two hits and neither is a read: line 638 (prose in a docstring) and line 1191 ("families": summary["families"], in the response). The event carries no families key the handler honours, so run_sweep falls to DEFAULT_FAMILIES — and this family is deliberately not in DEFAULT_FAMILIES. There is no payload that makes a Lambda worker run the backfill. It needs the one thing this paragraph promises it does not: a handler change.
The next clause has the same problem from the other end. "partitioning (issue #377) ... come for free" — the handler never reads event["partition"] either, and runner._build_sweep_event fires the 2^n partitions as CONCURRENT InvocationType="Event" invokes. Against a store-granular lease that is not free, it is fatal: all but one invoke would take SweepRefusedError straight out of acquire_lease. (The in-process form is fine — sweep_partition.sweep_partitions runs them sequentially.)
What is true is the local claim, and it is worth keeping: --partitions works from the CLI, and _normalize_leaves / select_partition / discovery are genuinely inherited. Scope the sentence to that and say plainly that fleet execution of this family still needs a handler seam. The claim is repeated verbatim in the PR body, CHANGELOG.md, tests/test_column_backfill.py:10-11 and docs/pyramid_upgrade.md:75-78, so all five sites move together.
There was a problem hiding this comment.
🤖 from Claude
Folded in a5ad84d (code/test/CHANGELOG) plus the PR body. You are right; lambda_handler.py is #519 territory and stays untouched, so the fold was to make every statement of the claim true.
The accurate statement, now in all five sites (column_backfill.py module docstring, tests/test_column_backfill.py module docstring, CHANGELOG.md, docs/pyramid_upgrade.md step 2 — that one in 0b0f545, see the sibling thread — and the PR body):
- the family is a registry entry, so it needs no NEW transport and no new mode: the work-set normalization,
--partitionsand discovery are inherited whole; - supported entry points today are in-process only — the CLI (
--families columns) andrun_sweep(..., families=["columns"]); - fleet execution is not one of them, and the module docstring now spells out exactly why: the handler's
mode: "sweep"arm isrun_sweep(event["store_path"], leaves, store_kwargs=...), reads neitherfamiliesnorpartitionoff the event, and so always falls toDEFAULT_FAMILIES, which this family is deliberately not in. Forwarding those two keys is the handler's change, stage-worker Lambda transport: run the /2 staged dense sweep fleet-side (open question (b) of the staged-sweep landing) #519's territory, deliberately not in this PR; - the partition half is stated too:
--partitionsis real in-process (sweep_partitionsruns them sequentially), while the runner fires fleet partitions as concurrentEventinvokes, which one store-granular lease admits exactly one of.
PR-body question 3 is rewritten to ask the decision outright: should the one-call forwarding land here (crossing into #519's file) or on #519? — with the concurrent-Event / store-granular-lease tension named as a design question that comes with it either way.
| ``` | ||
|
|
||
| `columns` is a registered sweep family, so this rides the already-wired | ||
| `mode: "sweep"` transport: `--partitions 2^n` bounds peak memory (or fans the |
There was a problem hiding this comment.
🤖 from Claude (review)
Both halves of this sentence are wrong for an operator, and the second one hides a real difference from --stages.
-
"or fans the work out worker-side" — it does not.
deployment/aws/lambda_handler.py'smode: "sweep"arm callsrun_sweep(event["store_path"], leaves, store_kwargs=store_kwargs)and reads nofamilieskey from the event, so a fleet invoke can only ever runDEFAULT_FAMILIES— which this page itself correctly says three lines down excludescolumns. An operator with a store too large for one machine will read this line, go looking for the fan-out, and find that no invoke payload reaches this family. -
"exactly as it does for the other families" — not exactly, and the difference matters here. The other four families take no lease; this one takes the store-granular §4.8 lease inside
backfill_columns. Undersweep_partitionsthat is onerun_sweepper partition, so the lease is acquired and released once per partition and the store is unleased in every gap — where a foreign sweep can interleave with a half-finished backfill. Compare the CLI help for the sibling flag:--stages"Composes with --partitions (swept under one lease)."
On the CA ATL03 target (2,721 o9 shards) --partitions 64 therefore opens 63 unleased windows in the middle of the one pass this PR's own §4.6 amendment calls the sanctioned second writer — the same page whose Preconditions section leans on the lease.
There was a problem hiding this comment.
🤖 from Claude
Folded in 0b0f545 — step 2 of the runbook is rewritten into three paragraphs, and both halves of the sentence you flagged are gone.
-
The fan-out claim is replaced by a paragraph saying plainly that this runs in-process, on the machine you type it on: the two entry points are the CLI and
run_sweep(..., families=["columns"]), the handler's sweep branch forwards nofamilies/partitionfrom the event so a worker falls to the default family set (which excludes this one), adding that forwarding is stage-worker Lambda transport: run the /2 staged dense sweep fleet-side (open question (b) of the staged-sweep landing) #519's change, and until it lands a store too large for one machine has no fan-out for this step. -
The
--partitionsparity claim is replaced by a paragraph that names the lease churn rather than recommending the flag unqualified: unlike--stages, which sweeps every partition under one lease,--partitionsgives this family onerun_sweepper partition, so the lease is acquired and released per partition —--partitions 64leaves 63 unleased windows in the middle of the pass §4.6 sanctions as the second writer. The recommendation is explicit: harmless on a genuinely quiesced store (nothing else is admitted anyway), but if you cannot guarantee quiescence, run the backfill unpartitioned so the whole pass sits under one lease.
The short-form runbook in the PR body carries the same caveat in its comment line.
| child_order=int(manifest["cell_order"]), | ||
| ) | ||
| block = build_pyramid_block(config, shard_order, default_chunk) | ||
| if (overviews is not None or chunk_order is not None) and "overviews" not in block: |
There was a problem hiding this comment.
🤖 from Claude (review)
This guard is unreachable on the overviews= arm — and that is the arm that can write a manifest the store contradicts: overviews= installs a /2 block on a RASTER store. The comment says the raise catches "a reader: raster config, which build_pyramid_block exempts from the flip by ruling". But the raster exemption lives only inside the DEFAULT-FLIP condition:
and (config.data_source or {}).get("reader") != "raster"retrofit_declaration's overviews= arm never goes through the flip — it writes knob["overviews"] into the config, so build_pyramid_block takes the explicit branch (if knob.get("overviews") is not None:), which has no reader check at all, and always returns a block carrying overviews. Verified against 8689a56c:
cfg = replace(cfg, data_source={**cfg.data_source, "reader": "raster"})
c2, chunk, via = retrofit_declaration(cfg, overviews=[5], chunk_order=None,
parent_order=4, child_order=6)
blk = build_pyramid_block(c2, 4, chunk)
# declared_via: overviews=[5]
# spec: zagg-pyramid/2 has overviews: True
# guard fires? FalseSo python -m zagg.sweep <raster root> --declare-pyramid raster.yaml --overviews 5 writes zagg-pyramid/2 onto a store spec §4.6 places entirely out of scope — "Raster hive stores are column-less by construction: nothing in this section applies to them (issue #399 owns their overview regime)" — after which manifest_column_plan accepts that manifest and the backfill starts opening raster leaves looking for {cell_order}/{field} slabs. Refuse reader: raster by name in retrofit_declaration beside the other two silent-no-op refusals, or move the exemption into build_pyramid_block's explicit-overviews branch so both arms inherit it.
Second, smaller, on the same raise: it IS reachable on the chunk_order= arm for a non-raster config, and then the message is simply wrong. build_pyramid_block gates the flip on the CONFIG's output.grid.child_order, while retrofit_declaration validated against the MANIFEST's cell_order; orders are outside the semantic core (semantics.py: "Excluded as packaging: all orders"), so _semantic_guard cannot catch a config whose grid disagrees with the store. chunk_order=5 against config child_order: 5 / manifest cell_order: 6 lands here and is told it is a raster config. Nothing in TestRetrofitDeclaration reaches this raise on either arm.
There was a problem hiding this comment.
🤖 from Claude
Folded in af89609. Both halves.
Raster. Refused by name in retrofit_declaration, up front, on both arms rather than only overviews= — the chunk_order= arm reached the same conclusion by a worse route (a /1 fallback caught downstream by a guard whose message blamed the config), so one refusal at the lever is the honest place:
if (config.data_source or {}).get("reader") == "raster":
raise ValueError(
"declare_pyramid(overviews=/chunk_order=) refuses a `reader: raster` config: "
"raster hive stores are column-less by construction and spec §4.6 does not apply "
"to them (issue #399 owns their overview regime). The #384 default flip already "
"exempts them, but `overviews=` bypasses the flip entirely — it would install a "
"zagg-pyramid/2 block declaring leaf columns nothing writes, which the column "
"backfill would then chase into raster leaves. Nothing was written"
)The remaining raise. sweep_overview's post-build guard keeps its belt-and-braces role but now describes what actually reaches it — the chunk_order= arm on a non-raster config — and names the real cause: the lever was validated against the manifest's cell_order while the #384 flip gates on the config's own output.grid.child_order, and orders are packaging, outside the semantic core, so _semantic_guard cannot catch a config whose grid disagrees with the store. The message quotes both numbers and points at overviews=.
Two tests in TestRetrofitDeclaration, one per arm of your finding:
test_a_raster_config_is_refused_by_name_on_both_levers—overviews=5andchunk_order=5on areader: rasterconfig, bothValueError, manifest unchanged byte for byte;test_chunk_order_refuses_a_config_grid_the_store_contradicts— configchild_order: 5against manifestcell_order: 6,chunk_order=5, matchingdoes not describe this store, manifest unchanged.
| store_kwargs=store_kwargs, | ||
| ) | ||
| counts["written"] += 1 | ||
| except Exception as e: |
There was a problem hiding this comment.
🤖 from Claude (review)
A store-wide fault is laundered into N per-leaf failed, and nothing downstream stops the operator: the CLI exits 0 and the runbook's next step publishes the short ladder. This blanket catch is the right posture for "leaf 4,112's digest is corrupt". It is the wrong one for the failure this pass will actually meet on the CA ATL03 target — a rotated/expired credential, a 403 on the column prefix, a region-wide 503 — where every remaining leaf raises the same exception and the pass returns {"written": 0, "failed": 2721} as an ordinary summary.
Nothing after it is a gate:
run_sweeprecords the counts and returns normally;zagg.sweep.mainprints the JSON andreturn 0— no exit code carriesfailed;docs/pyramid_upgrade.mdstep 2 → step 3 sends the operator straight on to--stages, and only says the summary "countswritten/current/empty/failed" (line 85) — never that a non-zerofailedmust stop them;- the staged sweep then records each gap as
source_childrenunder-coverage inside artifacts nobody is reading, and publishes a ladder folded from whichever columns happened to land.
That posture is defensible for the other four families because their artifacts are regenerable D9 caches whose absence is the normal state (run_sweep's own docstring says so). A column backfill is an upgrade of a published store, and its partial result is the one input the /2 staged sweep silently under-covers on.
Either fix closes it: (a) backfill_columns raises when failed > 0 and written == 0 — a store-wide fault is not N leaf faults, and the distinction is cheap to make here; and/or (b) the runbook gates step 3 in the page itself ("failed must be 0 before step 3; the backfill is idempotent, re-run it"). Right now neither the code nor the docs says it.
There was a problem hiding this comment.
🤖 from Claude
Folded in 275d00d — both arms you offered, since they close different halves.
(a) The abort. _backfill_leaf now returns the failure text, and backfill_columns raises after the lease is released when the pass wrote nothing, skipped nothing as current, and failed at least one leaf:
if counts["failed"]:
if not counts["written"] and not counts["current"]:
raise RuntimeError(
f"column backfill on {store_root} wrote nothing: all {counts['failed']} "
f"leaf attempts failed, the last with {last_error}. A whole-store fault "
f"(expired credentials, a denied column prefix, an outage) is not N leaf "
f"faults — ... The pass is idempotent: fix the fault and re-run it"
)
logger.error(...)One deviation from your wording, deliberate: the condition is written == 0 **and** current == 0, not written == 0 alone. A re-run of an already-upgraded store with one corrupt leaf is written: 0, current: N-1, failed: 1 — a leaf fault, and raising there would be a false alarm. Every store-wide shape still lands in the raise, because _column_state swallows its read error and returns "not current", so a blanket 403/503 leaves current at 0 too. Raised outside the try so the lease is already released: a loud return, not a torn pass. Since run_sweep does not catch family exceptions, the CLI surfaces it and exits non-zero.
(b) The runbook is loud. Step 2 gains a bolded failed must be 0 before step 3`` paragraph (what a partial result means, that the staged sweep would fold a ladder over whatever landed and record the gaps only as source_children under-coverage, and that the repair is re-running the idempotent pass), and step 3 opens with only run it once step 2 reported `failed: 0` and why it cannot tell a missing column from an undeclared one. The `backfill_columns` docstring states the same split.
Tests: test_a_store_wide_fault_raises_instead_of_counting_every_leaf_failed (every write_column denied → RuntimeError, last error named, lease released) and test_a_single_failed_leaf_is_counted_loudly_and_still_returns (written: 1, failed: 1, logger.error fired).
Left standing by choice: the CLI still returns 0 on a partial failure. Making zagg.sweep.main exit non-zero on any family's failed is a shared-CLI behaviour change well past this PR's scope; the runbook gate plus the ERROR log is what covers it here. Happy to file it if you want the exit code too.
| from zagg.hive import read_manifest | ||
|
|
||
| return backfill_columns( | ||
| str(root), read_manifest(str(root)), {d: {None} for d in SHARDS}, **kwargs |
There was a problem hiding this comment.
🤖 from Claude (review)
The windowed (leaf, window) arm has no test at all — and the backfill is the ONE column writer that recovers time_range from a stamp instead of computing it. Every entry point in this file is unwindowed: this helper hard-codes {d: {None} for d in SHARDS}, _column_path / _verdict / _leaf_stamp / _column_state all default window=None, _write_run_record writes "window": [None] * n, and TestUpgradeEndToEnd never builds a windowed store. So nothing exercises the one genuinely new line in _backfill_leaf:
write_column(..., window=window,
time_range=leaf_stamp.get("time_range"),
granule_count=int(leaf_stamp.get("granule_count") or 0), ...)At build time hive.process_and_write_hive hands write_leaf_column the run's own computed time_range; here it is read back out of the leaf's D15 stamp, where stamp_commit stored it as [str(t) for t in time_range]. "Those two are the same value" is exactly the claim a characterization test exists to pin, and the blast radius is normative: spec §4.6 requires a windowed column's stamp to be morton-hive/2 carrying window plus the observed time_range, and stamp_commit fails CLOSED around it (time_range without window raises; a reversed pair raises). A backfill that dropped, reordered or mistyped it fails the leaf silently, into failed, via the blanket except Exception above.
Untested with it: column_name(window) vs the SCHEDULE_NONE_TOKEN in the zagg_column.window attr, the shard_leaf_path(window=) legs in _leaf_stamp and _column_state, and the driver's sorted(..., key=lambda w: (w is not None, w or "")) window ordering — the only place a leaf with several windows is handled.
The harness exists next door: tests/test_column.py's builder already takes window=/windowing= and test_windowed_unit_gets_a_window_named_column drives process_and_write_hive through it. One windowed twin through TestBackfill._upgraded (byte-compare plus stamp["window"]/stamp["time_range"] on the backfilled column) covers all of it.
There was a problem hiding this comment.
🤖 from Claude
Folded in 3383765 — a new TestWindowedBackfill class, and the harness grew the test_column.py shape you pointed at.
_build_store now takes window= / windowing= / time_range= (the _run_unit arguments), and _backfill takes windows= so a work set can carry real labels instead of {None}. Three tests:
test_a_windowed_column_is_backfilled_byte_for_byte— a windowed pyramid-OFF store and its pyramid-ON twin, two leaves; every object under2019.pyramid.zarrcompared, rootzarr.jsonon the two-timestamp terms. That coverscolumn_name(window)and bothshard_leaf_path(window=)legs by construction.test_the_window_and_the_recovered_time_range_reach_the_stamp— the characterization you asked for, pinned four ways:stamp["spec"] == HIVE_SPEC_V2 == "morton-hive/2",stamp["window"] == "2019",stamp["time_range"] == ["2019-01-01T00:00:00+00:00", "2019-01-01T00:01:00+00:00"], and then the two equalities that make it a recovery rather than a coincidence — equal to the LEAF's own stamped pair, and equal to the pyramid-ON twin worker's(window, time_range). Pluszagg_column.windowoff the root attrs.test_a_leaf_with_several_windows_gets_one_column_each— two windows built per leaf, backfilled in one pass with the work set spelled out of order (("2020", "2019")),written == 4, then a second pass returningcurrent == 4(idempotent per window, not just per leaf). That is the driver'ssorted(..., key=lambda w: (w is not None, w or ""))exercised.
| open_store(str(path), read_only=True), path="", mode="r", zarr_format=3 | ||
| ) | ||
| attrs = dict(group.attrs) | ||
| if attrs.get("role") != "overview": |
There was a problem hiding this comment.
🤖 from Claude (review)
_ladder (and _overview_attrs) filter to role == "overview", so seven of the artifacts this recipe produces are compared by NOTHING in the acceptance test. Running TestUpgradeEndToEnd's exact recipe on 8689a56c and listing every *.zarr in both stores:
1/1/2/1/3/all.pyramid.zarr role=column groups=['4','5'] <- backfilled LEAF column
1/1/2/1/4/all.pyramid.zarr role=column groups=['4','5']
1/1/2/2/3/all.pyramid.zarr role=column groups=['4','5']
2/1/2/1/3/all.pyramid.zarr role=column groups=['4','5']
1/1/2/1/all.pyramid.zarr role=column groups=['4'] <- STAGE column (§4.6)
1/1/2/2/all.pyramid.zarr role=column groups=['4']
2/1/2/1/all.pyramid.zarr role=column groups=['4']
...9x role=overview <- the only thing compared
The three stage columns are a first-class §4.6 artifact with their own regime: "stage-gather", generation, source_children and run_id; the four leaf columns are this PR's entire output. The class docstring says "pyramid-OFF -> declare -> backfill -> CLI staged sweep == pyramid-ON twin", and the PR body says the whole upgrade is pinned — as written it pins nine of sixteen artifacts.
It is free to close, which is the strongest argument for doing it. I ran the comparison on this head, on the same terms _overview_attrs already uses (arrays byte-compared per group; zagg_column minus RUN_LOCAL and, inside generation, RUN_LOCAL_GENERATION):
same set: True
1/1/2/1/3/all.pyramid.zarr attrs EQ arrays EQ
1/1/2/1/4/all.pyramid.zarr attrs EQ arrays EQ
1/1/2/1/all.pyramid.zarr attrs EQ arrays EQ
1/1/2/2/3/all.pyramid.zarr attrs EQ arrays EQ
1/1/2/2/all.pyramid.zarr attrs EQ arrays EQ
2/1/2/1/3/all.pyramid.zarr attrs EQ arrays EQ
2/1/2/1/all.pyramid.zarr attrs EQ arrays EQ
Widening both collectors to role in ("overview", "column") (keying the group selection off the role, since a column carries several groups and an overview exactly one) turns the acceptance into what the PR body claims for it.
There was a problem hiding this comment.
🤖 from Claude
Folded in 27de402 — widened, and it passes on the same terms, exactly as you measured.
Both collectors now walk one shared generator, _compared(root), keyed off a COMPARED_ROLES = {"overview": "zagg_overview", "column": "zagg_column"} map, and group selection is keyed off the role as you suggested — [block["cell_order"]] for an overview, sorted(dict(group.groups())) for a column:
_ladderkeys arrays by(res, name)so a multi-group column fits the same dict shape;_overview_attrscompares each artifact's provenance block minusRUN_LOCAL, and insidegenerationminusRUN_LOCAL_GENERATION— unchanged terms, now applied tozagg_columntoo, so the stage columns'regime,generation,source_childrenandrun_idare in the compare.
The count assertion moved with it:
assert len(upgraded) == 16, sorted(upgraded)
roles = [role for _p, role, _b, _g in _compared(off)]
assert (roles.count("overview"), roles.count("column")) == (9, 7)9 overviews + 4 backfilled leaf columns + 3 stage columns, and the (9, 7) split is pinned so a future change that quietly stops collecting one role fails rather than shrinking the compare. Green on both kitchen_sink parametrizations, no RUN_LOCAL terms added. The class docstring and the PR body's phase-4 bullet now say 16 artifacts rather than 9.
| if leaf_stamp is None: | ||
| counts["empty"] += 1 | ||
| return | ||
| stamp, attrs, structure = _column_state(store_root, shard_key, window, store_kwargs) |
There was a problem hiding this comment.
🤖 from Claude (review)
The skip gate does loop-invariant work on every leaf, and pays the whole read even when force=True throws the verdict away. Two separate wastes on these two lines, both on the pass's hot path (2,721 leaves on the CA o9 target this issue names):
-
column_is_currentcallscolumn_structure(fields, node_order=node_order, resolutions=resolutions)per leaf — and all three arguments areplan.fields/plan.node_order/plan.resolutions, constant for the entire pass. Each call constructslen(resolutions)HealpixGrids, callsshard_spec()on each, thenmodel_dump()+json.dumps+json.loadsper member. At the reference geometry (groups {13,12,11,10,9}, ~7 members apiece) that is ~35 pydantic dumps and ~35 JSON round-trips per leaf to rebuild a value that cannot change. Computing it once inbackfill_columnsand threading it (or memoizing on the plan) is a one-liner and keepscolumn_structure's "it IS the writer's template" property intact. -
_column_stateis called unconditionally, butstamp/attrs/structureare consumed only insideif not force:.stored_column_structurewalksgroup.groups()thensub.arrays(), which on S3 with no consolidated metadata is a LIST plus onezarr.jsonGET per member — the ~35-object read its own docstring calls "the whole cost the term adds to a skip". Underforce=Truethat buys nothing: the next statement rewrites the prefix wholesale. Moving the call inside theif not force:block is free and makesforcethe cheap path it reads as.
There was a problem hiding this comment.
🤖 from Claude
Folded in ccbfc87. Both wastes.
-
column_structure(plan.fields, node_order=plan.node_order, resolutions=plan.resolutions)is derived once inbackfill_columns, right aftermanifest_column_plan, and threaded to_backfill_leafand on intocolumn_is_currentas a newtemplate=keyword.template=Nonerecomputes, which keeps the direct-call form the gate tests use unchanged, andcolumn_structurestays the writer's own projection (nothing about how it is built moved). The docstring says why it is safe: it is a function of the declaration alone, so it cannot move inside a pass. -
_column_statemoved insideif not force:, with a comment naming the cost it skips (a LIST plus onezarr.jsonGET per member, ~35 objects, all of it consumed only by the gateforcediscards). Nothing outside that branch referencedstamp/attrs/structure.
Two tests in TestBackfill, both counting calls through monkeypatch:
test_the_gate_template_is_derived_once_per_pass— a 4-leaf pass callscolumn_structureexactly once;test_force_skips_the_stored_column_read— aforce=Truepass over an already-columned store calls_column_statezero times and still writes all 4.
| `/2` declaration (§4.5) carries leaf-node levels **and** at least one | ||
| composable field; a store still declaring `/1`, declared off, or `class: | ||
| "none"` on every field is refused and MUST be re-declared first, never | ||
| guessed at. |
There was a problem hiding this comment.
🤖 from Claude (review)
The amendment sanctions a second writer but leaves §4.6's "Failure identity" paragraph enumerating only two readings of a missing column — and this pass adds a third whose repair is the opposite of the one that paragraph prescribes. Further down this same section, unchanged:
A column-write failure fails the worker unit; the retry rewrites leaf and column wholesale. A committed leaf whose column is absent or unstamped therefore reads as either a torn worker or a leaf whose writing declaration carried no column ... where the writing declaration is known to carry leaf-node levels, absence is the torn-worker signature and the repair is re-invoking the idempotent leaf.
None of that survives the backfill. column_backfill._backfill_leaf catches every exception, counts the leaf failed, and the pass returns normally — so a leaf whose backfill failed is absent-with-a-/2-declaration, which is precisely the state this paragraph tells a reader to diagnose as a torn worker. The prescribed repair (re-invoke the idempotent leaf) would re-aggregate from source granules, the one thing the upgrade exists to avoid; the correct repair is re-running the idempotent backfill.
§4.6 is normative and external readers decode from this page alone (CLAUDE.md §4), so the "therefore ... either ... or" needs the third case, or the amendment needs a sentence saying what a partially-completed backfill leaves behind and how it is told apart. As it stands the page now sanctions a writer whose failure mode it does not describe.
There was a problem hiding this comment.
🤖 from Claude
Folded in a0c6305 — the "Failure identity" bullet now enumerates three readings and, more to your point, separates them by their repair:
- torn worker, where the writing declaration is known to carry leaf-node levels → re-invoke the idempotent leaf (unchanged);
- leaf-node levels that arrived by RE-DECLARATION — the
/1 -> /2upgrade, whose whole point is that the leaf is not rewritten → absence is the backfill's signature, and the repair is re-running the idempotent backfill. The page now says outright that re-invoking the leaf there would re-aggregate from source granules, "the one cost the upgrade exists to avoid"; - a leaf predating any column declaration → no repair.
It also says what a partially-completed pass leaves behind and how it is told apart: the backfill counts a leaf it cannot read or fold and carries on, so the store carries exactly this state and what distinguishes it is the pass's own summary (failed), not the artifacts. The "manifest MAY lag" clause is kept and extended — after a re-declaration the manifest necessarily leads the leaves, which is the whole window this case lives in.
No fixture work needed, and I checked rather than assumed: this is prose about diagnosis and repair only — no wire format, no attrs grammar, no versioned spec marker moves — so tools/generate_spec_fixtures.py and tests/data/spec/ are untouched.
…kfill # Conflicts: # CHANGELOG.md # src/zagg/sweep_overview.py
1 participant
Closes #520.
The
/1 → /2upgrade bridge: an existing published store gets leaf columnswithout re-aggregation, then takes the
/2staged sweep. Implements theissue's plan sketch and the implementation plan
comment.
Phases
computes a leaf's column from its stored bytes, pinned byte-identical to
the build-time column, per leaf, on a local pyramid-ON store.
columnsfamily in the existing sweepfamily registry (a registry entry — no new transport, no new mode; the
in-process CLI/API arms only, see the note under phase 2),
declaration-driven, skip-if-current.
/2retrofit declaration.declare_pyramid(overviews=…/chunk_order=…)for the grid-less retrofit path, validated against the manifest's own orders.
backfill → the existing CLI staged sweep → ladder byte-equal to a twin built
pyramid-ON from identical inputs.
/1 → /2runbook, spec §4.6, CHANGELOG.What landed, by phase
37c3e16zagg/column.py,tests/test_column_backfill.py90c984bzagg/column_backfill.py(new),zagg/sweep.py(registry)4b8e81ezagg/pyramid.py,zagg/sweep_overview.py,zagg/sweep.py(CLI)6eb01e6ruff formatfix a bulk format run swept in09f0823tests/test_column_backfill.py6f53530docs/pyramid_upgrade.md(new),docs/specification.md§4.6,mkdocs.yml,CHANGELOG.md88cb239..d24ce3d8689a56column_backfill.pycc5c9c8..a0c630579e0927Phase 1 — the recipe read off stored bytes
stored_leaf_slabs(leaf_path, fields, *, cell_order, n_cells, …)— theread-back twin of
leaf_slabs: samecomposable_fieldsfilter, sameabsent-field-is-fill rule, same
(n_cells,)extent refusal, same companionpickup, but values come from the leaf's stored arrays instead of the Freeze the O11 content-hash serialization: writer must match the moczarr reader's pinned recipe (keys, combined, vlen) #342
staged sink. Two guards a read-back needs and the sink cannot, both borrowed
from
sweep_overview._fold_nodeso the two read paths refuse the samestores: the leaf's
mortonextent pins the geometry (Overview sweep: fold mixed-order source leaves (D24) — replace the skip fence #347), and every digestfield's stored §2.0
weights/ §8.4 companion declaration is checked againstthe manifest's.
column_from_leaf(...)—stored_leaf_slabs→fold_column, the same twocalls
write_leaf_columnmakes against the resident sink. Pure compute.manifest_column_plan(manifest) -> ColumnPlan— the declaration gate read offa store rather than a config.
leaf_column_planis the worker's gate(workers never open the manifest, §4.6); a backfill has no config, so its gate
is the manifest's own
/2declaration. Every refusal names itself.column_is_current(...) -> (verdict, reason)— the skip gate.Phase 2 — the
columnssweep familyzagg/column_backfill.pyholds the whole implementation;sweep.pygains onlythe ~30-line
ColumnFamilyand its registry entry. Being a registry entry itneeds no new transport and no new mode — the work-set normalization,
--partitions(#377) and the run-record discovery are inherited whole, andneither
lambda_handler.pynor the runner dispatch seam is touched.Supported entry points today are in-process only: this CLI and
run_sweep(root, leaves, families=["columns"]). Fleet execution is notwired, and the earlier claim that it needed no handler change was wrong
(corrected in
a5ad84d):lambda_handler.py'smode: "sweep"arm isrun_sweep(event["store_path"], leaves, store_kwargs=...)and reads neitherfamiliesnorpartitionoff the event, so a worker always falls toDEFAULT_FAMILIES— which this family is deliberately not in. Forwarding thosetwo keys is a
lambda_handler.pychange, #519's territory, deliberately not inthis PR (question 3 below). Partitioning inherits the same seam:
--partitionsis real in-process (
sweep_partitionsruns them sequentially, one leaseacquire/release per partition), while the runner fires fleet partitions as
concurrent
Eventinvokes, which one store-granular lease admits exactly oneof.
It is deliberately out of
DEFAULT_FAMILIES; spell it:Per
(leaf, window): leaf commit stamp → skip-if-current → recompute fromstored bytes → wholesale write. Counts
written/current/empty/failed;a leaf that cannot be read fails only itself — but a pass that writes
nothing while every leaf fails raises instead of returning that as a
summary (
275d00d: a store-wide fault is not N leaf faults), and the runbooknow gates step 3 on
failed == 0. The pass takes thesweep_leasefor itsduration, beating on the wall clock (
ttl_s / 3, thesweep_stagesdiscipline —
cc5c9c8replaced a per-64-leaves count that could not bound thebeat interval), releasing in a
finally.Declaration gate, all refusing by name before the lease is taken:
zagg-pyramid/1(orders/spacing)orders: [](declared off)/2, every fieldclass: "none"(the CA ATL03 0.48 case)/2with composable fieldsPhase 3 —
declare_pyramid's/2retrofit leversSignature as landed:
plus the CLI
python -m zagg.sweep <root> --declare-pyramid <cfg> --overviews 13.The grammar work is
zagg.pyramid.retrofit_declaration(pyramid.py owns the/2grammar and has room;sweep_overview.pygrows by ~15 lines only). Bothlevers are validated against the manifest's
shard_order/cell_order, andthe two silent no-ops refuse rather than falling back to
/1:chunk_order=is inert once a schedule is spelled);chunk_order=againstoutput.pyramid: false→ refused (nothing to default);chunk_order=against a config that spellsoverviews/orders/spacing→ refused;reader: rasterconfig on either lever → refused by name (af89609): raster hive stores are column-less by construction (§4.6 / raster hive under a /2 pyramid declaration: leaf columns are never written (rule PR #391 options (a)/(b)) #399), and theoverviews=arm bypasses the default flip's exemption entirely;chunk_order=lever that still fails to produce a/2block → refused, naming the real cause (the config's ownoutput.grid.child_orderdisagreeing with the store'scell_order);overviews=does override apyramid: falseconfig — logged loudly; that is the point of the lever.The summary gains
declared_via("config"/"overviews=[13]"/"chunk_order=13").Byte-parity results
Phase 1 — recomputed column == build-time column, per leaf.
tests/test_column_backfill.py::TestStoredLeafParitybuilds a real pyramid-ONstore — 4 leaves through the production
process_and_write_hive, two basecells, order 4/6 with
chunk_inner=5so the #384 default flip declaresoverviews: [5]— recomputes each column from the stored leaf, writes it to ascratch store, and compares every object under the prefix:
zarr.json;zarr.jsonequal after dropping the two keys a rewrite always moves(
zagg_column.generated_at,morton_hive_commit.written_at) — socells_with_dataandgranule_countare pinned too (the latter recoveredfrom the leaf's own stamp, which is where the build-time value came from).
Run over both declarations the fixture generator carries: plain (exact + one
digest) and kitchen sink (two located digests with their
_locationscompanions, plus a
packedcomposition word over itsofdigest) — the exact,approximate-with-companions and packed fold arms all covered.
Phase 2 — backfilled column == the pyramid-ON twin's, per leaf. Same
object-level comparison, but between a pyramid-OFF store re-declared and
backfilled and a twin built pyramid-ON from identical inputs. Both declarations.
Phase 4 — the whole upgrade == the pyramid-ON twin, end to end, offline.
TestUpgradeEndToEndruns the full recipe on local-backend stores:pyramid-OFF build →
declare_pyramid(overviews=5)→run_sweep(families=["columns"])→ the existing CLI staged sweep (
zagg.sweep.main([root, "--stages"]),unmodified) — and against a pyramid-ON twin given the same staged sweep:
backfilled leaf columns and the 3 §4.6 stage columns (
27de402widened thecollectors, which previously compared the 9 overviews and nothing else);
tobytes(), ragged per-cell payloadbytes);
zagg_overview/zagg_columnprovenance block equal — includingsource,generation.n_leaves,source_children,regimeand the contenthashes — after dropping only
generated_atand the sweep's ownrun_id;overviewsmaterialization inventory equal on the same terms.Also parameterized over the kitchen sink. No fleet, no AWS, no #519: the
staged-sweep transport is the only thing #519 changes.
Notes
column.py804 → 804 — byte-identical tomain. Phase 1 originallygrew it to 1,067 and the review folds pushed it to 1,233, over the trigger;
8689a56moved the whole/1 → /2read-back path (stored_leaf_slabs,column_from_leaf,column_structure,column_is_current,manifest_column_plan) intocolumn_backfill.py, which is its onlyconsumer.
column.pystays the build-time writer;column_backfill.pyowns the read-back path and the pass.
column_backfill.py773 (new).sweep.py1,397 → 1,447 (+50, already over the trigger before this PR —only the family class and the CLI flag).
sweep_overview.py2,426 → 2,474 (+48, already over — only thedeclare_pyramidsignature, docstring and wiring; the grammar went topyramid.py).pyramid.py349 → 459.backfilled column is byte-identical to a build-time one, and
zagg-column/1is untouched.
docs/specification.md§4.6 changes only in what it sanctions(see below).
store.py(published-bucket store handle 403s on LIST: the #495 canned-ACL default header is sent unsigned on non-PUT requests (blocks all source.coop fleet builds/sweeps) #522),lambda_handler.py, the runnerdispatch seam and
sweep_stages.py(stage-worker Lambda transport: run the /2 staged dense sweep fleet-side (open question (b) of the staged-sweep landing) #519) are untouched. The phase 4 testcalls the CLI staged sweep; it does not modify it.
failures also present on
origin/main:test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds(needs network to resolve
zarr>=3.1.5) and a flakytest_client_transport.py::TestStatusPoller::test_invoke_fault_burns_an_attempt_and_retries(passes in isolation).
The
/1 → /2upgrade runbookFull version with the API forms and the failure table:
docs/pyramid_upgrade.md.The short form, on a quiesced store (see below), with the original build
config:
/1overview zarrs the old declaration left are now declared-offregenerable debris (D24 option A).
declare_pyramidpreserves thematerializedactuals across the re-declaration, so the manifest stillinventories what is on disk; deleting them is optional, reversible and has
no automated arm.
Deferred: the live CA ATL03 run. The issue's first target is the CA ATL03
store on source.coop. Nothing in this PR was run against it, or against any AWS
resource — acceptance is fully offline on local-backend stores, per the standing
egress and no-AWS-mutation rules. The live upgrade is an operator run
(the operator-runs-validation convention), and it wants the #519 fleet transport
for the backfill leg at CA scale; the CLI arm here is what makes it runnable and
pre-validated.
Questions for review
espg ruled on all four in-session on 2026-08-25; recorded here with what
changed. Nothing on this PR is waiting on an answer.
RESOLVED — the §4.6 amendment shape is accepted as written. The backfill
stands as the one sanctioned second writer of a column, with §4.8 lease
serialization plus the unenforceable "no aggregation run in flight" operator
precondition. Rationale of record: quiescence cannot be proven without a
global run registry zagg deliberately lacks (D8 — the dispatcher is not a
control plane), and the failure mode is bounded and self-healing (a stale
column is regenerable cache, D9; the skip gate usually catches it, and the
next backfill + sweep heals it).
Ruled addition, landed in
79e0927: a best-effort in-flight WARNING,never a refusal. Before the first write — and before the lease, which
serializes sweeps and cannot see a fleet —
warn_if_runs_in_flightlists theissue zagg.client v2 transport: Event invoke + status-object future resolver #327 status channel (
{store}.status/run-*/, a store sibling) and logsa loud WARNING naming every run id with an object newer than
IN_FLIGHT_WINDOW_S = 900s. The threshold is one Lambda worker wall: aunit is killed at 900 s, so an older status object cannot belong to a unit
still executing. It never refuses, never fails the pass, and is fail-open on
the listing itself (absent, unreadable, or credential-scoped-away channels
are silent) — and its silence proves nothing, which the docstring, the
module header and the runbook all say: a pre-zagg.client v2 transport: Event invoke + status-object future resolver #327 dispatcher or a lifecycled
status prefix looks exactly like a quiesced store. The flagged ids ride the
summary as
runs_in_flight. Four tests cover the ruled cases (recent object→ warning + pass completes; aged-out object → silent; no channel → silent;
listing raises → silent + pass completes), plus a nonsense-root smoke test.
RESOLVED by split — filed as #532, not implemented here. The staged sweep skip gate: same-second rewrite blind spot — close it with the run_id already in the stamps #417 skip-key residual
(a column records no
run_idfor its source leaf, so a same-second leafrewrite at an unchanged
granule_countreads as current) is split out ratherthan fixed here; closing it properly is a §4.6 attrs-grammar change (spec +
conformance fixtures) whose build-time half is
hive.py/published-bucket store handle 403s on LIST: the #495 canned-ACL default header is sent unsigned on non-PUT requests (blocks all source.coop fleet builds/sweeps) #522 territory.force=Truestays the documented unconditional-rewrite escape.Tracked in #532 — "column skip
gate cannot see a same-second leaf rewrite at unchanged granule count (record
the leaf's stamp identity in
zagg_column)".RESOLVED by events — nothing owed here. The handler forwarding of
families/partitionlanded in sweep handler: forward the partition and families blocks (issue #527) #528 (issue sweep handler drops the partition and families blocks: every partitioned fleet sweep silently sweeps the whole store (0.51.0 blocker) #527), so the family isreachable worker-side without any change to
lambda_handler.pyin this PR.Its lease-vs-partition sub-question — the runner fires partitions as
concurrent
Eventinvokes while the lease is store-granular, so a fleetbackfill wants either a single unpartitioned invoke or a scoped lease — is a
separate espg design call in progress. This PR's store-granular behavior is
left alone pending that ruling, and a fleet-scale partitioned backfill is
gated on it.
Informational, no action. Pre-existing lint on
main, deliberately nottouched (and one bulk-format sweep-in reverted in
6eb01e6):src/zagg/registry.py:64N818, andtests/data/benchmark/README.mdfailsruff format --check.