feat(firestore): implement limitToLast #2420

IlyaFaer · 2020-06-10T09:17:24Z

Closes #2419

Change-Id: Iac088306a1cc846bc27ab43aae0761c6d5350de1

Change-Id: Ib9d54c0f80af0368f8a4bd8ab77228e05cdd2bcf

Change-Id: Ib94fa125d7f3ec4bf1667228cc4ceccf726c3923

firestore/integration_test.go

firestore/query.go

BenWhitehead

@tritone or @codyoss Can either of you review this for the go implementation.

A query with limitToLast(n) defined on it, should:

invert any ordering defined on the query
Send the query to the backend
Read the n results into memory
Return a query result to the user with the results read back in 3 reversed

Due to the fact that this feature is dependency on buffering to memory, streaming results are not supported.

firestore/query.go

codyoss · 2020-06-15T14:10:54Z

firestore/query.go

@@ -569,8 +579,48 @@ func trunc32(i int) int32 {
 	return int32(i)
 }

+// Get returns an array of query's resulting documents.
+func (q Query) Get(ctx context.Context) ([]*DocumentSnapshot, error) {


To be consistent with Query.Documents , I wonder if this should be called GetAll

I thought about that. The thing is that in other languages it's called just get(). And GetAll() 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 about Query. Anyway, I'm just explaining, if you think it should be renamed, I'll rename.

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.

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

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 to n 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.

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.

NP. As you mentioned before, I've added an unexported Query field into DocumentIterator and moved all the implementation into DocumentIterator.GetAll(). Seems to me we're no more in need of an example (as there are no new methods added), and a separate test.

codyoss · 2020-06-15T14:15:30Z

firestore/query.go

+
+		q.limitToLast = false
+	}
+	docs, err := q.Documents(ctx).GetAll()


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 to Documents.Next.

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.

firestore/query.go

Change-Id: I7f8d59c20623d56a700c356e5c72e342e4f8a616

Change-Id: I8ecf940714704f30b7bc06181e466e84e1292495

IlyaFaer · 2020-06-29T08:41:06Z

Friendly ping

codyoss · 2020-06-29T13:09:11Z

I'll defer to @tritone for the rest of the review. I think I mentioned everything that I saw.

tritone

Apologies for the delay. I have a few comments and questions about the surface design here-- would appreciate thoughts.

firestore/query.go

tritone · 2020-06-30T01:07:13Z

firestore/query.go

+	return q
+}
+
+// LimitToLast returns a new Query that specifies the maximum number of last


Will this the meaning of this first / last results distinction be obvious to users of Firestore? @BenWhitehead

tritone · 2020-06-30T01:22:11Z

firestore/query.go

@@ -569,8 +579,48 @@ func trunc32(i int) int32 {
 	return int32(i)
 }

+// Get returns an array of query's resulting documents.
+func (q Query) Get(ctx context.Context) ([]*DocumentSnapshot, error) {


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.

tritone · 2020-06-30T01:37:08Z

firestore/query.go

+
+		q.limitToLast = false
+	}
+	docs, err := q.Documents(ctx).GetAll()


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.

tritone · 2020-06-30T01:46:31Z

firestore/query.go

@@ -569,8 +579,48 @@ func trunc32(i int) int32 {
 	return int32(i)
 }

+// Get returns an array of query's resulting documents.
+func (q Query) Get(ctx context.Context) ([]*DocumentSnapshot, error) {


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

Change-Id: I58783a8cb39f5215e518cbe98772a07dc5c32691

Change-Id: I4427d9b1b48ec8549b1ead25e2b80d91b80fefb9

Change-Id: I0e9eaa202ed642e5aa38991b9e32a2020bcff2a5

tritone

This implementation looks a lot cleaner, thanks @IlyaFaer ! A couple remaining questions I had.

firestore/query.go

firestore/integration_test.go

firestore/query.go

tritone

LGTM. @BenWhitehead @codyoss please take a final look before I merge?

codyoss · 2020-07-21T13:45:53Z

LGTM

tritone · 2020-07-27T18:39:18Z

Got verbal sign off from @BenWhitehead , so I'm going to go ahead and merge this.

gcf-merge-on-green · 2020-07-27T21:40:03Z

Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR

Support "limit to last" feature. This allows a query which will return only the final N results. See https://firebase.google.com/docs/reference/js/firebase.database.Query#limittolast

IlyaFaer added 3 commits June 9, 2020 13:07

feat(firestore): support "limit to last" feature

692408e

Change-Id: Iac088306a1cc846bc27ab43aae0761c6d5350de1

use bool instead of int for limitToLast

8345f23

Change-Id: Ib9d54c0f80af0368f8a4bd8ab77228e05cdd2bcf

add periods into comments

247633c

Change-Id: Ib94fa125d7f3ec4bf1667228cc4ceccf726c3923

IlyaFaer added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. api: firestore Issues related to the Firestore API. labels Jun 10, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 10, 2020

IlyaFaer commented Jun 10, 2020

View reviewed changes

firestore/integration_test.go Outdated Show resolved Hide resolved

firestore/integration_test.go Outdated Show resolved Hide resolved

firestore/query.go Show resolved Hide resolved

IlyaFaer requested review from tritone, BenWhitehead and codyoss June 10, 2020 09:33

IlyaFaer marked this pull request as ready for review June 10, 2020 09:33

BenWhitehead reviewed Jun 11, 2020

View reviewed changes

codyoss reviewed Jun 15, 2020

View reviewed changes

IlyaFaer added 2 commits June 16, 2020 11:09

add "firestore" prefix into the error message

13bd01f

Change-Id: I7f8d59c20623d56a700c356e5c72e342e4f8a616

del period at the end of the error message

cc6ac5d

Change-Id: I8ecf940714704f30b7bc06181e466e84e1292495

tritone reviewed Jun 30, 2020

View reviewed changes

IlyaFaer added 6 commits June 30, 2020 10:27

Merge branch 'master' into limit_to_last

14634fe

Change-Id: I58783a8cb39f5215e518cbe98772a07dc5c32691

add an example, rename to GetAll

413dad1

Change-Id: I4427d9b1b48ec8549b1ead25e2b80d91b80fefb9

fix a results num error

b77e52b

Change-Id: I0e9eaa202ed642e5aa38991b9e32a2020bcff2a5

Merge branch 'master' into limit_to_last

4e14235

move limitToLast implementation into DocumentIterator.GetAll()

dfef166

error msg typo fix

158e44c

tritone reviewed Jul 8, 2020

View reviewed changes

firestore/query.go Outdated Show resolved Hide resolved

firestore/integration_test.go Outdated Show resolved Hide resolved

firestore/query.go Show resolved Hide resolved

use Query pointer

f84c0a6

tritone approved these changes Jul 21, 2020

View reviewed changes

IlyaFaer mentioned this pull request Jul 22, 2020

firestore: add LimitToLast samples GoogleCloudPlatform/golang-samples#1598

Closed

Merge branch 'master' into limit_to_last

d7a3bae

Merge branch 'master' into limit_to_last

e07f9cc

tritone added the automerge Merge the pull request once unit tests and other checks pass. label Jul 27, 2020

tritone merged commit 29aebdf into googleapis:master Jul 27, 2020

IlyaFaer deleted the limit_to_last branch September 29, 2020 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(firestore): implement limitToLast #2420

feat(firestore): implement limitToLast #2420

IlyaFaer commented Jun 10, 2020

BenWhitehead left a comment

codyoss Jun 15, 2020

IlyaFaer Jun 16, 2020

tritone Jun 30, 2020

tritone Jun 30, 2020

BenWhitehead Jun 30, 2020

tritone Jul 7, 2020

IlyaFaer Jul 8, 2020

codyoss Jun 15, 2020

tritone Jun 30, 2020

IlyaFaer commented Jun 29, 2020

codyoss commented Jun 29, 2020

tritone left a comment

tritone Jun 30, 2020

tritone Jun 30, 2020

tritone Jun 30, 2020

tritone Jun 30, 2020

tritone left a comment

tritone left a comment

codyoss commented Jul 21, 2020

tritone commented Jul 27, 2020

gcf-merge-on-green bot commented Jul 27, 2020

feat(firestore): implement limitToLast #2420

feat(firestore): implement limitToLast #2420

Conversation

IlyaFaer commented Jun 10, 2020

BenWhitehead left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IlyaFaer commented Jun 29, 2020

codyoss commented Jun 29, 2020

tritone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tritone left a comment

Choose a reason for hiding this comment

tritone left a comment

Choose a reason for hiding this comment

codyoss commented Jul 21, 2020

tritone commented Jul 27, 2020

gcf-merge-on-green bot commented Jul 27, 2020