From c9d10d55c76872a8a1e71c06ebee930a96855657 Mon Sep 17 00:00:00 2001 From: Eric Coissac Date: Fri, 21 Aug 2026 15:22:33 +0200 Subject: [PATCH] Simplify MphfLayer::open by removing explicit IndexMode parameter The MphfLayer::open signature now accepts only a directory path. Mode configuration is resolved internally via disk-based auto-detection using LayerEvidence, eliminating the need for explicit arguments from callers. All internal query methods and external call sites have been updated accordingly, while TypedLayer::open temporarily retains the unused parameter for future cleanup. --- src/obikindex/src/index/dump_layer.rs | 10 +- src/obikindex/src/index/query_layer.rs | 8 +- src/obikindex/src/index/rebuild_layer.rs | 2 +- src/obikindex/src/layer/mphf_layer.rs | 178 ++++++++++++++--------- src/obikindex/src/layer/typed_layer.rs | 8 +- src/obikphylo/src/siblings/build.rs | 5 +- src/obikphylo/src/siblings/tests.rs | 2 +- 7 files changed, 128 insertions(+), 85 deletions(-) diff --git a/src/obikindex/src/index/dump_layer.rs b/src/obikindex/src/index/dump_layer.rs index a4c77ff2..69fca0d5 100644 --- a/src/obikindex/src/index/dump_layer.rs +++ b/src/obikindex/src/index/dump_layer.rs @@ -1,6 +1,6 @@ use obicompactvec::{PersistentBitMatrix, PersistentCompactIntMatrix}; use obikseq::CanonicalKmer; -use crate::layer::{IndexMode, MphfLayer, OLMError}; +use crate::layer::{MphfLayer, OLMError}; use obiskio::{SKError, SKResult, UnitigFileReader}; use crate::index::filter::{KmerFilter, passes_all}; @@ -41,8 +41,6 @@ impl KmerIndex { return Ok(true); } - let index_mode = self.partition_mode(part).unwrap_or(IndexMode::Exact); - let mut l = 0; loop { let layer_dir = self.layer_dir(part, l); @@ -50,7 +48,7 @@ impl KmerIndex { break; } l += 1; - let mphf = MphfLayer::open(&layer_dir, &index_mode).map_err(olm_to_sk)?; + let mphf = MphfLayer::open(&layer_dir).map_err(olm_to_sk)?; let reader = UnitigFileReader::open_sequential(&layer_dir.join("unitigs.bin"))?; let counts_dir = layer_dir.join("counts"); @@ -131,15 +129,13 @@ impl KmerIndex { return Ok(true); } - let index_mode = self.partition_mode(part).unwrap_or(IndexMode::Exact); - let mut layer = 0; loop { let layer_dir = self.layer_dir(part, layer); if !layer_dir.exists() { break; } - let mphf = MphfLayer::open(&layer_dir, &index_mode).map_err(olm_to_sk)?; + let mphf = MphfLayer::open(&layer_dir).map_err(olm_to_sk)?; let reader = UnitigFileReader::open_sequential(&layer_dir.join("unitigs.bin"))?; let counts_dir = layer_dir.join("counts"); diff --git a/src/obikindex/src/index/query_layer.rs b/src/obikindex/src/index/query_layer.rs index 9fb8422e..7cfd3084 100644 --- a/src/obikindex/src/index/query_layer.rs +++ b/src/obikindex/src/index/query_layer.rs @@ -3,7 +3,7 @@ use std::path::Path; use obicompactvec::{PersistentBitMatrix, PersistentCompactIntMatrix}; use obikseq::CanonicalKmer; -use crate::layer::{IndexMode, MphfLayer, OLMError}; +use crate::layer::{MphfLayer, OLMError}; use obiskio::{SKError, SKResult}; use crate::index::kmer_index::KmerIndex; @@ -26,8 +26,8 @@ enum QueryLayer { } impl QueryLayer { - fn open(layer_dir: &Path, with_counts: bool, mode: &IndexMode) -> SKResult { - let mphf = MphfLayer::open(layer_dir, mode).map_err(olm_to_sk)?; + fn open(layer_dir: &Path, with_counts: bool) -> SKResult { + let mphf = MphfLayer::open(layer_dir).map_err(olm_to_sk)?; let counts_dir = layer_dir.join("counts"); let presence_dir = layer_dir.join("presence"); @@ -185,7 +185,7 @@ impl KmerIndex { let meta = self.partition_meta(part_idx)?; let layers: Vec = (0..meta.n_layers) - .map(|i| QueryLayer::open(&self.layer_dir(part_idx, i), with_counts, &meta.mode)) + .map(|i| QueryLayer::open(&self.layer_dir(part_idx, i), with_counts)) .collect::>()?; // ── Stage 1: MPHF-only pass, bucket hits by (layer_idx, slot) ──────── diff --git a/src/obikindex/src/index/rebuild_layer.rs b/src/obikindex/src/index/rebuild_layer.rs index adaf6379..530df658 100644 --- a/src/obikindex/src/index/rebuild_layer.rs +++ b/src/obikindex/src/index/rebuild_layer.rs @@ -235,7 +235,7 @@ impl KmerIndex { let dst_layer_dir = self.layer_dir(i, 0); let n_new = materialize_layer(g, &dst_layer_dir, block_bits, &IndexMode::Exact)?; - let dst_mphf = MphfLayer::open(&dst_layer_dir, &IndexMode::Exact) + let dst_mphf = MphfLayer::open(&dst_layer_dir) .map_err(|e| olm_to_sk(e, "rebuild"))?; // ── Prepare matrix builders (one column per genome) ─────────────────── diff --git a/src/obikindex/src/layer/mphf_layer.rs b/src/obikindex/src/layer/mphf_layer.rs index b290d1d9..a75e9340 100644 --- a/src/obikindex/src/layer/mphf_layer.rs +++ b/src/obikindex/src/layer/mphf_layer.rs @@ -29,23 +29,89 @@ type MphfEps = PtrHash, Xx // ── LayerEvidence ───────────────────────────────────────────────────────────── +/// Pure identity — paths only, no disk I/O beyond the existence probe in +/// [`at`](Self::at). `unitig_path` appears in `Exact`/`Hybrid` only: for +/// those two modes, `unitigs.bin` is genuinely part of what "evidence" +/// means (`evidence.decode` gives a `(chunk_id, rank)` pointer, `unitigs. +/// verify_canonical_kmer` is what completes the check) — for `Approx`, +/// `find` never touches unitigs at all, that's what makes it approximate. enum LayerEvidence { + Exact { + evidence_path: PathBuf, + unitig_path: PathBuf, + }, + Approx { + fingerprint_path: PathBuf, + }, + Hybrid { + evidence_path: PathBuf, + fingerprint_path: PathBuf, + unitig_path: PathBuf, + }, +} + +/// Resources actually opened (mmap). `Exact`/`Hybrid` carry their own +/// `unitigs` — needed to complete evidence verification, not shared with +/// `MphfLayer`'s own separate `unitigs` (used by `iter_kmers`/`Approx`'s +/// `find_strict` fallback, mode-independent). Yes, this means a layer's +/// `unitigs.bin` gets mmapped twice for Exact/Hybrid — deliberately not +/// optimised away here: avoiding that redundant open is the future cache +/// crate's job, not this base type's. +enum OpenLayerEvidence { Exact { evidence: Evidence, unitigs: Arc, }, Approx { fingerprint: FingerprintVec, - unitigs: Arc, - unitigs_path: PathBuf, }, Hybrid { evidence: Evidence, - unitigs: Arc, fingerprint: FingerprintVec, + unitigs: Arc, }, } +impl LayerEvidence { + /// Identify which evidence files exist at `layer_dir`, without opening + /// any of them — a lightweight disk probe (same signal `EvidenceKind:: + /// detect` uses), not a load. + fn at(layer_dir: &Path) -> OLMResult { + Ok(match EvidenceKind::detect(layer_dir)? { + EvidenceKind::Exact => LayerEvidence::Exact { + evidence_path: layer_dir.join(EVIDENCE_FILE), + unitig_path: layer_dir.join(UNITIGS_FILE), + }, + EvidenceKind::Approx => LayerEvidence::Approx { + fingerprint_path: layer_dir.join(FINGERPRINT_FILE), + }, + EvidenceKind::Hybrid => LayerEvidence::Hybrid { + evidence_path: layer_dir.join(EVIDENCE_FILE), + fingerprint_path: layer_dir.join(FINGERPRINT_FILE), + unitig_path: layer_dir.join(UNITIGS_FILE), + }, + }) + } + + /// mmap the evidence file(s) this identity points to. + fn open(self) -> OLMResult { + Ok(match self { + LayerEvidence::Exact { evidence_path, unitig_path } => OpenLayerEvidence::Exact { + evidence: Evidence::open(&evidence_path)?, + unitigs: Arc::new(UnitigFileReader::open(&unitig_path)?), + }, + LayerEvidence::Approx { fingerprint_path } => OpenLayerEvidence::Approx { + fingerprint: FingerprintVec::open(&fingerprint_path)?, + }, + LayerEvidence::Hybrid { evidence_path, fingerprint_path, unitig_path } => OpenLayerEvidence::Hybrid { + evidence: Evidence::open(&evidence_path)?, + fingerprint: FingerprintVec::open(&fingerprint_path)?, + unitigs: Arc::new(UnitigFileReader::open(&unitig_path)?), + }, + }) + } +} + // ── EvidenceKind ────────────────────────────────────────────────────────────── /// Coarse evidence mode of a layer — the `IndexMode` a layer was opened @@ -85,54 +151,40 @@ impl EvidenceKind { /// - [`find_strict`](Self::find_strict) — always exact; O(1) on Exact/Hybrid layers, /// O(n) sequential scan on Approx layers. pub struct MphfLayer { + layer_dir: PathBuf, + /// Mode-independent copy — used by `iter_kmers` (all modes) and + /// `find_strict`'s `Approx` fallback. `Exact`/`Hybrid` additionally + /// carry their own inside `OpenLayerEvidence` (see its doc comment for + /// why that's a second, deliberately unshared mmap of the same file, + /// not a bug). + unitigs: Arc, + mphf: MemCase, - ev: LayerEvidence, + ev: OpenLayerEvidence, n: usize, } impl MphfLayer { - /// Open a layer using the index-level `mode` determined at `LayeredMap` open time. - /// No per-layer metadata file is read. - pub fn open(dir: &Path, mode: &IndexMode) -> OLMResult { + /// This layer's own directory. + pub(crate) fn dir(&self) -> &Path { + &self.layer_dir + } + + /// Open a layer — evidence mode is auto-detected from which files exist + /// on disk (see [`LayerEvidence::at`]), never passed in: the caller + /// can't tell this type anything about its own mode that isn't already + /// derivable from `layer_dir` itself. + pub fn open(dir: &Path) -> OLMResult { let mphf: MemCase = Mphf::mmap(&dir.join(MPHF_FILE), Flags::empty()) .map_err(|e| OLMError::InvalidLayer(e.to_string()))?; - let (ev, n) = match mode { - IndexMode::Exact => { - let evidence = Evidence::open(&dir.join(EVIDENCE_FILE))?; - let n = evidence.len(); - let unitigs = Arc::new(UnitigFileReader::open(&dir.join(UNITIGS_FILE))?); - (LayerEvidence::Exact { evidence, unitigs }, n) - } - IndexMode::Approx { .. } => { - let fingerprint = FingerprintVec::open(&dir.join(FINGERPRINT_FILE))?; - let n = fingerprint.n(); - let unitigs = Arc::new(UnitigFileReader::open(&dir.join(UNITIGS_FILE))?); - let unitigs_path = dir.join(UNITIGS_FILE); - ( - LayerEvidence::Approx { - fingerprint, - unitigs, - unitigs_path, - }, - n, - ) - } - IndexMode::Hybrid { .. } => { - let evidence = Evidence::open(&dir.join(EVIDENCE_FILE))?; - let fingerprint = FingerprintVec::open(&dir.join(FINGERPRINT_FILE))?; - let n = evidence.len(); - let unitigs = Arc::new(UnitigFileReader::open(&dir.join(UNITIGS_FILE))?); - ( - LayerEvidence::Hybrid { - evidence, - unitigs, - fingerprint, - }, - n, - ) - } + let unitigs = Arc::new(UnitigFileReader::open(&dir.join(UNITIGS_FILE))?); + let ev = LayerEvidence::at(dir)?.open()?; + let n = match &ev { + OpenLayerEvidence::Exact { evidence, .. } => evidence.len(), + OpenLayerEvidence::Hybrid { evidence, .. } => evidence.len(), + OpenLayerEvidence::Approx { fingerprint } => fingerprint.n(), }; - Ok(Self { mphf, ev, n }) + Ok(Self { layer_dir: dir.to_owned(), unitigs, mphf, ev, n }) } // ── Query API ───────────────────────────────────────────────────────────── @@ -148,9 +200,7 @@ impl MphfLayer { return None; } match &self.ev { - LayerEvidence::Exact { - evidence, unitigs, .. - } => { + OpenLayerEvidence::Exact { evidence, unitigs } => { let (chunk_id, rank) = evidence.decode(slot); if unitigs.verify_canonical_kmer(chunk_id as usize, rank as usize, kmer) { Some(slot) @@ -158,8 +208,8 @@ impl MphfLayer { None } } - LayerEvidence::Approx { fingerprint, .. } - | LayerEvidence::Hybrid { fingerprint, .. } => { + OpenLayerEvidence::Approx { fingerprint } + | OpenLayerEvidence::Hybrid { fingerprint, .. } => { if fingerprint.matches(slot, kmer.seq_hash()) { Some(slot) } else { @@ -180,12 +230,8 @@ impl MphfLayer { return None; } match &self.ev { - LayerEvidence::Exact { - evidence, unitigs, .. - } - | LayerEvidence::Hybrid { - evidence, unitigs, .. - } => { + OpenLayerEvidence::Exact { evidence, unitigs } + | OpenLayerEvidence::Hybrid { evidence, unitigs, .. } => { let (chunk_id, rank) = evidence.decode(slot); if unitigs.verify_canonical_kmer(chunk_id as usize, rank as usize, kmer) { Some(slot) @@ -193,9 +239,14 @@ impl MphfLayer { None } } - LayerEvidence::Approx { unitigs_path, .. } => { - let reader = UnitigFileReader::open_sequential(unitigs_path).ok()?; - for (stored, _, _) in reader.iter_indexed_canonical_kmers() { + OpenLayerEvidence::Approx { .. } => { + // `self.unitigs` was opened via `UnitigFileReader::open`, which + // falls back to sequential mode automatically when no `.idx` + // sidecar exists — true for every Approx layer (`build_unitig_idx` + // is only ever called for Exact/Hybrid). Reusing it here avoids + // the second file-open + mmap + kmer-count scan a fresh + // `open_sequential` call would repeat on every call. + for (stored, _, _) in self.unitigs.iter_indexed_canonical_kmers() { if self.mphf.index(&stored.raw()) == slot { return if stored == kmer { Some(slot) } else { None }; } @@ -214,9 +265,9 @@ impl MphfLayer { /// already in memory, no disk access. pub fn evidence_kind(&self) -> EvidenceKind { match &self.ev { - LayerEvidence::Exact { .. } => EvidenceKind::Exact, - LayerEvidence::Approx { .. } => EvidenceKind::Approx, - LayerEvidence::Hybrid { .. } => EvidenceKind::Hybrid, + OpenLayerEvidence::Exact { .. } => EvidenceKind::Exact, + OpenLayerEvidence::Approx { .. } => EvidenceKind::Approx, + OpenLayerEvidence::Hybrid { .. } => EvidenceKind::Hybrid, } } @@ -250,13 +301,8 @@ impl MphfLayer { /// concurrently for as long as the underlying file exists, because each /// carries its own cursor. pub fn iter_kmers(&self) -> KmerIter { - let reader = match &self.ev { - LayerEvidence::Exact { unitigs, .. } => unitigs, - LayerEvidence::Approx { unitigs, .. } => unitigs, - LayerEvidence::Hybrid { unitigs, .. } => unitigs, - }; KmerIter { - inner: Box::new(reader.iter_indexed_canonical_kmers_owned()), + inner: Box::new(self.unitigs.iter_indexed_canonical_kmers_owned()), } } diff --git a/src/obikindex/src/layer/typed_layer.rs b/src/obikindex/src/layer/typed_layer.rs index 82986704..11809725 100644 --- a/src/obikindex/src/layer/typed_layer.rs +++ b/src/obikindex/src/layer/typed_layer.rs @@ -201,8 +201,12 @@ pub struct Hit { // ── Common read path ────────────────────────────────────────────────────────── impl TypedLayer { - pub fn open(path: &Path, mode: &IndexMode) -> OLMResult { - let mphf = MphfLayer::open(path, mode)?; + // `mode` no longer forwarded to `MphfLayer::open` (auto-detected from + // disk now, see mphf_layer.rs) — kept as a parameter for the moment so + // this signature (and `KmerLayer::open`'s call to it) doesn't have to + // change in the same step; dropped when content_layer.rs is done. + pub fn open(path: &Path, _mode: &IndexMode) -> OLMResult { + let mphf = MphfLayer::open(path)?; let data = D::open(path)?; Ok(Self { mphf, data }) } diff --git a/src/obikphylo/src/siblings/build.rs b/src/obikphylo/src/siblings/build.rs index 7760d684..090ea03b 100644 --- a/src/obikphylo/src/siblings/build.rs +++ b/src/obikphylo/src/siblings/build.rs @@ -6,7 +6,6 @@ use rayon::prelude::*; use obikseq::CanonicalKmer; use obikindex::layer::MphfLayer; -use obikindex::layer::meta::IndexMode; use obipipeline::ThrottleGuard; use obisys::progress_bar; @@ -98,7 +97,6 @@ impl SiblingAnnexBuildExt for KmerIndex { part_slots += build_layer_sibling_annex( self, &self.layer_dir(part, l), - &meta.mode, n_parts, l, &cache, @@ -123,12 +121,11 @@ impl SiblingAnnexBuildExt for KmerIndex { fn build_layer_sibling_annex( index: &KmerIndex, layer_dir: &Path, - mode: &IndexMode, n_parts: usize, l: usize, cache: &Arc, ) -> OKIResult { - let mphf = MphfLayer::open(layer_dir, mode).map_err(olm_to_ok)?; + let mphf = MphfLayer::open(layer_dir).map_err(olm_to_ok)?; let k = index.kmer_size(); let n = mphf.n(); diff --git a/src/obikphylo/src/siblings/tests.rs b/src/obikphylo/src/siblings/tests.rs index 85acd873..880e67af 100644 --- a/src/obikphylo/src/siblings/tests.rs +++ b/src/obikphylo/src/siblings/tests.rs @@ -99,7 +99,7 @@ fn annex_info_for(idx: &KmerIndex, kmer: CanonicalKmer) -> FamilyMask { let meta = idx.partition_meta(0).unwrap(); for l in 0..meta.n_layers { let layer_dir = idx.layer_dir(0, l); - let mphf = MphfLayer::open(&layer_dir, &meta.mode).unwrap(); + let mphf = MphfLayer::open(&layer_dir).unwrap(); if let Some(slot) = mphf.find(kmer) { let annex = SiblingAnnex::open(&layer_dir.join(ANNEX_FILE_NAME)).unwrap(); return annex.get(slot).expect("slot must have a computed annex entry");