-
Notifications
You must be signed in to change notification settings - Fork 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
test: update filtering of Pinecone mock to imitate doc store #3020
Conversation
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 had a rough pass for style and found a few occasions to use my favorite set operators 😄 No need to address those comments if you don't like them. I know some people have strong feelings on these operators...
I will pause the actual review of _filter
because I realized that we can imitate InMemoryDocumentStore
and rather leverage LogicalFilterClause
to perform filtering in this mock, as done here:
haystack/haystack/document_stores/memory.py
Lines 536 to 538 in aafa017
if filters: | |
parsed_filter = LogicalFilterClause.parse(filters) | |
filtered_documents = list(filter(lambda doc: parsed_filter.evaluate(doc.meta), documents)) |
Please give it a try and let me know if it works or if we should stick to your filtering implementation.
Co-authored-by: Sara Zan <[email protected]>
Co-authored-by: Sara Zan <[email protected]>
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 will merge this for now to test it on the other Pinecone PR, and eventually change the filters implementation there. Thanks!
Related Issues
Proposed Changes:
1. Filtering
The previous part-Pinecone part-SQL PineconeDocumentStore used the SQL db for many filtering tasks. With the update to a pure-Pinecone document store in #2749 this is no longer the case.
That means that during tests, filtering in the new PineconeDocumentStore relies solely on the filtering in the Pinecone mock instance. Unfortunately, this didn't work for more complex filters. This PR includes a new filtering function to handle these more complex filters.
2.
update
methodAnother need stemming from the pure-Pinecone document store is the need to handle labels without SQL. Part of the implementation for this includes the use of Pinecone's
update
method, which was not originally included in the mock. It has been added here.How did you test it?
Manual verification, unit tests
Notes for the reviewer
N/A
Checklist