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

feat: add ability to delete by filtering metadata only #57

Merged
merged 13 commits into from
Nov 14, 2023

Conversation

jeanmaried
Copy link
Contributor

@jeanmaried jeanmaried commented Nov 3, 2023

What kind of change does this PR introduce?

Feature

What is the current behavior?

I need to be able to delete vectors based solely on their metadata but currently the only way to do so is to already have the ids.

What is the new behavior?

I added 2 methods as suggestions.
Option #1: add a delete_by_metadata
Option #2: delete_vectors (temporary name) a suggestion for updating the current delete function to allow optionally passing the ids or metdata (one or the other).

Please let me know which is an acceptable approach and if there are any objections to having this capability.

@jeanmaried jeanmaried changed the title feat: add ability to delete by metadata only feat: add ability to delete by filtering metadata only Nov 3, 2023
@olirice
Copy link
Collaborator

olirice commented Nov 3, 2023

Deleting by metadata sounds reasonable. I think the right way to approach it would be to overload the existing delete method in a backwards compatible way

Something like:

From

def delete(self, ids: Iterable[str]) -> List[str]:

To

def delete(self, ids: Optional[Iterable[str]] = None, filters: Optional[Metadata] = None) -> List[str]:

where filters goes through the same build_filter logic as query.

@jeanmaried
Copy link
Contributor Author

@olirice thank you, I updated the method and the docs.

Copy link
Collaborator

@olirice olirice left a comment

Choose a reason for hiding this comment

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

This PR introduces some LOC that aren't covered by the test suite. Could you add tests to handle the new uses case?

These are the uncovered lines:
Screenshot 2023-11-06 at 9 52 02 AM

src/vecs/collection.py Outdated Show resolved Hide resolved
src/vecs/collection.py Outdated Show resolved Hide resolved
src/vecs/collection.py Outdated Show resolved Hide resolved
@olirice
Copy link
Collaborator

olirice commented Nov 14, 2023

very nice, thank you!

@olirice olirice merged commit 7680c01 into supabase:main Nov 14, 2023
5 checks passed
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