Skip to content

Read the external id map alongside the mmapped CSR - #43

Merged
chishui merged 2 commits into
opensearch-project:mainfrom
zirui-song-18:mmap-csr-id-map
Sep 3, 2026
Merged

Read the external id map alongside the mmapped CSR#43
chishui merged 2 commits into
opensearch-project:mainfrom
zirui-song-18:mmap-csr-id-map

Conversation

@zirui-song-18

Copy link
Copy Markdown
Collaborator

Description

Add IDMapIndex::read_csr_and_read_id: the delegate ingests vectors from the CSR (borrowed via mmap) and the Lucene -> internal id map is read from a companion [int64 count][idx_t x count] file, row-aligned with the CSR. The file-based analog of add_with_ids. The id file is fully validated before the CSR is ingested, so a bad file leaves the index untouched.

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Zirui Song <zrsong@amazon.com>

@chishui chishui left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swig file also needs to be updated with read_csr_and_read_id, add one test in python

Comment thread nsparse/id_map_index.h Outdated
void add_with_ids(idx_t n, const idx_t* indptr, const term_t* indices,
const float* values, const idx_t* ids) override;

void read_csr_and_read_id(const char* csr_path, const char* id_path,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about read_csr_and_ids?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to read_csr_and_ids throughout

Comment thread nsparse/id_map_index.cpp Outdated
// the count-vs-CSR-row check can fail once the delegate has ingested; on
// ANY throw from this method the half-built index must be discarded, not
// reused.
if (!std::filesystem::exists(id_path)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a check_if_file_valid in check.h to include null pointer check and file exist check

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great call. Ack.

Comment thread nsparse/id_map_index.cpp
std::to_string(delegate_size) + "): " + id_path);
}

internal_to_external_ = std::move(internal_to_external);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from 167-172 is shared logic with other functions, make it a function

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

@chishui chishui left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add t

Comment thread nsparse/id_map_index.cpp Outdated
set_id_map(std::move(internal_to_external));
}

void IDMapIndex::set_id_map(std::vector<idx_t> internal_to_external) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void IDMapIndex::set_id_map(std::vector<idx_t> &&internal_to_external)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

Comment thread nsparse/id_map_index.cpp
// the count-vs-CSR-row check can fail once the delegate has ingested; on
// ANY throw from this method the half-built index must be discarded, not
// reused.
std::ifstream in(id_path, std::ios::binary);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about moving ids read to a private function and called from here

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

Signed-off-by: Zirui Song <zrsong@amazon.com>
@chishui
chishui merged commit 21116a1 into opensearch-project:main Sep 3, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants