-
Notifications
You must be signed in to change notification settings - Fork 1
Add search-only mode #29
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
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.
Orca Security Scan Summary
Status | Check | Issues by priority | |
---|---|---|---|
![]() |
Secrets | ![]() ![]() ![]() ![]() |
View in Orca |
src/query/agent.ts
Outdated
*/ | ||
async search<T = undefined>( | ||
query: string, | ||
{ limit, collections }: QueryAgentSearchOnlyOptions = {}, |
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.
IMO, having this be options?: QueryAgentSearchOnlyOptions
would be clearer for a user on the exact nature of this functional argument. Having the object keys explicit and it having a default of {}
is quite different to the rest of the TS client so might be a point of confusion for users
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 makes sense, but doesn't match the existing QueryAgent
methods (which I don't watch to touch now). I'll CC @augustas1 to see what he thinks once he's back, but for now I'd prefer to be consistent with the existing run
/ stream
methods.
I've modified the signature a little though, to try and make the default argument values clearer 👍
src/query/search.ts
Outdated
* @param options.limit - The maximum number of results to return. Defaults to 20 if not specified. | ||
* @param options.offset - The offset to start from. If not specified, retrieval begins from the first object. |
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.
Optional args can be signified in JSdoc using []
like so: [options.limit]
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.
Cool - done! 👍
src/query/search.ts
Outdated
* @param options.offset - The offset to start from. If not specified, retrieval begins from the first object. | ||
* @returns A SearchModeResponse object containing the results, usage, and underlying searches performed. | ||
*/ | ||
async run(options: SearchExecutionOptions): Promise<SearchModeResponse<T>> { |
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.
Should a user be able to just call await search.run()
? If so, options
needs to be optional here: options?: SearchExecutionOptions
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 mostly a method the end user won't touch (they should be interacting with it via the next()
method on the SearchModeResponse
), but I've updated to have default values (following the same convention as above) so you can now call plain await search.run()
.
src/query/search.ts
Outdated
} | ||
return { | ||
...mappedResponse, | ||
next: async ({ limit: nextLimit = 20, offset: nextOffset = 0 } = {}) => |
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 function args here are different to how they are in the run
outer function, might that be confusing from a UX PoV? Can the same SearchExecutionOptions
type be reused here?
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.
Yeah, that's nicer - I've moved the SearchExecutionOptions
around and they're reused here.
src/query/response/response.ts
Outdated
next: (options?: { | ||
limit?: number; | ||
offset?: number; | ||
}) => Promise<SearchModeResponse<T>>; |
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.
Likewise here as above wrt to the SearchExecutionOptions
type
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.
Same as above, now uses SearchExecutionOptions
👍
src/query/response/api-response.ts
Outdated
searches?: ApiSearchResult[]; | ||
usage: ApiUsage; | ||
total_time: number; | ||
search_results: WeaviateReturn<T>; |
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.
Sadly results also probably are returned from API as underscore_case
and not camelCase
, so we'd have to map it as well to make it developer friendly 😢
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.
Argh, good catch! I've adding mapping from the api snake_case
to camelCase
👍 I've also removed all the generics on the types to make this work (we can add them back later if we want, but they were for typing properties but our search results are potentially multi-collection, so maybe didn't make sense anyway 🤷 ).
The types of the search result objects
are also an extension of the Weaviate types, to add a collection
field (which was missing from the original type).
src/query/response/response.ts
Outdated
@@ -260,3 +262,23 @@ export type StreamedTokens = { | |||
outputType: "streamedTokens"; | |||
delta: string; | |||
}; | |||
|
|||
export type MappedSearchModeResponse<T> = { |
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 looks like internal type and everything in this file is exported to user (see index.ts file).
So Maybe move it to response-mapping.ts
?
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.
Makes sense 👍 Moving to response-mapping.ts
would end up with circular dependencies, so I've just removed this type and am using Omit<SearchModeResponse, "next">
in it's place (since it's internal anyway)
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 works as well :) fyi circular dependencies are supported in JS/TS (especially fine with for types)
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.
Huh, TIL!
src/query/search.ts
Outdated
} | ||
return { | ||
...mappedResponse, | ||
next: async (options: SearchExecutionOptions = {}) => this.run(options), |
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.
In SearchModeResponse
options
are declared as required, so probably = {}
shouldn't be here?
On that note, if it's required, we probably expect user to always pass offset
on next
, so maybe offset
need to be required as well (in comparison to run
)?
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.
Even better, I see run
is being used only internally and we explicitly pass offset: 0
, so maybe we can just make offset
required on whole SearchExecutionOptions
?
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 SearchExecutionOptions
are also used on the SearchModeResponse.next
method, so I don't want to make offset
required there. I've removed the = {}
default value though.
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.
isn't the whole point of next
that offset should be passed? Otherwise it'll return the first page again (returned on initial search)?
Just think that users might think next
is smart and will magically return next page somehow and don't pass offset
if we don't require it.
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.
Just think that users might think next is smart and will magically return next page somehow and don't pass offset if we don't require it.
Yeah, that's true.. I've updated the SearchExecutionOptions
so that offset
is required (and left limit
optional). That's not consistent with what's in the Python version, but we can discuss whether that should be changed 👍
|
||
return { | ||
properties: object.properties, | ||
metadata: metadata, |
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.
nitpick can omit variable when key has the same name. metadata,
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.
Nice catch - fixed.
This adds the new search-only mode to the Query Agent client:
search
method toQueryAgent
to retrieve the first page of search resultsnext()
method on the response is used to paginate the next sets of results consistently (i.e., same results set)