Sd optimize streaming merge#3
Merged
Merged
Conversation
Adds a doc-by-doc StoredStreamWriter for .fdt/.fdx (generic over std::io::Write, no libc/unix deps) and reimplements write_stored on top of it so the batch path proves the streaming path is byte-identical. TDD: added stream_writer_matches_batch_writer_byte_for_byte first, watched it fail to compile, then implemented. Reuses the exact byte layout from the old write_stored (no new format).
Add a term-at-a-time TermDictStreamWriter for .tis/.tii/.frq/.prx, driven over Write+Seek sinks (headers written up front, term-count back-patched at offset 4 on finish). Reimplement write_term_dict on top of it via Cursor<Vec<u8>> sinks, proven byte-identical to the prior single-pass implementation by a new test covering multiple fields, shared prefixes, >128 terms (.tii sampling + count patch), and multi-doc postings with positions.
stream_writer_matches_batch_writer_byte_for_byte compared TermDictStreamWriter against write_term_dict, but write_term_dict is now a thin wrapper over the same streaming writer, so the test compared it against itself. A reviewer-injected pointer-delta bug slipped past it and was only caught by an unrelated reader round-trip test. Add reference_write_term_dict, a self-contained copy of the pre-refactor (commit 0654030) algorithm duplicated into the test module under reference_* names, and assert the streaming writer's four output buffers against it byte-for-byte instead. Verified the gate by temporarily reintroducing the same class of pointer-delta bug (frq_len off-by-one), confirming the test fails, then reverting.
TermDict::cursor() / ZslSegment::term_cursor() yield (field, term) pairs in ZSL canonical order (field names ascending, then term ascending within each field) without materializing a Vec of all terms like all_terms() does. Backed directly by FieldTerms's already-sorted per-field buffer.
… a real index copy)
Adds CfsSource (Mem/Path) and write_cfs_streaming, which produces the same byte layout as write_cfs (VInt(fileCount) + directory + blocks) but computes all directory offsets up front (source lengths known in advance) and streams each data block into the output writer, so Path sources (temp files) never need to be loaded fully into RAM. TDD: added tests asserting byte-identity against write_cfs for an all-Mem case and a mixed Mem/Path case (temp file), both passing. Full sdsearch-core suite (147 lib tests + 17 integration tests) stays green.
…ismatch CfsSource::len() reads a Path source's length from metadata once, up front, to compute CFS directory offsets, but write_to discarded io::copy's return value. If the file's actual size ever diverged from the metadata length (truncation/replacement between the two calls), write_cfs_streaming would return Ok while emitting a .cfs whose directory offsets didn't match the data written. write_to now takes the expected length (the same value used for the directory offset) and errors with UnexpectedEof if the bytes actually copied don't match, instead of silently corrupting the compound file. Also documents that a partial write can land in `out` on Err.
Add merge_segments_streaming: reproduces merge_segments' 4 phases exactly but streams the big blocks (.fdt/.frq/.prx) through temp files under index_dir and writes the merged .cfs durably (fsync) itself, so optimize()'s peak heap is independent of total text volume. Terms are merged k-way across each segment's term_cursor via a Reverse-keyed BinaryHeap, fed to the streaming dict writer in ZSL canonical order; only small blocks (.fnm/.fdx/.nrm/.tis/.tii + doc_maps/norm_cols) stay in RAM. Temp files are cleaned up on both the success and error paths. Wire optimize() to it (replacing merge_segments + write_durable); keep merge_segments as the byte-identity oracle. Differential tests assert the streaming .cfs is byte-identical to the batch merge on 3 scenarios (multi-segment no deletes, deletes across two base segments, single-segment with deletes) plus binary-stored-field and no-.prx error parity. All 182 sdsearch-core tests pass; clippy clean; std-only (Windows-safe).
…oc note stream_writer_matches_batch_writer_byte_for_byte compared StoredStreamWriter against write_stored, but write_stored is now a thin wrapper over the same streaming writer, so the test compared it against itself (same class of issue fixed for terms.rs in d93b16d). Add reference_write_stored, a self-contained copy of the pre-streaming (commit 583e39b) algorithm, and assert the streaming writer's fdt/fdx against it byte-for-byte instead. Verified the gate by temporarily flipping the flags byte in StoredStreamWriter::add_doc, confirming the test fails, then reverting. Add streaming_matches_batch_all_docs_deleted_zero_live_docs, a true 0-live-doc differential scenario (every doc in the KB fixture deleted), alongside the existing streaming-vs-batch merge scenarios. Note on merge_segments that it is retained as the differential-test oracle and a potential future small-index fallback, not on the production optimize() path.
Old generation manifest files (segments_1, segments_2, ...) were never deleted after commit()/optimize() flipped segments.gen to point at the new one, so they accumulated forever (a real index had ~270). Add a best-effort prune_old_generations() helper that runs right after each durable segments.gen flip, in write_generation_with_delgens (commit path) and write_optimized_generation (optimize/merge path), deleting segments_<base36> manifests strictly older than the immediately- previous generation. Keeps the current and previous manifests as a grace window for lock-free concurrent readers, compares generation numbers numerically (not lexically) via a new from_base36 parser, and never touches segments.gen or any .cfs/.del/.sti/.tmp/lock file.
…ng + optimize-per-feed guidance
This comment has been minimized.
This comment has been minimized.
Contributor
Code Metrics Report
Details | | main (bcba60d) | #3 (9a7617c) | +/- |
|---------------------|----------------|--------------|-------|
+ | Coverage | 96.4% | 96.8% | +0.3% |
| Files | 36 | 36 | 0 |
| Lines | 4398 | 5274 | +876 |
+ | Covered | 4244 | 5110 | +866 |
- | Test Execution Time | 4s | 5s | +1s |Code coverage of files in pull request scope (97.0% → 97.6%)
Reported by octocov |
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
1 participant
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.
No description provided.