DiskSeismicIndex does not override add_with_ids and cannot be used with IDMapIndex

[chishui]

What is the bug?

DiskSeismicIndex does not override Index::add_with_ids, so it cannot be used with external document ids the way the other seismic indexes can.

Calling it directly falls through to the base implementation, which throws:

// nsparse/index.cpp:58
void Index::add_with_ids(...) {
    throw_not_implemented("add_with_ids not implemented in Index");
}

IDMapIndex is the wrapper that normally provides id mapping (idmap,seismic_sq, idmap,seismic), and it does not require the delegate to override add_with_ids — IDMapIndex::add_with_ids routes to delegate_->add() (nsparse/id_map_index.cpp:70-80). So an in-memory idmap,disk_seismic does accept ids, but the combination still cannot be used end-to-end, because the wrapped index cannot be loaded back:

• IDMapIndex::read_index always does a copying read of its delegate — delegate_.reset(nsparse::detail::read_index(io_reader, true, io_flags)) (nsparse/id_map_index.cpp:101).
• DiskSeismicIndex::read_index unconditionally throws, since its inline forward index is borrowed from a mapping and never copied to the heap (nsparse/disk_seismic_index.cpp:311-317):
  "DiskSeismicIndex is mmap-only; load with read_index(file, IndexIoFlag::kUseMmap)".
• mmap_index_payload() in nsparse/io/index_io.cpp has no entry for IDMP, so the mmap fast path can only be taken for a top-level DiskSeismicIndex, not for one nested under an id map. The nested path also inherits InlineForwardIndex's alignment expectations about where the payload starts, which the id-map prefix shifts.

There is no test or demo covering idmap + disk_seismic (tests/disk_seismic_index_test.cpp never mentions IDMapIndex), which is why this went unnoticed.

How can one reproduce the bug?

Direct call:

nsparse::DiskSeismicIndex index(dim);
index.add_with_ids(n, indptr, indices, values, ids);  // throws: add_with_ids not implemented in Index

Through the id map (Python, same shape as demos/seismic_sq_idmap.py):

index = nsparse.index_factory(dim, "idmap,disk_seismic,lambda=10|beta=2|alpha=0.4")
index.add_with_ids(n, indptr, indices, values, ids)   # ok — routed to delegate add()
index.build()
nsparse.write_index(index, "idmap_dsei.bin")          # ok
nsparse.read_index("idmap_dsei.bin")                  # throws: DiskSeismicIndex is mmap-only
nsparse.read_index("idmap_dsei.bin", nsparse.IndexIoFlag_kUseMmap)  # IDMP has no mmap payload reader

What is the expected behavior?

disk_seismic should support external ids like seismic and seismic_sq do: either DiskSeismicIndex overrides add_with_ids, or IDMapIndex learns to carry an mmap-only delegate (an IDMP entry in mmap_index_payload() plus alignment-correct nesting of the payload), with tests covering idmap,disk_seismic add → build → search → write → mmap-load → search.

If the combination is not intended to be supported, it should fail loudly at construction time (e.g. index_factory rejecting idmap,disk_seismic) rather than only when the index is read back.

Do you have any additional context?

Introduced with #32 (DiskSeismic: Add disk_seismic_index).

[zirui-song-18]

Thanks for calling out. Will reach back to you after a careful review.

[zirui-song-18]

Thanks for the detailed writeup. I dug into this and reproduced it — the good news is that idmap,disk_seismic works end-to-end today; it just has to be loaded with kUseMmap (like any DiskSeismicIndex). There are a couple of spots in the analysis I think are slightly off, and one point that's genuinely valid.

Repro (the exact steps from the issue)

idx = index_factory(dim, "idmap,disk_seismic,lambda=10|beta=2|alpha=0.4")
idx.add_with_ids(n, indptr, indices, values, ids)   # ok — IDMapIndex routes to delegate.add()
idx.build(); write_index(idx, path)                 # ok
read_index(path)                 → RuntimeError "DiskSeismicIndex is mmap-only"   # as you noted
read_index(path, nsparse.kUseMmap) → loads OK; num_vectors correct; search returns the external ids

So the copying read throws (expected — DiskSeismic is mmap-only), but the mmap read loads and searches correctly.

Why the nested mmap path does work

IDMapIndex::read_index forwards io_flags to its delegate — id_map_index.cpp:101:

delegate_.reset(nsparse::detail::read_index(io_reader, /*keep_open=*/true, io_flags));

The true there is keep_open, not "copying"; io_flags (incl. kUseMmap) is passed through. And you're right that mmap_index_payload has no IDMP case — but that just makes it return nullptr for the top-level IDMP, so read_index falls through to IDMapIndex::read_index(io_flags) (index_io.cpp:168), which then hands kUseMmap to the delegate, whose nested read_index hits mmap_index_payload(DSEI, …, io_reader->pos()) → DiskSeismicIndex::mmap_index. The payload offset is io_reader->pos() (already past the id-map prefix), and the inline-forward padding is relative to the stream position, so alignment holds — the search returns correct results in the repro.

(Minor: the snippet uses nsparse.IndexIoFlag_kUseMmap, which isn't exposed in the bindings — the flag is nsparse.kUseMmap. That might be why the mmap attempt looked like it failed.)

On add_with_ids

No index overrides it — SeismicIndex/SeismicScalarQuantizedIndex/DiskSeismicIndex all inherit Index::add_with_ids (which throws), and external ids for all of them go through IDMapIndex::add_with_ids → delegate_->add(). So a direct DiskSeismicIndex::add_with_ids throwing is the same as SeismicIndex::add_with_ids; disk_seismic is consistent with the others here.

The valid part

There is an asymmetry — read_index(path) (no flag) works for idmap, seismic but throws for idmap, disk_seismic, because the delegate is mmap-only. This is the same contract as a top-level DiskSeismicIndex (which also requires kUseMmap), just propagated through the id map, so it's by-design rather than a correctness bug — but I agree it's an easy footgun. A test covering idmap, disk_seismic (add_with_ids → build → write → mmap-load → search, checking external ids + recall) is included in the follow-up PR (#33 ) and passes.

Happy to do either of

1. leave the mmap-only contract as-is and add a doc note + a clearer error when a mmap-only delegate is read without kUseMmap; or
2. make idmap transparently mmap-load a mmap-only delegate so read_index(path) "just works"

Which would you prefer?

[chishui]

it's not a bug, closing