-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[ENH] Handle metadata deletes + fix bugs related to Updates/deletes in the metadata writer #2344
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
||
// Merges update metadata to base metadata, updating | ||
// the delete list and upsert list. | ||
pub(crate) fn merge_update_metadata( |
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.
Why does this take base_metadata as a tuple? Feels clunky
record_from_map.metadata_to_be_merged = match merge_update_metadata( | ||
&record_from_map.metadata_to_be_merged, | ||
match merge_update_metadata( | ||
( |
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.
This tupling feels unnecessary? What is your explanation for doing it this way?
use thiserror::Error; | ||
|
||
use super::record_segment::{ApplyMaterializedLogError, RecordSegmentReader}; | ||
|
||
// Materializes metadata from update metadata, populating the delete list | ||
// and upsert list. | ||
pub(crate) fn materialize_update_metadata( |
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.
I find materialize_update_metadata + merge_update_metadata in tandem very confusing.
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.
This is fine. The metadata update logic and state handling is very hard to follow IMO.
Discussed offline that nothing obvious comes to mind wrt simplifying the metadata merge. At this point, ok to move on but revisit this later and assess if there are possibilities to do this |
621a903
to
d2fe0ab
Compare
d2fe0ab
to
f69f57f
Compare
…n the metadata writer (chroma-core#2344) ## Description of changes *Summarize the changes made by this PR.* - Improvements & Bug fixes - Handles metadata deletes - Full text writer adds (token, freq) pair even if freq is 0. Fixes this. - Full text writer does not remove postings list of documents that have been deleted. Fixes this. - Fix for test_query_without_add ## Test plan - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Documentation Changes None
Description of changes
Summarize the changes made by this PR.
Test plan
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
None