Centralize partition metadata access and add layer introspection APIs

Replaced scattered direct metadata loading with centralized instance methods on `KmerPartition` to guarantee consistent error mapping and legacy recovery. Introduced `StorageKind`, `LayerContent`, and `EvidenceKind` enums alongside lightweight disk-probe methods that inspect file presence without opening heavy data structures. Updated callers across the index, partitioner, and phylo modules to use the new partition API, and added unit tests validating the introspection behavior.
This commit is contained in:
Eric Coissac
2026-08-20 15:29:53 +02:00
parent f7ebc7a1ab
commit 76cbd3a886
23 changed files with 580 additions and 74 deletions
@@ -1,7 +1,9 @@
# Partition and layer caching (discussion)
Status: problem confirmed, design direction agreed (2026-08-20). No
implementation started. Ownership split (below) still open.
Status: problem confirmed, design direction agreed (2026-08-20). (1)/(2)
themselves not started; ownership split (below) still open. Preparatory
encapsulation work (partition/layer path and metadata accessors on
`KmerPartition`) landed the same day — see "Preparatory work done" below.
## The problem
@@ -127,3 +129,116 @@ deleted outright or kept as a thin sibling-specific wrapper, and whether (2)
needs an eviction policy or can simply hold every partition open for the
process lifetime (revisit once the VM-mapping-count question above has a
real number behind it for this codebase's scale).
## Preparatory work done (2026-08-20)
Groundwork for (1)/(2), landed ahead of the design itself:
- `KmerPartition` (`obikpartitionner`) gained `partition_dir`/`index_dir`/
`layer_dir` as the single source of truth for a partition's on-disk
layout, replacing per-module duplicated `const INDEX_SUBDIR: &str =
"index"` (7 copies) and ad hoc path joins — including one found
duplicated *inside `KmerPartition` itself* (`ensure_writer` rebuilt
`part_dir`'s own logic by hand).
- `KmerPartition` gained `partition_meta`/`n_layers`/`index_mode`, wrapping
`obilayeredmap::meta::PartitionMeta::load` (via the existing
`common::load_meta`, which also recovers indexes built before
`meta.json` existed). Before this, `obikphylo` and `obikindex` imported
`obilayeredmap::meta::PartitionMeta` directly and called `::load()`
themselves at 21 call sites, each redoing its own error-mapping —
every one of those crates knew the on-disk metadata format instead of
going through an interface. Fixed everywhere except one remaining spot
(below). Caught as a side effect: `dump_layer.rs`/`query_layer.rs` had
been calling `PartitionMeta::load` directly, bypassing `load_meta`
entirely — they never got the missing-`meta.json` recovery the other
callers did.
- Layer introspection API discussed but **not yet implemented** — three
axes, deliberately kept separate after an initial draft conflated them:
- `LayerContent { Count, Presence }` — what the layer stores; a `const`
on `LayerData` (compile-time, zero-cost), not a runtime field.
- `StorageKind { Implicit, Columnar, Packed, Sparse }` — how it's
stored; only meaningful for `D` that actually carry data (`Layer<()>`
has neither this nor `LayerContent` — it's a write-time-only state,
never a queryable content: once a layer is closed, "no matrix file"
reads back as `Presence`/`Implicit` via `PersistentBitMatrix::open`'s
own fallback, not as some third "empty" content).
- `EvidenceKind { Exact, Approx, Hybrid }` — from `MphfLayer`'s own
already-in-memory `LayerEvidence` discriminant.
- Not all `(LayerContent, StorageKind)` pairs are legal: `Count` never
has `Implicit` or `Sparse`.
**Implemented (2026-08-20).** `LayerContent`/`StorageKind`/`EvidenceKind`
now exist, each with two forms:
- A runtime accessor on an already-open value (`Layer<D>::content()`/
`storage_kind()`/`evidence_kind()`, `PersistentBitMatrix::storage_kind()`,
`PersistentCompactIntMatrix::storage_kind()`, `MphfLayer::evidence_kind()`)
— reads a discriminant already in memory, zero disk access.
- A lightweight `detect()`/`detect_storage()` disk probe that mirrors the
corresponding `open()`'s own priority order by hand (file-existence
checks only, no mmap) — usable *before* committing to a `D`, unlike the
runtime accessors. Exposed per-layer on `LayeredMap<D>` as
`detect_layer_content`/`detect_layer_storage`/`detect_layer_evidence`
(work regardless of `D`, since they only use `self.root` + the layer
index).
`StorageKind` lives in `obicompactvec` (owner of `PersistentBitMatrix`/
`PersistentCompactIntMatrix`); `LayerContent`/`EvidenceKind` live in
`obilayeredmap`. `HasLayerContent`/`HasStorageKind` gate `Layer<()>` out of
`content()`/`storage_kind()` (no matrix, nothing to report), matching the
"empty is transitional" conclusion above. 42 new tests across
`obilayeredmap`'s `tests/layer.rs` and `tests/map.rs`; full workspace
suite green (0 failed) after.
Not done: these `detect()` probes don't yet replace `Mat::open`'s or
`QueryLayer::open`'s own hand-rolled equivalents (still duplicated content/
storage decisions, now a *third* copy of the same logic to keep in sync)
— that consolidation is (1)/(2)'s job, not this prep step's.
## One bug found while reading around this (signalled, not fixed); one earlier claim retracted
- `obicompactvec::bitmatrix::sparse.rs`'s module doc says
"Not used by any production code path yet" — false since `obikmer pack
--sparse` (`cmd/pack/mod.rs`) is wired to `pack_sparse_bit_matrix` and
`Mat::open` already reads the result back in the sibling-annex path.
Stale comment, not corrected.
- **Retracted (2026-08-20)**: an earlier pass through this doc claimed
`obikpartitionner::query_layer::QueryLayer::open` had no sparse-format
detection and would silently corrupt reads on a `pack --sparse`d layer.
False — `PersistentBitMatrix` (`obicompactvec::bitmatrix::persistent`)
is a 4-way enum (`Columnar`/`Packed`/`Sparse`/`Implicit`), not 3-way as
first read; its `open()` already detects `Sparse` via
`presence/sparse_meta.json`, and every method on the type (`row`,
`fill_row`, `nonzero_iter`, …) already dispatches all 4 arms.
`QueryLayer::open`'s `PersistentBitMatrix::open(layer_dir)` call was
never the bug. Root cause of the false claim: a `grep -n
"Implicit\|Columnar\|Packed"` used to read the enum definition silently
skipped the `Sparse(...)` line because it matched none of those three
words — a self-inflicted blind spot from a filtered read, not a fact
about the code. Lesson: for a `pub enum` whose variant list matters,
read the definition unfiltered, don't grep for the variant names you
expect to find.
- One real consequence of that same correction: `obikphylo::siblings::
cache::Mat::SparsePresence(Layer<PersistentSparseBitMatrix>)` looks
redundant now — `Mat::Presence(Layer<PersistentBitMatrix>)` alone
would already handle sparse layers transparently, since
`PersistentBitMatrix` absorbs `Sparse` internally. Likely `Mat` predates
`PersistentBitMatrix` growing native sparse support. Signalled, not
removed — no mandate to touch `obikphylo` for this.
## Remaining instance of the PartitionMeta-encapsulation problem
`obikphylo::siblings::family_scan::scan_layer_families` still re-derives
`index_dir` from `layer_dir.parent()` and calls `PartitionMeta::load`
itself, purely to get `.mode` for `Mat::open`. Fixing it the way the 21
other call sites were fixed needs more than a 1:1 swap: `scan_layer_families`
only receives a bare `layer_dir: &Path`, not a `(partition, part, layer)`
triple, and its single upstream source of layer paths,
`sibling_layer_dirs`, returns a flat `Vec<PathBuf>` with the partition/layer
indices already discarded. Fixing it properly means either having
`sibling_layer_dirs` return `(PathBuf, IndexMode)` (or `(part, layer)`)
pairs, or threading `&KmerPartition` + indices through instead of paths —
and touching every one of `scan_layer_families`'s 8 callers (`distance.rs`,
`alignment.rs`, `cardinality.rs`, `entropy.rs` ×2, `sankoff_bundle.rs` ×2,
`stats.rs`). Left alone this round; worth doing as part of the same pass
that builds (1)/(2), since those callers are exactly the sibling-annex
consumers (2) is meant to serve.