-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: BaseVectorStorage-Add-Search-Function #2058
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
base: master
Are you sure you want to change the base?
Conversation
add the search test
Add the search function test
Add the search function test
zjrwtx
left a comment
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.
thanks @subway-jack !left some comments
| field_name="id", | ||
| datatype=DataType.VARCHAR, | ||
| descrition='A unique identifier for the vector', | ||
| descrition="A unique identifier for the vector", |
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.
| descrition="A unique identifier for the vector", | |
| description="A unique identifier for the vector", |
|
please run pre-commit before commit |
| id: str, | ||
| payload: Dict[str, Any], | ||
| ) -> "VectorDBSearchResult": | ||
| r"""A class method to construct a `VectorDBSearchResult` instance.""" |
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.
docstring may need may clear
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.
Thanks, I'll add more details
| id: str, | ||
| payload: Dict[str, Any], | ||
| ) -> "VectorDBSearchResult": | ||
| r"""A class method to construct a `VectorDBSearchResult` instance.""" |
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.
docstring may need more clear
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.
Thanks, I'll add more details
|
Thanks @subway-jack for the contributions! @AveryYay Hi Avery. It would be great if you can help with reviewing this PR. |
add the more description detail
add the more description detail
add the more description detail
add the more description detail
fix the E501
Sure! |
| if isinstance(cond, dict): | ||
| expressions = [] | ||
| for op, v in cond.items(): | ||
| if op == "$eq": |
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 would consider using a dictionary to do the if-else matching here
hesamsheikh
left a comment
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.
Thanks for the PR @subway-jack. I have left some comments.
| payload_filter (Dict[str, Any]): The filter criteria used to | ||
| select records.For example, {"category": "fruit", "color": | ||
| "red"}. | ||
| top_top_k (int, optional): The number of top similar vectors to |
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.
typo in top_top_k
| """ | ||
|
|
||
| record: VectorRecord | ||
| match_info: Dict[str, Any] |
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.
Is this actually used?
| @abstractmethod | ||
| def search( | ||
| self, | ||
| search: VectorDBSearch, |
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 think this is supposed to be "filter" not search
| pass | ||
|
|
||
| @abstractmethod | ||
| def search( |
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.
maybe for the sake of name clarity, it's better to rename this function to search_by_payload, to differentiate from the filter funciton
| if index == search.top_k - 1: | ||
| break |
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.
could you please make sure that the SDK does not support an argument like limit or similar?
| from datetime import datetime | ||
| from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple, Union, cast | ||
|
|
||
| from qdrant_client.http.models import Filter |
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.
this is already imported multiple times in the functions
| def search( | ||
| self, | ||
| search: VectorDBSearch, | ||
| **kwargs: Any, | ||
| ) -> List[VectorDBSearchResult]: |
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.
empty function
|
hello @subway-jack ,anything blocking you?let us know if you need any help,thanks |
Thank you for the gentle reminder. I’m sorry I haven’t pushed the latest changes yet—some other tasks cropped up unexpectedly. I’ll make sure to update this PR later this week. |
|
Hey @subway-jack Thanks again for contributing to this PR, would love to hear about how progress has been coming along and whether we can help in any way! |
Description
Describe your changes in detail (optional if the linked issue already contains a detailed description of the changes).
Checklist
Go over all the following points, and put an
xin all the boxes that apply.Fixes #issue-numberin the PR description (required)pyproject.tomlanduv lockIf you are unsure about any of these, don't hesitate to ask. We are here to help!