-
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
| 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:
searchmethod toQueryAgentto 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)