Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add support for vector search with Query#findNearest #1827
feat: Add support for vector search with Query#findNearest #1827
Changes from 5 commits
a971555
44d364d
8fe28ce
151f245
c2da4a8
dff55dc
a29c6b5
96a3940
b391efb
7bd43aa
58756e0
264e103
bb83ee7
31e9be2
1f61461
f29120c
87fcff4
5fa33f5
1fe96df
c606e57
7cfab37
84a8f8b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Seems to me that
documentSet
is just converted todocuments
.I suggest you remove parameter, and just have subclasses use
documents
parameter instead.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.
Also pre-existing logic, so I'm not certain of the reasoning, but there are several instances of lazy-loading and memoization in this code. My suspicion is that it's related to performance. I don't have strong feelings on this either way. What are your thoughts with that context?
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 of slightly greater concern because
documentSet
is also omitted fromequals
method.I prefer this get fixed.
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 changed this.
I don't think there was an issue before. The value of
documentSet
was tested for equality because it's value was converted by thegetDocuments()
method.However, I think the new approach is equally fine. The benefits of lazily converting
documentSet
to a list of document snapshots are rarely if ever seen.