-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ENH] Prefetch block by prefixes #4623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: sicheng/05-19-_tst_add_unit_test_for_literal_expr
Are you sure you want to change the base?
[ENH] Prefetch block by prefixes #4623
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Please tag your PR title with one of: [ENH | BUG | DOC | TST | BLD | PERF | TYP | CLN | CHORE]. See https://docs.trychroma.com/contributing#contributing-code-and-ideas |
Implements Blockfile Prefetch by Prefixes This PR adds the ability to prefetch multiple blocks in blockfile storage by specifying a set of prefixes, allowing more efficient batched loading of relevant data blocks. The update introduces new methods to ArrowBlockfileReader and related reader interfaces, as well as supporting logic in the sparse index for resolving block IDs from multiple prefixes. Key Changes: Affected Areas: This summary was automatically generated by @propel-code-bot |
f3cedac
to
78dbbb3
Compare
dedfbd1
to
3feb7e0
Compare
prefixes: impl IntoIterator<Item = &'prefix str>, | ||
) { | ||
match self { | ||
BlockfileReader::MemoryBlockfileReader(_reader) => unimplemented!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[BestPractice]
It looks like you've implemented an unimplemented!() case for MemoryBlockfileReader. While this works for now, consider adding a TODO comment explaining when this might be implemented or why it's left unimplemented to help future developers understand the design decision.
@@ -362,7 +362,47 @@ impl SparseIndexReader { | |||
result_uuids | |||
} | |||
|
|||
pub(super) fn get_block_ids_range<'prefix, 'referred_data, PrefixRange>( | |||
pub(super) fn get_block_ids_for_prefixes(&self, mut prefixes: Vec<&str>) -> Vec<Uuid> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Documentation]
The new get_block_ids_for_prefixes
function could benefit from some documentation comments explaining its purpose, behavior, and usage - especially since it's a key component of the newly added functionality.
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?