Use ElasticsearchDocumentStore.get_all_documents in ElasticsearchFilterOnlyRetriever.retrieve

[adri1wald]

Closes #2139

Proposed changes:

• Previously ElasticsearchFilterOnlyRetriever.retrieve used ElasticsearchDocumentStore.query, which takes a top_k parameter but doesn't apply it to the es request body on the if query is None branch.
• Based on discussion with @julian-risch and @mathislucka, top_k doesn't have semantic meaning without a query anyways, because there is nothing to score agains and thus no ordering over which to select the top k documents.
• We came to the conclusion that "the expected behavior of the ElasticsearchFilterOnlyRetriever is to return all documents that match the given filter and not use a top_k at all"
• ElasticsearchFilterOnlyRetriever.retrieve therefore now uses ElasticsearchDocumentStore.get_all_documents, which returns all documents matching the filters.

Status (please check what you already did):

• First draft (up for discussions & feedback)
• Final code
• Added tests
• Updated documentation

[adri1wald]

The tests around retrievers are a bit confusing to me - would love some pointers on how to test this change.

[julian-risch]

The tests around retrievers are a bit confusing to me - would love some pointers on how to test this change.

Hi @adri1wald sure, I think it would be nice to add a new test case here that is parameterized with an ElasticsearchDocumentStore and an ElasticsearchFilterOnlyRetriever:

@pytest.mark.parametrize("retriever", ["es_filter_only"], indirect=True)
@pytest.mark.parametrize("document_store", ["elasticsearch"], indirect=True)
def test_es_filter_only(document_store, retriever, docs):
...

There you could add let's say 12 documents of which 11 documents should be selected by the filter. If all 11 are returned, the test passes. Any other idea?

[ZanSara]

Hey @adri1wald, do you think you'll have time to work on this PR or shall we take over, apply the last touches and merge?

[adri1wald]

Hey @ZanSara, really sorry I completely forgot about this PR :’) I will make some time this weekend to finish it off!

[ZanSara]

Great, thanks! 🙂

[julian-risch]

Hi @adri1wald it would be really nice to include your changes in the next release. Do you think you could make the final changes by the end of next week (end of April)?

[adri1wald]

Hey, major apologies for the delay. I've added the test case now. Let me know if there's anything else that needs doing!

[julian-risch]

Hi @adri1wald thanks for adding the test. Right now, it's failing because of the following line of code:

        document_store.write_documents(docs)
>       retrieved_docs = retriever.retrieve(filters={"f1": ["0"]})
E       TypeError: retrieve() missing 1 required positional argument: 'query'

Currently, the retrieve method has no default value for query and therefore the test fails.

[julian-risch]

Looks great to me, ready to merge! 👍 Thank you @adri1wald for your contribution!