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

Query method is immutable reference #17

Merged
merged 9 commits into from
Aug 20, 2022
Merged

Conversation

tmpfs
Copy link
Collaborator

@tmpfs tmpfs commented Aug 20, 2022

Closes #16.

@tmpfs
Copy link
Collaborator Author

tmpfs commented Aug 20, 2022

You were right @marcus-pousette, we got another small perf improvement 👍

@tmpfs
Copy link
Collaborator Author

tmpfs commented Aug 20, 2022

@marcus-pousette, I had to revert the changes I made to the Filter type to accommodate owned values which is required for when filters need to perform transformations (such as to_lowercase()). Using a std::borrow::Cow we can allow for no-op filters that just do Cow::from(s) and also transformations that incur allocations such as Cow::Owned(s.to_lowercase()). Some of those performance gains in add_document() have been lost as we have to use String instead of &str but it does allow for no-op filters to have minimal overhead.

Copy link
Collaborator

@marcus-pousette marcus-pousette left a comment

Choose a reason for hiding this comment

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

Left some comments, that might be worth considering.

but the main problem as that I think the count_documents fn has to respect removed set

Good find and solution regarding the Filter I thought about this when reviewing your last PR but I thought there would be a explicit lifetime trick you could do in order to circumvent this, but I realize now it is not possible yet to associate lifetimes with borrows in a fn.

Comment on lines +88 to +89
let mut term_counts: HashMap<String, Vec<usize>> = HashMap::new();
let mut all_terms: Vec<String> = Vec::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let mut term_counts: HashMap<String, Vec<usize>> = HashMap::new();
let mut all_terms: Vec<String> = Vec::new();
let mut term_counts: HashMap<Cow<str>, Vec<usize>> = HashMap::new();
let mut all_terms: Vec<Cow<str>> = Vec::new();

src/lib.rs Outdated Show resolved Hide resolved
src/index.rs Show resolved Hide resolved
src/query.rs Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
src/index.rs Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@marcus-pousette marcus-pousette left a comment

Choose a reason for hiding this comment

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

I think this looks good now. Have you done all the changes you wanted to do? @tmpfs

@tmpfs
Copy link
Collaborator Author

tmpfs commented Aug 20, 2022

Sure @marcus-pousette, I think this is good enough for this PR, ping me when you push your Cow fix, interested to see how that shapes up!

@marcus-pousette marcus-pousette merged commit 67e52f9 into quantleaf:master Aug 20, 2022
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.

query method requires a mutable reference to Index
2 participants