Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ENH] Sparse Index Reader/Writer Split #2918

Merged
merged 4 commits into from
Oct 16, 2024
Merged

[ENH] Sparse Index Reader/Writer Split #2918

merged 4 commits into from
Oct 16, 2024

Conversation

HammadB
Copy link
Collaborator

@HammadB HammadB commented Oct 9, 2024 •
edited
Loading

Description of changes

Summarize the changes made by this PR.

  • Improvements & Bug fixes
    • Introduces a reader/writer split for sparse indexes. This makes the code a bit simpler and also removes locking on the read path.
  • New functionality
    • None

Test plan

How are these changes tested?
Existing tests cover the scope

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

None

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 9, 2024

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@HammadB HammadB mentioned this pull request Oct 9, 2024
Merged
1 task
@HammadB Graphite App
Copy link
Collaborator Author

HammadB commented Oct 9, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @HammadB and the rest of your teammates on Graphite Graphite

@HammadB HammadB mentioned this pull request Oct 9, 2024
Merged
1 task
@HammadB HammadB force-pushed the hammad/si_reader_writer_root branch from 6cf1823 to a4eb924 Compare October 9, 2024 16:30
@codetheweb codetheweb mentioned this pull request Oct 10, 2024
Merged
1 task
@HammadB HammadB force-pushed the hammad/si_reader_writer_root branch from 1464d12 to 6fea211 Compare October 11, 2024 05:41
This was referenced Oct 11, 2024
Merged
Merged
@HammadB HammadB force-pushed the hammad/si_reader_writer_root branch 2 times, most recently from 90dda79 to d2485d9 Compare October 11, 2024 12:36
@HammadB HammadB marked this pull request as ready for review October 11, 2024 12:37
HammadB
HammadB commented Oct 11, 2024
View reviewed changes
rust/cache/src/lib.rs
@@ -95,7 +95,7 @@ where
CacheConfig::Unbounded(unbounded_config) => {
Ok(Box::new(UnboundedCache::new(unbounded_config)))
}
CacheConfig::Memory(c) => Ok(c.build_memory().await? as _),
CacheConfig::Memory(c) => Ok(c.build_memory().await?),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these were needless casts and making clippy angry

HammadB
HammadB commented Oct 11, 2024
View reviewed changes
rust/blockstore/src/arrow/blockfile.rs
@@ -52,9 +52,7 @@ impl ArrowBlockfileWriter {
root_manager: RootManager,
) -> Self {
let initial_block = block_manager.create::<K, V>();
// TODO: we can update the constructor to take the initial block instead of having a seperate method
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed this TODO

@HammadB HammadB requested a review from sanketkedia October 11, 2024 12:44
@HammadB HammadB force-pushed the hammad/si_reader_writer_root branch from d2485d9 to 6b36165 Compare October 11, 2024 12:56
HammadB
HammadB commented Oct 14, 2024
View reviewed changes
rust/blockstore/src/arrow/sparse_index.rs
/// - `replace_block` - Replace an existing block with a new one
/// - `len` - Get the number of blocks in the sparse index
/// - `is_valid` - Check if the sparse index is valid, useful for debugging and testing
// #[derive(Clone, Serialize, Deserialize)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

clean this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cleaned in above pr

sanketkedia
sanketkedia reviewed Oct 15, 2024
View reviewed changes
rust/blockstore/src/arrow/sparse_index.rs
/// - `len` - Get the number of blocks in the sparse index
/// - `is_valid` - Check if the sparse index is valid, useful for debugging and testing
// #[derive(Clone, Serialize, Deserialize)]
// pub struct SparseIndex {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: cleanup

sanketkedia
sanketkedia reviewed Oct 15, 2024
View reviewed changes
rust/blockstore/src/arrow/sparse_index.rs Outdated
impl chroma_cache::Weighted for SparseIndex {
fn weight(&self) -> usize {
1
pub(super) fn get_target_block_id(
Copy link
Contributor

@sanketkedia sanketkedia Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

should we add a comment here that it is the caller's responsibility to ensure that the forward btree is synchronized properly if needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this should be internal to the module, its just a helper. i'll address.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

f77b878

@HammadB Graphite App
Copy link
Collaborator Author

HammadB commented Oct 16, 2024

Merge activity

  • Oct 16, 9:29 AM EDT: A user started a stack merge that includes this pull request via Graphite.

@HammadB HammadB changed the base branch from hammad/bf_root to main October 16, 2024 13:34
@HammadB HammadB force-pushed the hammad/si_reader_writer_root branch from f77b878 to 7f4ba89 Compare October 16, 2024 13:38
@HammadB HammadB merged commit 3286f77 into main Oct 16, 2024
71 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanketkedia sanketkedia sanketkedia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@HammadB @sanketkedia