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.

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

Delete term set #2284

Merged
merged 24 commits into from
Nov 18, 2022
Merged

Delete term set #2284

merged 24 commits into from
Nov 18, 2022

Conversation

trinity-1686a
Copy link
Contributor

fix #2217

Add support for deleting using a set of terms (and searching with a term of search).

Comment on lines +206 to +208
&'a self,
query: &CacheKey<'b, T>,
range_end: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

I could be wrong... because it requires a bit of code wrangling to check that but....

wouldn't the code be clearer by changing this function into...

fn get_block<'a>(&'a self, tag: &'b T, byte_range: Range<usize>) -> Option<(Range<usize>, &CacheValue)> {}

the goal being to restrict the number of place where we manipulate CachedKey.

Copy link
Contributor

Choose a reason for hiding this comment

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

...
i'm not sure the refactoring I suggest is a good idea, due to the write usage.

}

fn put_slice(&mut self, tag: T::Owned, byte_range: Range<usize>, bytes: OwnedBytes) {
let len = byte_range.end - byte_range.start;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this code could have been mildly simpler... but it is well encapsulated and well tested, so actually I think we don't care.

@trinity-1686a trinity-1686a marked this pull request as ready for review November 17, 2022 16:02
@trinity-1686a trinity-1686a enabled auto-merge (squash) November 18, 2022 11:58
@trinity-1686a trinity-1686a merged commit 897e9da into main Nov 18, 2022
@trinity-1686a trinity-1686a deleted the delete-term-set branch November 18, 2022 12:10
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.

Make it possible to delete a set of terms. (using the TermSetQuery on tantivy side)
2 participants