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

Extend meta data support for SQLDocumentStore #2199

Merged
merged 17 commits into from
Feb 21, 2022

Conversation

MichelBartels
Copy link
Contributor

Proposed changes:
As discussed in #1958, this PR extends SQLDocumentStore to support extended meta data filtering with the format described in #2108.

When this PR and #2143 are merged, the new filter functionality can be used everywhere where filtering could be used before. However, there are still changes necessary to make sure filtering can be used in all query methods as those currently do not support filtering at all.

Status (please check what you already did):

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

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

LGTM 👍 One of the import statements seems not to be needed though: from haystack.utils.import_utils import safe_import. Feel free to merge after removing it.

@@ -216,7 +216,7 @@ def test_get_all_documents_with_incorrect_filter_value(document_store_with_docs)
assert len(documents) == 0


@pytest.mark.parametrize("document_store_with_docs", ["elasticsearch"], indirect=True)
@pytest.mark.parametrize("document_store_with_docs", ["elasticsearch", "sql"], indirect=True)
Copy link
Member

Choose a reason for hiding this comment

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

That's good. It's exactly what I meant in your other PR for the InMemoryDocumentStore. 👍

Copy link
Member

Choose a reason for hiding this comment

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

Just make sure that there are no problems when merging this with Bogdan's changes in the same line of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have pulled master manually to take care of that.

@@ -1,6 +1,12 @@
from typing import Union, List, Dict
from abc import ABC, abstractmethod
from collections import defaultdict
from functools import reduce

from haystack.utils.import_utils import safe_import
Copy link
Member

Choose a reason for hiding this comment

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

I can't see where safe_import is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, this import is unnecessary. I've removed it now.

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.

2 participants