Repository navigation
Conversation
Adds svs_stream_interface_ops / svs_stream_interface, the svs_stream_ops_t / svs_stream_t / svs_stream_i typedef triple and SVS_INIT_STREAM_OPS to the public header, following the shape of the existing threadpool, allocator and id_filter interfaces. No seek callback: index load is strictly sequential and save needs only a position query, which the adapter answers from its own byte counter. The adapter in src/stream.hpp bridges the vtable to std::istream/std::ostream through a 64 KiB buffered std::streambuf. Two details are load-bearing: - tellp() reports bytes handed to the streambuf rather than bytes flushed. StreamArchiver::write_table derives cache-line padding from it, so a stale count misaligns the payload silently and would defeat a future mmap load. - Both stream wrappers set exceptions(badbit). Without it the iostream sentry converts a callback exception into a silent badbit, and a save would report success after the write callback had reported failure. validate() follows allocator.hpp, the only existing interface that honours the version and struct_size fields it declares. A null stream is an error rather than the no-op leniency IDFilterAdapter applies to a null filter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Appends a virtual save(std::ostream&) to c_runtime::Index and gives IndexBuilder load_stream and load_stream_dynamic entry points, with matching dispatcher targets registered through the existing per-file Dispatcher rather than a new one. DynamicIndex inherits the new virtual and needs no declaration of its own. The virtual is appended to the end of Index, never inserted: inserting one mid-vtable was shipped and reverted once already (9980bd2). load_stream takes std::unique_ptr<std::istream>&& rather than std::istream&. Ordinary load copies data out of the stream, but a future zero-copy load would hand out pointers into the stream's own buffer and must own it. Taking the unique_ptr now keeps that from becoming a signature change through every dispatcher later. This loosens coupling: the internal load path is no longer filesystem-specific. Known gap, a property of the core rather than of this layer: the stream overloads of Vamana::assemble and DynamicVamana::assemble build the graph from GraphLoader<>::return_type and accept no graph loader, so a custom graph allocator is not honoured on the stream path and a stream-loaded dynamic index gets a non-blocked graph. blocksize_bytes still reaches the data allocator. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds coded_error, holding an svs_error_code_t alongside its message, and one catch clause that reports the carried code instead of collapsing it to SVS_ERROR_RUNTIME. A callback reporting SVS_ERROR_OUT_OF_MEMORY can now surface that code rather than leaving it readable only as text inside the message. The clause must precede the std::runtime_error clause it derives from, or it becomes unreachable; the comment records that. No existing error path changes behaviour, and the threadpool path keeps its lossy reporting for now. Nothing throws coded_error yet: wiring the stream adapter's throw sites to it belongs with the public streaming functions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add svs_index_save_stream, svs_index_load_stream and svs_index_load_stream_dynamic, delegating to the stream plumbing through the streambuf adapter. Purely additive; the directory-based functions are unchanged. Switch the two throw sites in src/stream.hpp from std::runtime_error to coded_error so a callback's svs_error_code_t survives to the public API instead of collapsing to SVS_ERROR_RUNTIME. Without this the error.hpp catch clause added earlier is dead code. Carry the Vamana-only NOT_IMPLEMENTED_IF guards on the new loaders, matching the directory-based wrappers; the IndexBuilder layer does not enforce this. svs_index_save_stream flushes explicitly after saving. The core never flushes and ~StreamBuf swallows the exception from its final write, so a write callback failing on the last partial buffer previously reported success over a truncated stream. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round-trips a static and a dynamic index through an in-memory stream, then exercises the interface validation paths and the callback error conventions. Two cases target defects found while implementing the feature rather than the happy path. A write callback that rejects only a sub-buffer-sized write catches the final partial flush, which the streambuf destructor used to swallow into a successful save over a truncated stream; it is guarded by a precondition that the payload does not land on a 64 KiB boundary, so it cannot pass vacuously. A read callback returning zero with an error set confirms the caller's error code survives to the public API instead of collapsing to SVS_ERROR_RUNTIME. Note that `ctest -R c_api` matches nothing: the filter is a regex over test names, which start with "C API". Use `-L c_api`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a Stream Interface section to C_API_Design.md covering the callback contracts, the serial-invocation guarantee, the lifetime window and the encoding asymmetry, plus a runnable example that saves and loads an index through a caller-supplied in-memory stream. Two contracts were reachable only by reading the implementation. A read callback reports failure by returning zero with an error set, since the return value alone cannot distinguish that from end of stream; a caller who guessed otherwise would get a silently truncated index loaded as a success. And a supplied allocator does not reach graph memory on the stream path, which also costs a stream-loaded dynamic index its blocked growth. Both are now stated at the declarations, the allocator gap because the core fix is deferred and the header is the only place a caller will see it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lback `flush_write_buffer()` threw before resetting the put area, so the buffer still held the unwritten bytes. The explicit `os.flush()` in `svs_index_save_stream` rethrew, unwinding destroyed the stream, and `~StreamBuf`'s final flush handed the identical bytes to the caller's write callback a second time and then swallowed the failure. Resetting the put area before invoking the callback leaves it empty on a throw, so the destructor's flush is a no-op. Three smaller review findings, same file: - `StreamBuf` declared a destructor but no copy or move members. Since `std::streambuf`'s copy constructor is protected rather than deleted, an implicit copy constructor existed that deep-copied the buffers while shallow-copying the base's get and put pointers, so a copy's areas aliased the source's storage. Dormant, because the only holder is a private base of the non-copyable stream wrappers, but all four are now deleted. - Buffer sizing was keyed on which callbacks were non-null, so a caller supplying both directions allocated 128 KiB with half of it dead. The adapter now takes an explicit direction and allocates only what it uses. - `overflow()` returned `eof()` for the eof sentinel, which the streambuf contract reads as failure; it now returns `traits_type::not_eof(ch)`. Latent only, as no path here passes the sentinel. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The stream loaders validated the algorithm type before the stream pointer, so a non-Vamana builder passed together with a NULL stream reported `SVS_ERROR_NOT_IMPLEMENTED` where the directory loaders report `SVS_ERROR_INVALID_ARGUMENT`. `EXPECT_ARG_NOT_NULL(stream)` now precedes `NOT_IMPLEMENTED_IF` in all three streaming functions, matching the ordering the directory-based loaders already use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every test built 100x32 vectors, roughly 25-30 KB, so the adapter's 64 KiB buffer never filled and `StreamBuf::overflow()` had no coverage at all. The consequence was that the test guarding the unflushed-save regression passed trivially: it proved that the only flush fails, not that the final partial flush fails after several full ones. Both round-trips now use a payload spanning more than two full buffers and assert on the measured byte count, and the partial-failure case additionally requires that at least two full-size writes succeeded first. Also: a regression test for the redelivery fix, asserting the write callback is never invoked again once it has returned false; element-by-element result comparison plus a retrievability check for an added point in the dynamic round-trip, which previously asserted only the query count; and dedicated cases for an empty stream and for one truncated partway through a valid payload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`C_API_Design.md` is the single source of truth for the C API, so the decision behind the save/load asymmetry belongs there rather than in a scratch document: load accepts both the native encoding and a directory archive because SVS detects which it has, while save produces only the native one. The consequence worth stating is that a directory-saved index cannot be streamed, so streaming is self-sufficient only for indexes that were themselves stream-saved. Also fixes the Samples lists in both files, which pointed at a `samples/` directory that does not exist; the examples live under `examples/c/`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both comments kept their substance: the stream alternative is matched through the generic variant `DispatchConverter`, the same way the existing build and directory-load alternatives are. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ahuber21
requested review from
ibhati and
rfsaliev
and removed request for
ibhati
September 29, 2026 18:24
rfsaliev
requested changes
Oct 1, 2026
rfsaliev
left a comment
rfsaliev
left a comment
Member
There was a problem hiding this comment.
Load from stream to be changed to support graph allocator.
Member
Author
|
Thanks for the feedback @rfsaliev. I will go through the code again and clean up before marking it ready for review. Agree that allocator is still missing. Probably some more changes are needed to fully implement eshenayo/valkey-search#12 |
Brings in index conversion (#392) and the IndexVamana split into index_vamana.{hpp,cpp}. Stream load/save sits beside the new convert API: - svs_c.h, svs_c.cpp, dispatcher_*: keep both the stream-load and the convert/copy entry points. - index.hpp: Index::save(std::ostream&) is appended after main's size(), keeping new virtuals at the end of the vtable. - index_vamana.hpp: carry the save(std::ostream&) overrides over from the removed in-header IndexVamana and DynamicIndexVamana. - index_builder: move load_stream and load_stream_dynamic into index_builder.cpp and adopt the IndexVamana(const IndexBuilder&, ...) constructor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Vamana::assemble's archive branch passed explicit allocators to view-typed data, bypassing the requires(is_view) overload that throws a clear error. Guard it like the native branch, and share the lazy loaders across both distance branches. DynamicVamana::assemble(std::istream&) defaulted its graph allocator to Blocked<HugepageAllocator>, committing a full block per loaded index for callers that pass no allocator. Default to the exact-size HugepageAllocator again; the C API passes Blocked explicitly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Throw on failbit as well as badbit, so a stream ending inside the last graph payload fails instead of loading stale rows. - Reject a read callback that returns more bytes than requested, and honor a read error even when bytes are returned. - Report a failed write whose callback set SVS_OK as SVS_ERROR_UNKNOWN. - Drop unflushed bytes in ~StreamBuf rather than delivering a truncated chunk during unwinding. - Document the 64 KiB read-ahead, remove the stale allocator limitation, and fix the callbacks' @return text. - Add tests for each case; remove dead test helpers and trim comments. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The directory-archive branch instantiated GraphLoader with the stream-bound view allocator, which has no disk load and broke the build. Views cannot be backed by a temporary directory, so the view overload now accepts only the native stream format and takes no allocators. Passing allocators with a view type is now a compile error instead of being silently ignored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
Note: I think C API bindings test require an update to the prebuilt lib. Correct me if I'm wrong @ethanglaser |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Directory-archive loading exposes unchecked extraction paths that can overwrite files outside the temporary directory.
3 open findings
What changed in this PR
Adds callback-based streaming persistence to the C API, connecting static and dynamic Vamana indexes to existing serialization.
Changes:
- Adds stream interfaces, save/load functions and callback error propagation.
- Extends stream assembly with data and graph allocators.
- Adds round-trip tests, documentation and a C example.
| File | Description |
|---|---|
| tests/svs/index/vamana/index.cpp | Tests view-backed stream assembly. |
| include/svs/orchestrators/vamana.h | Splits owning and view-backed stream assembly. |
| include/svs/orchestrators/dynamic_vamana.h | Adds stream allocator parameters. |
| examples/c/save_load_stream.c | Demonstrates in-memory persistence. |
| examples/c/CMakeLists.txt | Registers the stream example. |
| bindings/c/tests/README.md | Documents stream tests. |
| bindings/c/tests/CMakeLists.txt | Registers stream tests. |
| bindings/c/tests/c_api_stream.cpp | Tests round trips, failures and allocators. |
| bindings/c/src/svs_c.cpp | Implements streaming entry points. |
| bindings/c/src/stream.hpp | Adapts callbacks to C++ streams. |
| bindings/c/src/index.hpp | Adds virtual stream saving. |
| bindings/c/src/index_vamana.hpp | Implements Vamana stream saving. |
| bindings/c/src/index_builder.hpp | Declares stream loaders. |
| bindings/c/src/index_builder.cpp | Connects builders to stream dispatch. |
| bindings/c/src/error.hpp | Preserves callback error codes. |
| bindings/c/src/dispatcher_vamana.hpp | Declares static stream dispatch. |
| bindings/c/src/dispatcher_vamana.cpp | Registers static stream specializations. |
| bindings/c/src/dispatcher_dynamic_vamana.hpp | Declares dynamic stream dispatch. |
| bindings/c/src/dispatcher_dynamic_vamana.cpp | Registers dynamic stream specializations. |
| bindings/c/README.md | Introduces streaming and updates example links. |
| bindings/c/include/svs/c/svs_c.h | Defines public streaming contracts. |
| bindings/c/docs/C_API_Design.md | Documents streaming design and usage. |
| bindings/c/CMakeLists.txt | Includes the stream adapter header. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+899
to
+900
| auto index = builder->impl->load_stream( | ||
| std::make_unique<InputStream>(*stream->ops, stream->self) |
Member
Author
There was a problem hiding this comment.
Pre-existing. Will add a separate PR
| for_simple_specializations<false>(load_stream_closure); | ||
| for_leanvec_specializations<false>(build_closure); | ||
| for_leanvec_specializations<false>(load_closure); | ||
| for_leanvec_specializations<false>(load_stream_closure); |
Comment on lines
+192
to
+194
| // Without this, the sentry swallows a write-callback exception into a silent | ||
| // badbit instead of rethrowing it, so a failed save would report success. | ||
| exceptions(std::ios_base::badbit); |
rfsaliev
reviewed
Oct 7, 2026
rfsaliev
left a comment
rfsaliev
left a comment
Member
There was a problem hiding this comment.
LGFM
but I am afraid of new index loading calls compatibility with LVQ/LeanVec loading.
Unfortunately, "C API (with static library)" failed due to outdated headers in static library package which prevents from such kind of validation.
@ethanglaser, can you please help us to validate this PR compatibility with LVQ/LeanVec code?
Member
Yes, I've pinged Andreas with the steps, lmk if any questions about it |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
svs_index_save_stream,svs_index_load_streamandsvs_index_load_stream_dynamic, backed by astd::streambufadapter over a caller-suppliedsvs_stream_iread/write callback pair.Also adds
coded_errorto pass error codes through the existingwrap_exceptionswrapper.Caveats:
svs_index_savecannot be streamed back in. But I actually don't think this is needed.svs_allocator_iis ignored for graph memory and a stream-loaded dynamic index reallocates its graph on growth (similar to C++ runtime).