diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md index 0f071a3..269bd45 100644 --- a/DEVELOPER_GUIDE.md +++ b/DEVELOPER_GUIDE.md @@ -383,6 +383,28 @@ If your change alters what the Python bindings expose or how an index behaves en If your changes could affect backward compatibility, please include relevant tests along with your PR. +### Index file format + +Every serialized index starts with a fixed header (`nsparse::IndexHeader` in `nsparse/io/io.h`). It is written and parsed centrally, by `write_header`/`read_header` in `nsparse/io/index_io.cpp` — an index type never reads its own header, it receives the already-parsed one: + +| Field | Type | Notes | +|---|---|---| +| id | `uint32` | fourcc of the index type, e.g. `SEIS` | +| version | `uint32` | layout revision of the payload that follows | +| dimension | `int32` | | + +The payload follows immediately, and its layout is the index type's own business. A type parses it in `read_index` (copying), `mmap_index` (borrowed from a file mapping), or both — `DiskSeismicIndex` is mmap-only and its `read_index` just throws, while `IDMapIndex` has no `mmap_index` at all and instead threads `io_flags` down to its delegate. + +Versions are numbered **per index type**, not per file: `IndexIO::format_version()` returns the type's own `kFormatVersion`, so revising one type's payload leaves the others' numbering alone. An `IDMapIndex` writes its own header for the id map and then a second, complete header for the delegate it wraps, each with its own version. + +The same central code rejects a version outside `1..format_version()` before dispatching to either parse path — a file from a newer build fails with a clear error instead of consuming whatever its fields happen to align with. + +To change a payload layout: + +1. Bump that type's `kFormatVersion`. +2. Branch on `header.version` wherever that type actually parses its payload — its `read_index` and/or its `mmap_index` — keeping the older branch so existing files still load. +3. Add a round-trip test for the new version and a test that reads the old layout. + ### Outdated or irrelevant code Do not submit code that is not used or needed, even if it's commented. We rely on GitHub as a version control system; code can be restored if needed. diff --git a/nsparse/disk_seismic_index.cpp b/nsparse/disk_seismic_index.cpp index 4b5b9bf..4cc6cb4 100644 --- a/nsparse/disk_seismic_index.cpp +++ b/nsparse/disk_seismic_index.cpp @@ -312,7 +312,9 @@ void DiskSeismicIndex::write_index(IOWriter* io_writer) { forward.serialize(io_writer); } -void DiskSeismicIndex::read_index(IOReader* /*io_reader*/, int /*io_flags*/) { +void DiskSeismicIndex::read_index(IOReader* /*io_reader*/, + const IndexHeader& /*header*/, + int /*io_flags*/) { // The inline forward index is borrowed from a mapping, never copied onto // the heap, so this index has no copying read path. throw std::runtime_error( @@ -320,11 +322,11 @@ void DiskSeismicIndex::read_index(IOReader* /*io_reader*/, int /*io_flags*/) { "IndexIoFlag::kUseMmap)"); } -DiskSeismicIndex* DiskSeismicIndex::mmap_index(int dimension, +DiskSeismicIndex* DiskSeismicIndex::mmap_index(const IndexHeader& header, const char* index_file, size_t pos) { throw_if_null(index_file, "index_file must not be null"); - auto index = std::make_unique(dimension); + auto index = std::make_unique(header.dimension); MmapFile mmap_file(std::string{index_file}); MmapCursor cursor(mmap_file.data(), mmap_file.size()); diff --git a/nsparse/disk_seismic_index.h b/nsparse/disk_seismic_index.h index 60b18b4..a5c2054 100644 --- a/nsparse/disk_seismic_index.h +++ b/nsparse/disk_seismic_index.h @@ -11,6 +11,7 @@ #define DISK_SEISMIC_INDEX_H #include #include +#include #include #include "absl/container/flat_hash_set.h" @@ -52,6 +53,8 @@ struct DiskSeismicSearchParameters : public SeismicSearchParameters { class DiskSeismicIndex : public MmapIndex, public IndexIO { public: static constexpr std::array name = {'D', 'S', 'E', 'I'}; + // Bump whenever write_index's payload layout changes. + static constexpr uint32_t kFormatVersion = 1; explicit DiskSeismicIndex(int dim); DiskSeismicIndex(int dim, SeismicClusterParameters parameter); @@ -70,16 +73,20 @@ class DiskSeismicIndex : public MmapIndex, public IndexIO { // Borrows a serialized index from a file mapping. `pos` is where the // payload begins. - static DiskSeismicIndex* mmap_index(int dimension, const char* index_file, - size_t pos); + static DiskSeismicIndex* mmap_index(const IndexHeader& header, + const char* index_file, size_t pos); protected: std::vector clustered_inverted_lists; private: + [[nodiscard]] uint32_t format_version() const override { + return kFormatVersion; + } void write_index(IOWriter* io_writer) override; // Unsupported: the inline forward index is mmap-only. Throws. - void read_index(IOReader* io_reader, int io_flags = 0) override; + void read_index(IOReader* io_reader, const IndexHeader& header, + int io_flags = 0) override; auto search(idx_t n, const idx_t* indptr, const term_t* indices, const float* values, int k, diff --git a/nsparse/id_map_index.cpp b/nsparse/id_map_index.cpp index 9eb8afe..f95d606 100644 --- a/nsparse/id_map_index.cpp +++ b/nsparse/id_map_index.cpp @@ -89,7 +89,8 @@ void IDMapIndex::write_index(IOWriter* io_writer) { nsparse::detail::write_index(delegate_.get(), io_writer, true); } -void IDMapIndex::read_index(IOReader* io_reader, int io_flags) { +void IDMapIndex::read_index(IOReader* io_reader, const IndexHeader& header, + int io_flags) { // Read internal_to_external_ vector size_t map_size = 0; io_reader->read(&map_size, sizeof(size_t), 1); diff --git a/nsparse/id_map_index.h b/nsparse/id_map_index.h index a4ec1d7..6db9715 100644 --- a/nsparse/id_map_index.h +++ b/nsparse/id_map_index.h @@ -11,6 +11,7 @@ #define ID_MAP_INDEX_H #include #include +#include #include #include #include @@ -79,6 +80,9 @@ class IDMapIndex : public Index, public IndexIO { public: IDMapIndex() = default; static constexpr std::array name = {'I', 'D', 'M', 'P'}; + // Covers the id map only; the delegate that follows carries its own header + // and versions its payload independently. + static constexpr uint32_t kFormatVersion = 1; // Takes ownership of the delegate index; it is freed when this IDMapIndex // is destroyed. explicit IDMapIndex(Index*); @@ -97,8 +101,12 @@ class IDMapIndex : public Index, public IndexIO { void add_with_ids(idx_t n, const idx_t* indptr, const term_t* indices, const float* values, const idx_t* ids) override; + [[nodiscard]] uint32_t format_version() const override { + return kFormatVersion; + } void write_index(IOWriter* io_writer) override; - void read_index(IOReader* io_reader, int io_flags = 0) override; + void read_index(IOReader* io_reader, const IndexHeader& header, + int io_flags = 0) override; private: // Owns the wrapped delegate index. Using unique_ptr ensures the delegate is diff --git a/nsparse/inverted_index.cpp b/nsparse/inverted_index.cpp index a2b8f76..d24abbf 100644 --- a/nsparse/inverted_index.cpp +++ b/nsparse/inverted_index.cpp @@ -427,7 +427,8 @@ void InvertedIndex::write_index(IOWriter* io_writer) { io_align::write_padded(io_writer, max_term_scores_.data(), scores_size); } -void InvertedIndex::read_index(IOReader* io_reader, int io_flags) { +void InvertedIndex::read_index(IOReader* io_reader, const IndexHeader& header, + int io_flags) { size_t num_vectors = 0; io_reader->read(&num_vectors, sizeof(size_t), 1); @@ -450,10 +451,10 @@ void InvertedIndex::read_index(IOReader* io_reader, int io_flags) { } } -InvertedIndex* InvertedIndex::mmap_index(int dimension, const char* index_file, - size_t pos) { +InvertedIndex* InvertedIndex::mmap_index(const IndexHeader& header, + const char* index_file, size_t pos) { throw_if_null(index_file, "index_file must not be null"); - auto index = std::make_unique(dimension); + auto index = std::make_unique(header.dimension); MmapFile mmap_file(std::string{index_file}); // `pos` is where write_index's payload begins, past the header read_header diff --git a/nsparse/inverted_index.h b/nsparse/inverted_index.h index c506e36..b114292 100644 --- a/nsparse/inverted_index.h +++ b/nsparse/inverted_index.h @@ -11,6 +11,7 @@ #define INVERTED_INDEX_H #include +#include #include #include @@ -35,12 +36,14 @@ class InvertedIndex : public MmapIndex, public IndexIO { size_t num_vectors() const override { return num_vectors_; } std::array id() const override { return name; } static constexpr std::array name = {'I', 'N', 'V', 'T'}; + // Bump whenever write_index's payload layout changes. + static constexpr uint32_t kFormatVersion = 1; // Reads what write_index wrote, with the posting lists borrowing from a // mapping of `index_file` instead of being copied onto the heap. `pos` is // where the payload begins, past the header read_header consumed. - static InvertedIndex* mmap_index(int dimension, const char* index_file, - size_t pos); + static InvertedIndex* mmap_index(const IndexHeader& header, + const char* index_file, size_t pos); protected: auto search(idx_t n, const idx_t* indptr, const term_t* indices, @@ -50,8 +53,12 @@ class InvertedIndex : public MmapIndex, public IndexIO { private: // IndexIO overrides + [[nodiscard]] uint32_t format_version() const override { + return kFormatVersion; + } void write_index(IOWriter* io_writer) override; - void read_index(IOReader* io_reader, int io_flags) override; + void read_index(IOReader* io_reader, const IndexHeader& header, + int io_flags) override; // `id_selector` may be null; when set, only member docs are returned. auto single_query(const term_t* indices, const float* values, int size, diff --git a/nsparse/io/index_io.cpp b/nsparse/io/index_io.cpp index 3e41500..1afc9a3 100644 --- a/nsparse/io/index_io.cpp +++ b/nsparse/io/index_io.cpp @@ -9,9 +9,11 @@ #include "nsparse/io/index_io.h" +#include #include #include #include +#include #include "nsparse/brutal_index.h" #include "nsparse/disk_seismic_index.h" @@ -24,6 +26,8 @@ namespace nsparse { namespace { +constexpr uint32_t kBitsPerByte = 8; + constexpr uint32_t BRUT = fourcc(BrutalIndex::name); constexpr uint32_t SEIS = fourcc(SeismicIndex::name); constexpr uint32_t SESQ = fourcc(SeismicScalarQuantizedIndex::name); @@ -71,57 +75,92 @@ class StreamCloser { T* stream_; }; -// Reads `id`'s payload by borrowing it from the file rather than copying it, or -// returns nullptr for an index type without a mapped reader. `pos` is where the -// payload begins, past the header read_header consumed. -Index* mmap_index_payload(uint32_t id, int dimension, const char* file_name, +// Reads `header`'s payload by borrowing it from the file rather than copying +// it, or returns nullptr for an index type without a mapped reader. `pos` is +// where the payload begins, past the header read_header consumed. +Index* mmap_index_payload(const IndexHeader& header, const char* file_name, size_t pos) { - switch (id) { + switch (header.id) { case SEIS: - return SeismicIndex::mmap_index(dimension, file_name, pos); + return SeismicIndex::mmap_index(header, file_name, pos); case SESQ: - return SeismicScalarQuantizedIndex::mmap_index(dimension, file_name, + return SeismicScalarQuantizedIndex::mmap_index(header, file_name, pos); case INVT: - return InvertedIndex::mmap_index(dimension, file_name, pos); + return InvertedIndex::mmap_index(header, file_name, pos); case DSEI: - return DiskSeismicIndex::mmap_index(dimension, file_name, pos); + return DiskSeismicIndex::mmap_index(header, file_name, pos); default: return nullptr; } } -void write_header(Index* index, IOWriter* io_writer) { +// The id as it reads in the file, for error messages: a fourcc is four +// printable characters, and its numeric value is not what a reader would +// recognise. +std::string id_to_string(uint32_t id_val) { + std::string chars(4, '\0'); + for (size_t i = 0; i < chars.size(); ++i) { + chars[i] = static_cast((id_val >> (kBitsPerByte * i)) & 0xFFU); + } + return chars; +} + +void write_header(const IndexHeader& header, IOWriter* io_writer) { // write index type - auto id_val = fourcc(index->id()); + uint32_t id_val = header.id; io_writer->write(&id_val, sizeof(uint32_t), 1); + // write payload layout version + uint32_t version = header.version; + io_writer->write(&version, sizeof(uint32_t), 1); // write dimension - auto dimension = index->get_dimension(); + int dimension = header.dimension; io_writer->write(&dimension, sizeof(int), 1); } -Index* read_header(IOReader* io_reader) { - uint32_t id_val = 0; - io_reader->read(&id_val, sizeof(uint32_t), 1); - int dimension = 0; - io_reader->read(&dimension, sizeof(int), 1); - switch (id_val) { +IndexHeader read_header(IOReader* io_reader) { + IndexHeader header; + io_reader->read(&header.id, sizeof(uint32_t), 1); + io_reader->read(&header.version, sizeof(uint32_t), 1); + io_reader->read(&header.dimension, sizeof(int), 1); + return header; +} + +// Constructs the index the id names, still empty: the payload is what +// read_index/mmap_index fill in. +Index* make_index(const IndexHeader& header) { + switch (header.id) { case BRUT: - return new BrutalIndex(dimension); + return new BrutalIndex(header.dimension); case SEIS: - return new SeismicIndex(dimension); + return new SeismicIndex(header.dimension); case SESQ: - return new SeismicScalarQuantizedIndex(dimension); + return new SeismicScalarQuantizedIndex(header.dimension); case DSEI: - return new DiskSeismicIndex(dimension); + return new DiskSeismicIndex(header.dimension); case IDMP: return new IDMapIndex(); case INVT: - return new InvertedIndex(dimension); + return new InvertedIndex(header.dimension); default: throw std::runtime_error("Unknown index type"); } } + +// A version outside 1..supported is one this build cannot lay out: either it +// postdates this binary, or no writer ever produced it. Reading the payload +// anyway would consume whatever the fields happen to align with, so the file is +// rejected here instead. +void throw_if_version_unsupported(const IndexHeader& header, + uint32_t supported) { + if (header.version == 0 || header.version > supported) { + throw std::runtime_error("Unsupported " + id_to_string(header.id) + + " index format version " + + std::to_string(header.version) + + "; this build reads versions 1 through " + + std::to_string(supported)); + } +} } // namespace namespace detail { @@ -132,7 +171,10 @@ void write_index(Index* index, IOWriter* io_writer, bool keep_open) { throw std::runtime_error("Index does not support serialization"); } // write header - write_header(index, io_writer); + write_header({.id = fourcc(index->id()), + .version = index_io->format_version(), + .dimension = index->get_dimension()}, + io_writer); // write index customized payload index_io->write_index(io_writer); closer.close(); @@ -140,12 +182,16 @@ void write_index(Index* index, IOWriter* io_writer, bool keep_open) { Index* read_index(IOReader* io_reader, bool keep_open, int io_flags) { StreamCloser closer(io_reader, keep_open); + const IndexHeader header = read_header(io_reader); // Held so it does not leak if anything below throws, close() included. - std::unique_ptr index(read_header(io_reader)); + std::unique_ptr index(make_index(header)); auto* index_io = dynamic_cast(index.get()); if (index_io == nullptr) { throw std::runtime_error("Index does not support serialization"); } + // Ahead of either read below, so a payload this build cannot lay out is + // never parsed. + throw_if_version_unsupported(header, index_io->format_version()); // handle mmap if ((io_flags & IndexIoFlag::kUseMmap) == IndexIoFlag::kUseMmap) { @@ -155,8 +201,7 @@ Index* read_index(IOReader* io_reader, bool keep_open, int io_flags) { // serialize() padded against. An index type without a mapped reader // returns null and falls through to the copying read below. std::unique_ptr mapped(mmap_index_payload( - fourcc(index->id()), index->get_dimension(), - file_io_reader->file_name().c_str(), io_reader->pos())); + header, file_io_reader->file_name().c_str(), io_reader->pos())); if (mapped != nullptr) { index.reset(); closer.close(); @@ -165,7 +210,7 @@ Index* read_index(IOReader* io_reader, bool keep_open, int io_flags) { } } - index_io->read_index(io_reader, io_flags); + index_io->read_index(io_reader, header, io_flags); closer.close(); return index.release(); } diff --git a/nsparse/io/io.h b/nsparse/io/io.h index 3ea6a12..19d9a9f 100644 --- a/nsparse/io/io.h +++ b/nsparse/io/io.h @@ -49,12 +49,46 @@ class Serializable { virtual void deserialize(IOReader* reader) = 0; }; +// The fixed-size header written ahead of every index payload, a nested +// delegate's included: fourcc id, format version, dimension. The version comes +// second so that a later change to the rest of the header stays reachable -- id +// and version parse the same way at every version. +// +// Passed around whole rather than as a loose version: the two are adjacent +// integers at every call site that needs them, and a struct is not silently +// swappable with an io_flags or a dimension. +struct IndexHeader { + uint32_t id = 0; + // Numbered per index type, not per file: each type versions its own + // payload, so changing one leaves the others' numbering untouched and a + // nested delegate carries its own. See IndexIO::format_version. + uint32_t version = 0; + int dimension = 0; +}; + +// Where a payload starts, relative to the header before it. The fields are +// written one at a time with no padding between them. +constexpr size_t kIndexHeaderSize = + sizeof(uint32_t) + sizeof(uint32_t) + sizeof(int); class IndexIO { public: virtual ~IndexIO() = default; + + // Layout revision this index type writes. Recorded in the header above and + // handed back to read_index; bumped only when *this* type's payload + // changes. + // + // Pure rather than defaulted: an inherited version would let a payload + // change ship without one, which is the failure the header exists to catch. + [[nodiscard]] virtual uint32_t format_version() const = 0; + virtual void write_index(IOWriter* io_writer) {}; - virtual void read_index(IOReader* io_reader, int io_flags = 0) {}; + + // `header` is what the file declared, with its version already checked + // against format_version(): it is in 1..format_version(), never newer. + virtual void read_index(IOReader* io_reader, const IndexHeader& header, + int io_flags = 0){}; }; constexpr uint32_t fourcc(const std::array& id) { diff --git a/nsparse/seismic_index.cpp b/nsparse/seismic_index.cpp index 6e881c1..489c886 100644 --- a/nsparse/seismic_index.cpp +++ b/nsparse/seismic_index.cpp @@ -284,7 +284,8 @@ void SeismicIndex::write_index(IOWriter* io_writer) { inv_list_writer.serialize(io_writer); } -void SeismicIndex::read_index(IOReader* io_reader, int io_flags) { +void SeismicIndex::read_index(IOReader* io_reader, const IndexHeader& header, + int io_flags) { // read vectors SparseVectors tmp_vectors; tmp_vectors.deserialize(io_reader); @@ -297,10 +298,10 @@ void SeismicIndex::read_index(IOReader* io_reader, int io_flags) { clustered_inverted_lists = std::move(inv_list_writer.release()); } -SeismicIndex* SeismicIndex::mmap_index(int dimension, const char* index_file, - size_t pos) { +SeismicIndex* SeismicIndex::mmap_index(const IndexHeader& header, + const char* index_file, size_t pos) { throw_if_null(index_file, "index_file must not be null"); - auto index = std::make_unique(dimension); + auto index = std::make_unique(header.dimension); MmapFile mmap_file(std::string{index_file}); // `pos` is where write_index's payload begins, past the header read_header diff --git a/nsparse/seismic_index.h b/nsparse/seismic_index.h index e65a8bd..d1d0a52 100644 --- a/nsparse/seismic_index.h +++ b/nsparse/seismic_index.h @@ -10,13 +10,14 @@ #ifndef SEISMIC_INDEX_H #define SEISMIC_INDEX_H #include +#include #include #include #include "absl/container/flat_hash_set.h" #include "nsparse/cluster/inverted_list_clusters.h" -#include "nsparse/mmap_index.h" #include "nsparse/io/io.h" +#include "nsparse/mmap_index.h" #include "nsparse/seismic_common.h" #include "nsparse/sparse_vectors.h" #include "nsparse/types.h" @@ -34,6 +35,8 @@ struct SeismicSearchParameters : public SearchParameters { class SeismicIndex : public MmapIndex, public IndexIO { public: static constexpr std::array name = {'S', 'E', 'I', 'S'}; + // Bump whenever write_index's payload layout changes. + static constexpr uint32_t kFormatVersion = 1; explicit SeismicIndex(int dim); SeismicIndex(int dim, SeismicClusterParameters parameter); @@ -47,15 +50,21 @@ class SeismicIndex : public MmapIndex, public IndexIO { void add(idx_t n, const idx_t* indptr, const term_t* indices, const float* values) override; - - static SeismicIndex* mmap_index(int dimension, const char * index_file, size_t pos); + + static SeismicIndex* mmap_index(const IndexHeader& header, + const char* index_file, size_t pos); + protected: std::vector clustered_inverted_lists; private: // override of IndexIO + [[nodiscard]] uint32_t format_version() const override { + return kFormatVersion; + } void write_index(IOWriter* io_writer) override; - void read_index(IOReader* io_reader, int io_flags = 0) override; + void read_index(IOReader* io_reader, const IndexHeader& header, + int io_flags = 0) override; auto search(idx_t n, const idx_t* indptr, const term_t* indices, const float* values, int k, diff --git a/nsparse/seismic_scalar_quantized_index.cpp b/nsparse/seismic_scalar_quantized_index.cpp index 01b53c1..f7313c5 100644 --- a/nsparse/seismic_scalar_quantized_index.cpp +++ b/nsparse/seismic_scalar_quantized_index.cpp @@ -357,7 +357,7 @@ auto SeismicScalarQuantizedIndex::single_query( } void SeismicScalarQuantizedIndex::write_index(IOWriter* io_writer) { - write_header(io_writer); + write_quantizer_header(io_writer); // write vectors if (vectors_ == nullptr) { empty_sparse_vectors.serialize(io_writer); @@ -368,8 +368,10 @@ void SeismicScalarQuantizedIndex::write_index(IOWriter* io_writer) { inv_list_writer.serialize(io_writer); } -void SeismicScalarQuantizedIndex::read_index(IOReader* io_reader, int io_flags) { - read_header(io_reader); +void SeismicScalarQuantizedIndex::read_index(IOReader* io_reader, + const IndexHeader& header, + int io_flags) { + read_quantizer_header(io_reader); SparseVectors tmp_vectors; tmp_vectors.deserialize(io_reader); throw_if_element_size_mismatch(tmp_vectors, sq_); @@ -382,9 +384,10 @@ void SeismicScalarQuantizedIndex::read_index(IOReader* io_reader, int io_flags) } SeismicScalarQuantizedIndex* SeismicScalarQuantizedIndex::mmap_index( - int dimension, const char* index_file, size_t pos) { + const IndexHeader& header, const char* index_file, size_t pos) { throw_if_null(index_file, "index_file must not be null"); - auto index = std::make_unique(dimension); + auto index = + std::make_unique(header.dimension); MmapFile mmap_file(std::string{index_file}); // `pos` is where write_index's payload begins, past the header read_header @@ -393,7 +396,7 @@ SeismicScalarQuantizedIndex* SeismicScalarQuantizedIndex::mmap_index( MmapCursor cursor(mmap_file.data(), mmap_file.size()); cursor.skip(pos); - // Same order write_index wrote them, starting with what write_header wrote. + // Same order write_index wrote them, starting with the quantizer header. const auto sq_type = cursor.read_scalar(); const auto vmin = cursor.read_scalar(); const auto vmax = cursor.read_scalar(); @@ -418,7 +421,7 @@ SeismicScalarQuantizedIndex* SeismicScalarQuantizedIndex::mmap_index( return index.release(); } -void SeismicScalarQuantizedIndex::write_header(IOWriter* io_writer) { +void SeismicScalarQuantizedIndex::write_quantizer_header(IOWriter* io_writer) { auto sq_type = sq_.get_quantizer_type(); io_writer->write(&sq_type, sizeof(QuantizerType), 1); auto vmin = sq_.get_min(); @@ -427,7 +430,7 @@ void SeismicScalarQuantizedIndex::write_header(IOWriter* io_writer) { io_writer->write(&vmax, sizeof(float), 1); } -void SeismicScalarQuantizedIndex::read_header(IOReader* io_reader) { +void SeismicScalarQuantizedIndex::read_quantizer_header(IOReader* io_reader) { QuantizerType sq_type = QuantizerType::QT_8bit; float vmin = 0.0F; float vmax = 1.0F; diff --git a/nsparse/seismic_scalar_quantized_index.h b/nsparse/seismic_scalar_quantized_index.h index 02d87b1..6415541 100644 --- a/nsparse/seismic_scalar_quantized_index.h +++ b/nsparse/seismic_scalar_quantized_index.h @@ -11,12 +11,14 @@ #define SEISMIC_SCALAR_QUANTIZED_INDEX_H #include +#include #include #include #include "absl/container/flat_hash_set.h" #include "nsparse/cluster/inverted_list_clusters.h" #include "nsparse/index.h" +#include "nsparse/io/io.h" #include "nsparse/mmap_index.h" #include "nsparse/seismic_index.h" #include "nsparse/types.h" @@ -35,6 +37,9 @@ struct SeismicSQSearchParameters : public SeismicSearchParameters { class SeismicScalarQuantizedIndex : public MmapIndex, public IndexIO { public: static constexpr std::array name = {'S', 'E', 'S', 'Q'}; + // Bump whenever write_index's payload layout changes. + static constexpr uint32_t kFormatVersion = 1; + explicit SeismicScalarQuantizedIndex(int dim); SeismicScalarQuantizedIndex(QuantizerType quantizer_type, float vmin, float vmax, SeismicClusterParameters parameter, @@ -54,7 +59,7 @@ class SeismicScalarQuantizedIndex : public MmapIndex, public IndexIO { // Borrows a serialized index from a file mapping instead of copying it onto // the heap; see SeismicIndex::mmap_index, which this mirrors past the // quantizer header. `pos` is where write_index's payload begins. - static SeismicScalarQuantizedIndex* mmap_index(int dimension, + static SeismicScalarQuantizedIndex* mmap_index(const IndexHeader& header, const char* index_file, size_t pos); @@ -66,10 +71,16 @@ class SeismicScalarQuantizedIndex : public MmapIndex, public IndexIO { private: // interfaces of IndexIO + [[nodiscard]] uint32_t format_version() const override { + return kFormatVersion; + } void write_index(IOWriter* io_writer) override; - void read_index(IOReader* io_reader, int io_flags = 0) override; - void write_header(IOWriter* io_writer); - void read_header(IOReader* io_reader); + void read_index(IOReader* io_reader, const IndexHeader& header, + int io_flags = 0) override; + // The quantizer parameters that open this index's payload -- distinct from + // the IndexHeader the file itself starts with. + void write_quantizer_header(IOWriter* io_writer); + void read_quantizer_header(IOReader* io_reader); // Null `search_parameters` searches with the defaults, as it does for // SeismicIndex and as the base signature's default argument implies. This diff --git a/tests/index_io_test.cpp b/tests/index_io_test.cpp index e889556..e7068a5 100644 --- a/tests/index_io_test.cpp +++ b/tests/index_io_test.cpp @@ -15,11 +15,13 @@ #include #include #include +#include #include #include #include #include "nsparse/brutal_index.h" +#include "nsparse/disk_seismic_index.h" #include "nsparse/id_map_index.h" #include "nsparse/index.h" #include "nsparse/inverted_index.h" @@ -83,10 +85,12 @@ class StrictBufferedIOReader : public nsparse::IOReader { class MockIndex : public nsparse::Index, public nsparse::IndexIO { public: static constexpr std::array name = {'M', 'O', 'C', 'K'}; + static constexpr uint32_t kFormatVersion = 1; explicit MockIndex(int dim = 0) : Index(dim) {} std::array id() const override { return name; } + uint32_t format_version() const override { return kFormatVersion; } void add(nsparse::idx_t /*n*/, const nsparse::idx_t* /*indptr*/, const nsparse::term_t* /*indices*/, @@ -104,7 +108,9 @@ class MockIndex : public nsparse::Index, public nsparse::IndexIO { io_writer->write(test_string_.data(), sizeof(char), size); } - void read_index(nsparse::IOReader* io_reader, int /*io_flags*/) override { + void read_index(nsparse::IOReader* io_reader, + const nsparse::IndexHeader& /*header*/, + int /*io_flags*/) override { io_reader->read(&test_data_, sizeof(int), 1); size_t size = 0; io_reader->read(&size, sizeof(size_t), 1); @@ -143,10 +149,19 @@ TEST(IndexIO, WriteIndexBasic) { std::memcpy(&written_fourcc, data.data(), sizeof(uint32_t)); ASSERT_EQ(written_fourcc, nsparse::fourcc(MockIndex::name)); - // Verify dimension is written after fourcc + // Verify the format version is written after the fourcc + uint32_t written_version = 0; + std::memcpy(&written_version, data.data() + sizeof(uint32_t), + sizeof(uint32_t)); + ASSERT_EQ(written_version, MockIndex::kFormatVersion); + + // Verify dimension is written after the version int written_dim = 0; - std::memcpy(&written_dim, data.data() + sizeof(uint32_t), sizeof(int)); + std::memcpy(&written_dim, data.data() + 2 * sizeof(uint32_t), sizeof(int)); ASSERT_EQ(written_dim, 128); + + // The payload starts right after those three fields. + ASSERT_EQ(nsparse::kIndexHeaderSize, 2 * sizeof(uint32_t) + sizeof(int)); } // Test write_index throws for non-IndexIO index @@ -172,11 +187,13 @@ TEST(IndexIO, WriteIndexThrowsForNonIndexIO) { // Test read_index throws for unknown index type TEST(IndexIO, ReadIndexThrowsForUnknownType) { // Create a buffer with an unknown fourcc - std::vector buffer(sizeof(uint32_t) + sizeof(int)); + std::vector buffer(nsparse::kIndexHeaderSize); uint32_t unknown_fourcc = 0xDEADBEEF; + uint32_t version = 1; int dimension = 64; std::memcpy(buffer.data(), &unknown_fourcc, sizeof(uint32_t)); - std::memcpy(buffer.data() + sizeof(uint32_t), &dimension, sizeof(int)); + std::memcpy(buffer.data() + sizeof(uint32_t), &version, sizeof(uint32_t)); + std::memcpy(buffer.data() + 2 * sizeof(uint32_t), &dimension, sizeof(int)); nsparse::BufferedIOReader reader(buffer); ASSERT_THROW(nsparse::read_index(&reader), std::runtime_error); @@ -494,10 +511,11 @@ TEST(IndexIO, RoundtripInvertedIndexCountsATrailingEmptyDocument) { EXPECT_EQ(loaded->num_vectors(), 2); } -// The INVT payload has no version to check, so a file written before it carried -// the document count reads with every field one slot early. The element width -// is what gives that away: write_index only ever writes U32, and the value -// landing there instead is the first posting list's size. +// Defence behind the header version, for a file whose version does not admit it +// is stale: dropping the document count leaves every later field one slot +// early. The element width is what gives that away -- write_index only ever +// writes U32, and the value landing there instead is the first posting list's +// size. TEST(IndexIO, ReadIndexRejectsAnInvertedIndexFileWithoutTheDocumentCount) { nsparse::InvertedIndex original(128); @@ -510,12 +528,11 @@ TEST(IndexIO, ReadIndexRejectsAnInvertedIndexFileWithoutTheDocumentCount) { nsparse::BufferedIOWriter writer; nsparse::write_index(&original, &writer); - // The header is a fourcc and a dimension; the count is the payload's first - // field, so dropping it is exactly the old layout. + // The count is the payload's first field, so dropping it is exactly the old + // layout. std::vector stale = writer.data(); - const size_t header = sizeof(uint32_t) + sizeof(int); - stale.erase(stale.begin() + header, - stale.begin() + header + sizeof(size_t)); + stale.erase(stale.begin() + nsparse::kIndexHeaderSize, + stale.begin() + nsparse::kIndexHeaderSize + sizeof(size_t)); nsparse::BufferedIOReader reader(stale); EXPECT_THROW(nsparse::read_index(&reader), std::runtime_error); @@ -704,3 +721,238 @@ TEST(IndexIO, UseMmapFlagReachesTheIDMapQuantizedDelegate) { distances.data(), labels.data(), ¶ms); EXPECT_EQ(labels[0], 100); } + +namespace { + +// Header fields are written one at a time with no padding, so the version sits +// one uint32_t past the start of the header it belongs to. +constexpr size_t version_offset(size_t header_offset) { + return header_offset + sizeof(uint32_t); +} + +uint32_t read_u32(const std::vector& data, size_t offset) { + uint32_t value = 0; + std::memcpy(&value, data.data() + offset, sizeof(uint32_t)); + return value; +} + +void patch_u32(std::vector& data, size_t offset, uint32_t value) { + ASSERT_LE(offset + sizeof(uint32_t), data.size()); + std::memcpy(data.data() + offset, &value, sizeof(uint32_t)); +} + +void patch_file_u32(const char* path, size_t offset, uint32_t value) { + std::fstream out(path, std::ios::binary | std::ios::in | std::ios::out); + ASSERT_TRUE(out.is_open()) << path; + out.seekp(static_cast(offset)); + out.write(reinterpret_cast(&value), sizeof(uint32_t)); + ASSERT_TRUE(out.good()); +} + +// A serialized index of each type that has a copying read. DiskSeismicIndex is +// left out: its read_index throws whatever the version says, so it cannot show +// what the version check alone rejects. It is covered over the mapped path +// below. +template +std::vector serialized(Index* index) { + nsparse::BufferedIOWriter writer; + nsparse::write_index(index, &writer); + return writer.data(); +} + +} // namespace + +// Every index type stamps its own current version into the header, and the +// header lands where kIndexHeaderSize says the payload begins. +TEST(IndexIOVersion, EachIndexTypeWritesItsCurrentFormatVersion) { + { + nsparse::SeismicIndex index(32); + const auto data = serialized(&index); + EXPECT_EQ(read_u32(data, 0), + nsparse::fourcc(nsparse::SeismicIndex::name)); + EXPECT_EQ(read_u32(data, version_offset(0)), + nsparse::SeismicIndex::kFormatVersion); + } + { + nsparse::SeismicScalarQuantizedIndex index(32); + const auto data = serialized(&index); + EXPECT_EQ(read_u32(data, version_offset(0)), + nsparse::SeismicScalarQuantizedIndex::kFormatVersion); + } + { + nsparse::InvertedIndex index(32); + const auto data = serialized(&index); + EXPECT_EQ(read_u32(data, version_offset(0)), + nsparse::InvertedIndex::kFormatVersion); + } + { + nsparse::IDMapIndex index(new nsparse::SeismicIndex(32)); + const auto data = serialized(&index); + EXPECT_EQ(read_u32(data, version_offset(0)), + nsparse::IDMapIndex::kFormatVersion); + } +} + +// A file from a build that postdates this one cannot be laid out here. Reading +// the payload anyway would consume whatever the fields happen to align with, so +// the version is what rejects it. +TEST(IndexIOVersion, ReadIndexRejectsAVersionFromTheFuture) { + nsparse::SeismicIndex original(32); + auto data = serialized(&original); + patch_u32(data, version_offset(0), + nsparse::SeismicIndex::kFormatVersion + 1); + + nsparse::BufferedIOReader reader(data); + try { + std::unique_ptr loaded(nsparse::read_index(&reader)); + ADD_FAILURE() << "accepted a version this build cannot read"; + } catch (const std::runtime_error& error) { + // The message is checked, not just the type: a payload read past a + // dropped guard throws too, somewhere downstream, and that must not + // read as a pass. + const std::string what = error.what(); + EXPECT_NE(what.find("SEIS"), std::string::npos) << what; + EXPECT_NE(what.find("format version"), std::string::npos) << what; + } +} + +// Zero is not a version any writer produces, so a file declaring it is corrupt +// rather than merely old. +TEST(IndexIOVersion, ReadIndexRejectsAZeroVersion) { + nsparse::InvertedIndex original(32); + original.build(); + auto data = serialized(&original); + patch_u32(data, version_offset(0), 0); + + nsparse::BufferedIOReader reader(data); + try { + std::unique_ptr loaded(nsparse::read_index(&reader)); + ADD_FAILURE() << "accepted a zero version"; + } catch (const std::runtime_error& error) { + const std::string what = error.what(); + EXPECT_NE(what.find("INVT"), std::string::npos) << what; + EXPECT_NE(what.find("format version 0"), std::string::npos) << what; + } +} + +// The version this build does write still reads, which is what keeps the check +// from rejecting every file. +TEST(IndexIOVersion, ReadIndexAcceptsTheVersionItWrites) { + nsparse::SeismicIndex original(32); + const auto data = serialized(&original); + ASSERT_EQ(read_u32(data, version_offset(0)), + nsparse::SeismicIndex::kFormatVersion); + + nsparse::BufferedIOReader reader(data); + std::unique_ptr loaded(nsparse::read_index(&reader)); + ASSERT_NE(loaded, nullptr); + EXPECT_EQ(loaded->get_dimension(), 32); +} + +// Versions are numbered per index type, so a nested delegate carries its own +// header rather than inheriting the wrapper's. Bumping IDMP must not implicate +// SEIS, and vice versa. +TEST(IndexIOVersion, ANestedDelegateCarriesItsOwnVersion) { + nsparse::IDMapIndex original(new nsparse::SeismicIndex(128)); + std::vector indptr = {0, 2, 4}; + std::vector indices = {0, 1, 2, 3}; + std::vector values = {1.0F, 0.5F, 0.8F, 0.3F}; + std::vector ids = {100, 200}; + original.add_with_ids(2, indptr.data(), indices.data(), values.data(), + ids.data()); + + auto data = serialized(&original); + + // IDMP's payload is the map size and the map itself; the delegate's header + // follows it. + const size_t map_size = ids.size(); + const size_t delegate_header = nsparse::kIndexHeaderSize + sizeof(size_t) + + map_size * sizeof(nsparse::idx_t); + ASSERT_EQ(read_u32(data, delegate_header), + nsparse::fourcc(nsparse::SeismicIndex::name)); + ASSERT_EQ(read_u32(data, version_offset(delegate_header)), + nsparse::SeismicIndex::kFormatVersion); + + // Reached only because IDMapIndex::read_index delegates through + // detail::read_index, which re-reads a header of its own. + patch_u32(data, version_offset(delegate_header), + nsparse::SeismicIndex::kFormatVersion + 1); + nsparse::BufferedIOReader reader(data); + try { + std::unique_ptr loaded(nsparse::read_index(&reader)); + ADD_FAILURE() << "accepted a future version on the delegate"; + } catch (const std::runtime_error& error) { + const std::string what = error.what(); + EXPECT_NE(what.find("SEIS"), std::string::npos) << what; + } +} + +// The check sits ahead of the mapped read as well as the copying one: mapping a +// payload this build cannot lay out would borrow arrays at the wrong offsets, +// which is worse than a copy that merely reads garbage. +TEST(IndexIOVersion, MappedReadAlsoRejectsAVersionFromTheFuture) { + TempIndexFile file("nsparse_index_io_future_version_mmap.idx"); + nsparse::InvertedIndex original(128); + std::vector indptr = {0, 2, 4}; + std::vector indices = {0, 1, 2, 3}; + std::vector values = {1.0F, 0.5F, 0.8F, 0.3F}; + original.add(2, indptr.data(), indices.data(), values.data()); + original.build(); + nsparse::write_index(&original, file.c_str()); + + // Sound before the patch, so the throw below is the version's doing. + { + std::unique_ptr loaded( + nsparse::read_index(file.c_str(), nsparse::IndexIoFlag::kUseMmap)); + ASSERT_NE(loaded, nullptr); + } + + patch_file_u32(file.c_str(), version_offset(0), + nsparse::InvertedIndex::kFormatVersion + 1); + + for (const int flags : + {0, static_cast(nsparse::IndexIoFlag::kUseMmap)}) { + try { + std::unique_ptr loaded( + nsparse::read_index(file.c_str(), flags)); + ADD_FAILURE() << "accepted the file, flags " << flags; + } catch (const std::runtime_error& error) { + const std::string what = error.what(); + EXPECT_NE(what.find("format version"), std::string::npos) + << "flags " << flags << ": " << what; + } + } +} + +// DiskSeismicIndex has no copying read, so the mapped path is the only place +// its version can be checked -- and the check has to come first, before +// read_index would throw its own mmap-only error. +TEST(IndexIOVersion, MappedReadRejectsAFutureDiskSeismicVersion) { + TempIndexFile file("nsparse_index_io_dsei_future_version.idx"); + nsparse::DiskSeismicIndex original( + 5, {.lambda = 10, .beta = 2, .alpha = 0.5F}); + std::vector indptr = {0, 2, 4}; + std::vector indices = {0, 1, 2, 3}; + std::vector values = {1.0F, 0.5F, 0.8F, 0.3F}; + original.add(2, indptr.data(), indices.data(), values.data()); + original.build(); + nsparse::write_index(&original, file.c_str()); + + ASSERT_NO_THROW({ + std::unique_ptr loaded( + nsparse::read_index(file.c_str(), nsparse::IndexIoFlag::kUseMmap)); + ASSERT_NE(loaded, nullptr); + }); + + patch_file_u32(file.c_str(), version_offset(0), + nsparse::DiskSeismicIndex::kFormatVersion + 1); + try { + std::unique_ptr loaded( + nsparse::read_index(file.c_str(), nsparse::IndexIoFlag::kUseMmap)); + ADD_FAILURE() << "accepted a future version"; + } catch (const std::runtime_error& error) { + const std::string what = error.what(); + EXPECT_NE(what.find("DSEI"), std::string::npos) << what; + EXPECT_NE(what.find("format version"), std::string::npos) << what; + } +} diff --git a/tests/inverted_index_test.cpp b/tests/inverted_index_test.cpp index cfb4789..40acdd9 100644 --- a/tests/inverted_index_test.cpp +++ b/tests/inverted_index_test.cpp @@ -776,11 +776,9 @@ pair_of_score_id_vector_t search_one(Index* index, return {distances, labels}; } -// The stored element width, counting past the fourcc and dimension read_index -// consumes, and past the document count and term count write_index puts ahead -// of it. -constexpr size_t kElementSizeOffset = - sizeof(uint32_t) + sizeof(int) + (2 * sizeof(size_t)); +// The stored element width, counting past the header read_index consumes, and +// past the document count and term count write_index puts ahead of it. +constexpr size_t kElementSizeOffset = kIndexHeaderSize + (2 * sizeof(size_t)); template T read_field(const std::string& path, size_t offset) { diff --git a/tests/seismic_index_test.cpp b/tests/seismic_index_test.cpp index cd06cf3..d09168c 100644 --- a/tests/seismic_index_test.cpp +++ b/tests/seismic_index_test.cpp @@ -918,11 +918,9 @@ std::vector search_top(Index* index, term_t term, int k) { return labels; } -// SparseVectors' element_size, counting past the fourcc and dimension -// read_index consumes and past the vector count and dimension serialize() -// wrote ahead of it. -constexpr size_t kElementSizeOffset = - sizeof(uint32_t) + sizeof(int) + (2 * sizeof(size_t)); +// SparseVectors' element_size, counting past the header read_index consumes and +// past the vector count and dimension serialize() wrote ahead of it. +constexpr size_t kElementSizeOffset = kIndexHeaderSize + (2 * sizeof(size_t)); template T read_field(const std::string& path, size_t offset) { diff --git a/tests/seismic_scalar_quantized_index_test.cpp b/tests/seismic_scalar_quantized_index_test.cpp index f1ba67c..9570f7e 100644 --- a/tests/seismic_scalar_quantized_index_test.cpp +++ b/tests/seismic_scalar_quantized_index_test.cpp @@ -997,9 +997,9 @@ class ScopedMmapAdvise { }; #endif -// Where read_index leaves off before the payload: the fourcc and the dimension. -// SESQ's payload opens with the quantizer header write_header wrote. -constexpr size_t kQuantizerTypeOffset = sizeof(uint32_t) + sizeof(int); +// Where read_index leaves off before the payload. SESQ's payload opens with the +// quantizer header write_quantizer_header wrote. +constexpr size_t kQuantizerTypeOffset = kIndexHeaderSize; constexpr size_t kVminOffset = kQuantizerTypeOffset + sizeof(QuantizerType); template