-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add new queryByVectorId
and queryByVector
functions to IndexInterface
#106
Conversation
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 pushing this change, left a small comment, LGTM!
@Override | ||
public ListenableFuture<QueryResponseWithUnsignedIndices> queryByVector(int topK, List<Float> vector, | ||
String namespace, Struct filter) { | ||
return query(topK, vector, null, null, null, namespace, filter, false, false); |
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.
Not saying this is wrong or right, but OOC why do you allow users to have a filter
when they query by vector, but not when they query by ID? I'd say they both probs would benefit from the inclusion of a filter
, since users who use these types of queries (I'd imagine) are looking for similarity clusters, essentially. So getting all vectors similar to a given vector (or a given ID), filtered by xyz
would be helpful
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 we support what you're talking ab out on ln 100 in this file, am I misunderstanding?
@Override
public ListenableFuture<QueryResponseWithUnsignedIndices> queryByVectorId(int topK,
String id,
String namespace,
Struct filter) {
return query(topK, null, null, null, id, namespace, filter, false, false);
}
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.
Oh, my bad! I must've just missed this while reviewing. Thanks a lot! Carry on!
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.
The code changes look good to me, but do we need any unit and integration tests for these? I think we do. I wouldn't merge w/o those.
…nterface and implementing classes
1d4e0a5
to
cbe9a7d
Compare
We should have them, but we're a bit strapped for time at the moment. I've added Asana tickets to track adding unit tests for our |
Problem
Because of the lack of optional arguments, we have a lot of overloaded functions in
IndexInterface
which are meant to make working with the data plane easier for users.Currently, we're missing some variations of
queryByVectorId
which need to be added. We're also missing any functionality forqueryByVector
behavior which is a commonly used operations by users.Solution
IndexInterface
to supportqueryByVectorId
minimal requests with namespace and/orincludeValues
andincludeMetadata
.queryByVectorId
functions into newqueryByVector
functions.All of these functions are going through the existing validation logic, so it should be safe to add these in calling the base
query
function. Unfortunately, there's not any unit tests forIndex
orAsyncIndex
and those will need to be added in a follow up.This is also a lot of overloads, I'm sure there is a better pattern for handling this from a UX perspective.
Type of Change
Test Plan
CI tests, or running an example app locally.