-
Notifications
You must be signed in to change notification settings - Fork 78
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
feat: Add filter_policy to mongodb_atlas integration #823
Conversation
@@ -108,7 +114,11 @@ def run( | |||
:returns: A dictionary with the following keys: | |||
- `documents`: List of Documents most similar to the given `query_embedding` | |||
""" | |||
filters = filters or self.filters | |||
if self.filter_policy == "merge" and filters: |
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.
it seems there's a test missing for this condition (filter_policy = 'merge'
), no? and what happens if but for some reason filters = None
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.
Yes, missing test, noted. If filters is None then else is executed and filters become either defined by user or {}
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'll add merge use case to all PRs
Please delay review until a new patch release of 2.2.x is released. We need these two PRs to be included in a patch release first. |
Ready for review now @davidsbatista
|
Stefano recommended one more small improvement. Please hold your review until a new commit with it is added here |
@davidsbatista please have a look at #827 @anakin87 has already gone through a through review of Astra and should help you validate this PR is well-aligned as well |
Why:
Adds flexible filtering options to mongodb_atlas integration. By introducing a filter policy option (
replace
ormerge
), developers can now control how runtime filters are applied relative to initialization time filters.filter_policy
init parameter to all retrievers #780What:
filter_policy
parameter with optionsreplace
andmerge
across multiple retrievers to control filter behavior dynamically.How can it be used:
Dynamic Filter Behavior Adjustment: Users can decide whether to completely override the initial filters set during the retriever's initialization (
replace
) or merge them with runtime filters, with the latter taking precedence (merge
).Complex Search Scenarios:
merge
option allows for an additive approach.replace
option offers a clean slate for filters at runtime.How did you test it:
replace
andmerge
scenarios for thefilter_policy
parameter.