Keep the mapped CSR and the mapped index in separate members - #47
Merged
zirui-song-18 merged 1 commit intoSep 4, 2026
Merged
Conversation
MmapIndex held one MmapFile for both mappings a borrowed buffer can point into, on the grounds that a native CSR corpus and a serialized index file are mutually exclusive sources. They are, but they are populated by unrelated paths and released at different times, and the single member made that hard to see: InvertedIndex::build() closes the mapping once its posting lists own their entries, which is only ever correct for the CSR side, while every mmap_index() assigns the index side. Split it into csr_mapping_ and index_mapping_. Both stay declared ahead of vectors_ and batch_spill_, so reverse-declaration-order destruction still outlives every borrower. No behaviour change. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
chishui
requested review from
model-collapse,
yuye-aws and
zirui-song-18
as code owners
September 4, 2026 01:27
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
2 participants
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.
Description
MmapIndexheld oneMmapFile mapped_file_for both mappings a borrowed buffer can point into: a native CSR corpus fromread_csr's mapped residency, and a serialized index file from a derivedmmap_index(). Those sources are mutually exclusive, but they're populated by unrelated paths and released at different times, which one member made hard to see —InvertedIndex::build()closes the mapping once its posting lists own their entries, correct only for the CSR side, while everymmap_index()assigns the other.Split into
csr_mapping_andindex_mapping_. Both stay declared ahead ofvectors_andbatch_spill_, so reverse-declaration-order destruction still outlives every borrower.batch_spill_keeps its own mapping:release()must unmap before unlinking, so path and mapping belong together.No behaviour change; comments updated to match.
Check List
ctest)Apache 2.0 license.