feat(firestore): Add Query#limit_to_last #7575
Conversation
google-cla
bot
added
the
cla: yes
| new_start_at = new_query.end_at.dup | ||
| new_end_at = new_query.start_at.dup | ||
| new_query.start_at = new_start_at | ||
| new_query.end_at = new_end_at |
There was a problem hiding this comment.
You also need to switch the "before" direction.
|
|
||
| _(snps.count).must_equal 2 | ||
| _(snps[0].count).must_equal 2 | ||
| _(snps[0].docs.map(&:document_id)).must_equal ["int 3", "int 2"] |
There was a problem hiding this comment.
Can we make sure this test verifies that the order is "int 2", "int 3"?
There was a problem hiding this comment.
Thanks for catching that, will do. I'll also change line 65 below to ["int 3", "int 4"]
There was a problem hiding this comment.
@schmidt-sebastian Can you see the mistake I made in the implementation that causes this result? This is an integration test that currently passes.
There was a problem hiding this comment.
I can take a look at the code if you need me to, but I haven't yet. My strong suspicion is that you are using the flipped query but not re-flipping the results (you always need to reverse the results if you are flipping the orderBy constraints on the user's behalf). The way I do this is Node and Java is that I use the pre-flipped query to process the local results.
There was a problem hiding this comment.
That sounds likely, thank you.
There was a problem hiding this comment.
@schmidt-sebastian I updated Inventory to reverse the results for limit_to_last queries. I'm sorry I missed that the first time. Please take a look at the additional assertions for changes ordering to verify that it is correct. (I'm not sure if changes ordering should reflect query orderBy.)
There was a problem hiding this comment.
This looks correct now. It is slightly different than what I have implemented since the Watch client in Ruby uses the reversed query Proto for its ordering, rather than the user-specified Query order. In the end this does not matter though, since we only need to match the user's order by clause. With the reversal this is now working as intended.
There was a problem hiding this comment.
@schmidt-sebastian Thank you for your help on this PR! I'm going to add samples for this feature (as in GoogleCloudPlatform/python-docs-samples#4196), then ask @dazuma to review for Ruby concerns.
There was a problem hiding this comment.
Here are the 3 samples PRs I found:
Go PR: GoogleCloudPlatform/golang-samples#1599
Python PR: GoogleCloudPlatform/python-docs-samples#4196
Node PR: googleapis/nodejs-firestore#1113
There was a problem hiding this comment.
Added sample in 13c2253
quartzmo
force-pushed
the
firestore-limit-to-last
branch
from
5b17b00
to
702ea24
Compare
|
Rebased on latest master to get |
* Support updating limit_to_last limit value without reflipping cursors
quartzmo
force-pushed
the
firestore-limit-to-last
branch
2 times, most recently
from
9dd8dd9
to
13c2253
Compare
quartzmo
force-pushed
the
firestore-limit-to-last
branch
from
13c2253
to
e3a45fc
Compare
closes: #7572
closes: #7594
/cc @schmidt-sebastian