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.

Sign up for GitHub

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

Jump to bottom

feat: support dynamic filters in custom_query #5427

Merged
merged 13 commits into from
Aug 8, 2023
Merged

feat: support dynamic filters in custom_query #5427

merged 13 commits into from
Aug 8, 2023

Conversation

tstadel
Copy link
Member

@tstadel tstadel commented Jul 25, 2023 •
edited
Loading

Related Issues

Proposed Changes:

  • add ${filters} placeholder analogous to ${query}
  • fix tests:
    • test_custom_query was not running at all
    • split into document_store integration and retriever unit test
  • adjust docstrings for custom_query param

How did you test it?

  • added tests and ensured that they ran

Notes for the reviewer

  • added tag breaking change as we won't support the old filter expressions in custom_query anymore. As those old expressions didn't allow for dynamic filtering at query time and thus their value was limited it's best to get rid of them here. Any old filter expression can be replaced by the new filter style.
  • This PR doesn't change the input format of custom_query (i.e. str). We can add support for dict in another PR to facilitate setting custom queries.

Checklist

@coveralls
Copy link
Collaborator

coveralls commented Jul 25, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5795432105

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 228 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.03%) to 46.913%
Files with Coverage Reduction New Missed Lines %
utils/context_matching.py 2 89.58%
nodes/prompt/invocation_layer/hugging_face.py 9 94.19%
nodes/retriever/sparse.py 63 48.21%
document_stores/search_engine.py 68 62.74%
document_stores/opensearch.py 86 66.8%
Totals Coverage Status
Change from base Build 5762104157: 0.03%
Covered Lines: 10902
Relevant Lines: 23239
💛 - Coveralls

@github-actions github-actions bot added topic:retriever type:documentation Improvements on the docs labels Jul 25, 2023
@tstadel tstadel changed the title feat: support filters in custom_query feat: support dynamic filters in custom_query Jul 25, 2023
@tstadel tstadel marked this pull request as ready for review July 25, 2023 13:09
@tstadel tstadel requested review from a team as code owners July 25, 2023 13:09
@tstadel tstadel requested review from dfokina and vblagoje and removed request for a team July 25, 2023 13:09
vblagoje
vblagoje requested changes Aug 3, 2023
View reviewed changes
Copy link
Member

@vblagoje vblagoje left a comment

Choose a reason for hiding this comment

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

@tstadel can you please add some non-happy path tests? For example, why not test an empty filter list, a mix of years and random strings, all the years plus some more in your filter query test? Also, what happens if we don't use the right meta key, e.g "month"? I am playing devil's advocate here :-)

@tstadel
Copy link
Member Author

tstadel commented Aug 4, 2023

@tstadel can you please add some non-happy path tests? For example, why not test an empty filter list, a mix of years and random strings, all the years plus some more in your filter query test? Also, what happens if we don't use the right meta key, e.g "month"? I am playing devil's advocate here :-)

Adding a test for empty filters makes sense as it's specific for the custom query how we handle those cases.
Aren't the other tests more for filters in general? None of those are specific to custom queries.

@tstadel
Copy link
Member Author

tstadel commented Aug 4, 2023

@tstadel can you please add some non-happy path tests? For example, why not test an empty filter list, a mix of years and random strings, all the years plus some more in your filter query test? Also, what happens if we don't use the right meta key, e.g "month"? I am playing devil's advocate here :-)

Adding a test for empty filters makes sense as it's specific for the custom query how we handle those cases. Aren't the other tests more for filters in general? None of those are specific to custom queries.

Just saw that we already have a test for empty filters. I will refactor them using parameterize.

@vblagoje
Copy link
Member

vblagoje commented Aug 4, 2023

@tstadel rebase to main, and these Coveralls failures will go away

@tstadel tstadel requested a review from vblagoje August 7, 2023 11:41
vblagoje
vblagoje approved these changes Aug 7, 2023
View reviewed changes
Copy link
Member

@vblagoje vblagoje left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @tstadel , LGTM 🚀

agnieszka-m
agnieszka-m approved these changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@agnieszka-m agnieszka-m left a comment

Choose a reason for hiding this comment

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

Commited my changes here directly

@vblagoje vblagoje merged commit d46c84b into main Aug 8, 2023
66 checks passed
@vblagoje vblagoje deleted the feat/custom_query_filters branch August 8, 2023 13:48
DosticJelena pushed a commit to smartcat-labs/haystack that referenced this pull request Aug 23, 2023
@tstadel @agnieszka-m
feat: support dynamic filters in custom_query (deepset-ai#5427)
fee3666 
* support filters in custom_query

* better tests

* Update docstrings

---------

Co-authored-by: agnieszka-m <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vblagoje vblagoje vblagoje approved these changes

@agnieszka-m agnieszka-m agnieszka-m approved these changes

@dfokina dfokina Awaiting requested review from dfokina dfokina is a code owner automatically assigned from deepset-ai/documentation

Assignees
No one assigned
Labels
breaking change topic:document_store topic:elasticsearch topic:opensearch topic:retriever topic:tests type:documentation Improvements on the docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support dynamic filters in custom_query (elasticsearch, OpenSearch)
4 participants
@tstadel @coveralls @vblagoje @agnieszka-m