[ENH] Handle metadata deletes + fix bugs related to Updates/deletes in the metadata writer #2344
Conversation
This file contains 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
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
HammadB
reviewed
Jun 16, 2024
HammadB
reviewed
Jun 16, 2024
HammadB
reviewed
Jun 16, 2024
|
|
||
| // 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.
Why does this take base_metadata as a tuple? Feels clunky
HammadB
reviewed
Jun 16, 2024
HammadB
reviewed
Jun 16, 2024
| 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.
This tupling feels unnecessary? What is your explanation for doing it this way?
HammadB
reviewed
Jun 16, 2024
| 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.
I find materialize_update_metadata + merge_update_metadata in tandem very confusing.
HammadB
approved these changes
Jun 16, 2024
|
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 |
sanketkedia
force-pushed
the
metadata_update_bugs
branch
2 times, most recently
from
621a903
to
d2fe0ab
Compare
sanketkedia
force-pushed
the
metadata_update_bugs
branch
from
d2fe0ab
to
f69f57f
Compare
Anush008
pushed a commit
to Anush008/chroma
that referenced
this pull request
Jun 27, 2024
…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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 of changes
Summarize the changes made by this PR.
Test plan
pytestfor python,yarn testfor js,cargo testfor rustDocumentation Changes
None