fix error in log message

[anakin87]

Related Issue(s): #1749

Proposed changes:

• In order to specify a correct log message, pass the index argument from _handle_duplicate_documents further on to _drop_duplicate_documents and change the logging there.
  @ZanSara please give it a look.

Pre-flight checklist

• I have read the contributors guidelines
• I have enabled actions on my fork
• If this is a code change, I added tests or updated existing ones
• If this is a code change, I updated the docstrings

[ZanSara]

Looks good! 😊

One last thing missing is to actually pass the index parameter to _drop_duplicate_documents here:

haystack/haystack/document_stores/base.py

Line 356 in 0021668

documents = self._drop_duplicate_documents(documents)

Once done I will merge it 👍

[masci]

Please add tests before merging

[anakin87]

One last thing missing is to actually pass the index parameter to _drop_duplicate_documents here:

@ZanSara sorry for forgetting 😅

@masci there are already these two tests:

haystack/test/document_stores/test_document_store.py

Line 122 in 1168f63

def test_write_with_duplicate_doc_ids(document_store: BaseDocumentStore):

haystack/test/document_stores/test_document_store.py

Line 136 in 1168f63

def test_write_with_duplicate_doc_ids_custom_index(document_store: BaseDocumentStore):

Should we think of other tests?

[ZanSara]

@masci I agree with @anakin87 here, this change affects only a log line, and has no functional impact on neither _handle_duplicate_documents or _drop_duplicate_documents. I don't think we need additional tests for it.

We might use caplog to check the log is emitted with the correct index name, but I don't think it's that important honestly 😄

[masci]

@masci I agree with @anakin87 here, this change affects only a log line, and has no functional impact on neither _handle_duplicate_documents or _drop_duplicate_documents. I don't think we need additional tests for it.

This adds logic and I believe it's not directly covered https://github.com/deepset-ai/haystack/pull/2719/files#diff-f69bf03b2cc38f923c98a0d1a1375adac40255f48a5ee8754181c92af3197408R618

Edit: outside the context of this PR, but changing a signature of a function without the need of changing tests is a yellow flag to me.

[anakin87]

I try to recap.

Current situation
What happens: dropped duplicate documents in CUSTOM_INDEX
Log message: "Document with id ... already exists in index DEFAULT_INDEX" 🐛

Fixed situation
What happens: dropped duplicate documents in CUSTOM_INDEX
Log message: "Document with id ... already exists in index CUSTOM_INDEX" 👌

In practice, the index parameter is passed to _drop_duplicate_documents only to return the right logging message, but the actual dropping does not depend on this parameter.

@masci however, if we identify significant test cases, we can include them. I can't think of anything particularly relevant, but as @ZanSara suggested, we could verify the log using caplog. @masci what do you think?

[ZanSara]

I stand by my opinion here that that line you link does not affect the functionality of _drop_duplicate_documents. It could be safely removed and placed into the log's f-string. We can actually do this, to make it more explicit that the index is used nowhere else.

Edit: outside the context of this PR, but changing a signature of a function without the need of changing tests is a yellow flag to me.

Agreed, but in this specific case this is a private function with a very limited use, and it's covered by two tests that check the important parameters. A reasonable solution here, to avoid the signature change, would be to merge _handle_duplicate_documents and _drop_duplicate_documents: I don't see at a first glance why they are separate.

[masci]

No need to test logging output. I stand by my point that index or self.index translates to a conditional branch and this would probably lower a diff coverage report (if we had one :)) but there's no need to block this PR.