-
Notifications
You must be signed in to change notification settings - Fork 7
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
Initial F.A.R.R related implementations #106
base: KG-v2.0
Are you sure you want to change the base?
Conversation
* minor stuff * fix some ciruclar dep issues * restructuring of logging system * lil more resturcturing
/** | ||
* @internal | ||
*/ | ||
export const cloneFLC = <R, RC extends FindLikeCursor, Opts extends GenericFindOptions>(cursor: FindLikeCursor, filter: SerializedFilter, options: Opts, mapping?: (doc: SomeDoc) => R): RC => { |
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.
Why is this a separate helper function instead of just a clone()
method on AbstractCursor?
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.
Needed a clone function which could override specific cursor attributes anyways, so the no-args .clone()
wouldn't do in that situation anywasy
Also, an external helper function has the extra bonus of not encoding specific cursor logic in AbstractCursor
(since its parameters depends on the specific cursor's implementation)
- Though I could see it being argued that this is unnecessary since there's no non-find-like cursors anyways
limit: this._options.limit, | ||
hybridLimits: this._options.hybridLimits, | ||
rerankOn: this._options.rerankOn, | ||
includeScores: true, |
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.
includeScores should also probably be configurable
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.
It will be 👍
When includeScores
is false, then rerankedResult.scores
will just be an empty record.
/** | ||
* @internal | ||
*/ | ||
private async* _iterator(method: string, tm?: TimeoutManager): AsyncGenerator<T, void, void> { |
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.
_iterator
doesn't seem to be necessary, you can easily replace it with a throwIfClosed()
method and _next()
in toArray()
and forEach()
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.
True but _iterator
also has the try-finally
which closes the cursor after iteration is done as well
May as well just abstract it into a helper function at that point 🤷
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.
Had a couple of minor comments. Also no tests for findAndRerank yet?
No I have some basic FARR cursor unit tests though here which mostly just tests builder methods & other such properties of the cursor |
No description provided.