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(firestore): implement limitToLast #2420
feat(firestore): implement limitToLast #2420
Changes from 5 commits
692408e
8345f23
247633c
13bd01f
cc6ac5d
14634fe
413dad1
b77e52b
4e14235
dfef166
158e44c
f84c0a6
d7a3bae
e07f9cc
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.
Will this the meaning of this first / last results distinction be obvious to users of Firestore? @BenWhitehead
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.
To be consistent with Query.Documents , I wonder if this should be called
GetAll
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 thought about that. The thing is that in other languages it's called just
get()
. AndGetAll()
is a method of an iterator, so it gives all the docs once instead of streaming/iterating them - that makes sense for an iterator, but I'm not sure aboutQuery
. Anyway, I'm just explaining, if you think it should be renamed, I'll rename.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.
Looks like pretty much all other get methods in cloud.google.com/go/firestore which return a slice are called GetAll, whereas Get() is used for a single Transaction or DocumentRef. From that perspective I think GetAll makes more sense.
Also, I feel like the fact that this method is being added to the surface for the library is a bigger change than what's implied by the title of the PR. Could you change the title to reflect this? I also think it would be good to add an example of how this would be used to examples_test.go for the docs.
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.
One more thing-- looking at this a little more closely I see that the following are now equivalent for a non-limitToLast query
q
:q.Get()
q.Documents().GetAll()
Do we have an opinion about which is preferred?
And really, is it necessary to have this duplication? (I guess the answer is yes with the current design because DocumentIterator doesn't have access to the Query fields. But in theory we could just add an unexported field there and pass it along I think-- this would mean that to do a limitToLast query you would also call
q.Documents().GetAll()
, though it would mean that other DocumentIterator methods would then need to error if limitToLast were set to true).Would be curious on your thoughts @BenWhitehead @codyoss
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.
If there isn't a semantic difference to how those methods execute, I suspect it's fine to have a single method. And if there is a uniform way for users to keep doing what they're used to doing to read results it's probably better to stick with that, than it is to try and have this implementation be uniform with the other languages.
The complexity with
limitToLast
is that it has to buffer the up ton
results and flip the order in memory before return to the application. So, as long as that property can be ensured whichever way is fine with me.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.
Discussed this with @codyoss offline, he also thinks that removing Query.Get and enabling this feature through the existing Query.DocumentIterator.GetAll method instead seems preferable. @IlyaFaer could you modify this change to do that? Feel free to reach out if there are questions that come up.
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.
NP. As you mentioned before, I've added an unexported
Query
field intoDocumentIterator
and moved all the implementation intoDocumentIterator.GetAll()
. Seems to me we're no more in need of an example (as there are no new methods added), and a separate test.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 all this logic just happen within this method? @tritone Thoughts?
If we did this the err returned from calling
Documents()
could instead be used toDocuments.Next
.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.
If I'm reading this correctly-- we will have flipped q.limitToLast to false before calling q.Documents, so there shouldn't be an error from q.Documents unless one is set by newQueryDocumentIterator, correct? It would just be equivalent to having L624-626 up here. Seems clearer in some ways to decouple these methods but I'm not super opinionated on this.