Skip to content
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

Firestore limit_to_last(n) returns the first n elements in a reverse order instead of returning the last n elements #536

Closed
brenhr opened this issue Feb 18, 2022 · 4 comments · Fixed by #692 or #691
Assignees
Labels
api: firestore Issues related to the googleapis/python-firestore API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@brenhr
Copy link

brenhr commented Feb 18, 2022

Describe your environment

  • SDK version: 2.3.4
  • Python version: 3.10.2
  • Pip version: 21.2.4

Describe the problem

As per the documentation for limit_to_last clause, it should return the last n elements that match the query. However, when executing a simple query (i.e. db.collection("numbers").order_by("number").limit_to_last(5)) it returns the first 5 elements but in a reverse order.

Steps to reproduce

Create a collection and add data (in my case, I created a "numbers" collection, and inserted a total of 50 documents (from number 0 to 49) as follows:

for i in range(50):
    db.collection('numbers').document(f'{i}').set({
        'number': i
    })

And then, execute the following query and print the results:

lastDocs = db.collection('numbers').order_by("number").limit_to_last(5).get()
print('Last docs [using limit_to_last() clause]')

for doc in lastDocs:
    print(f'{doc.to_dict()}')
Expected output:
Last docs [using limit_to_last() clause]
{'number': 45}
{'number': 46}
{'number': 47}
{'number': 48}
{'number': 49}
Real output:

Screen Shot 2022-02-05 at 19 32 46

In case you find it useful, I also tried to reproduce this behavior in the Node.js Admin SDK, and I obtained the desired output (I mean, limitToLast() works perfectly fine):
Node.js code:

var numbersRef = db.collection("numbers");

console.log("Last documents [using limitToLast()]:");

numbersRef.orderBy("number").limitToLast(5).get().then((snapshot) => {
  snapshot.docs.forEach(doc => {
    console.log(doc.data());
  })
});

Node.js output:
Screen Shot 2022-02-05 at 19 31 26

Thanks.

@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/python-firestore API. label Feb 18, 2022
@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Feb 19, 2022
@kolea2 kolea2 added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Mar 3, 2022
@yoshi-automation yoshi-automation removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Mar 3, 2022
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Jun 2, 2022
@javibookline
Copy link

I have the same issue

@dconeybe
Copy link

dconeybe commented Aug 3, 2022

The limit-to-last implementation in python-firestore is broken when startAt, endAt, startAfter, or endBefore is specified. The reason is that the implementation must flip these "cursors", in addition to flipping the ordering, and the implementation only flips the ordering:

if self._limit_to_last:
# In order to fetch up to `self._limit` results from the end of the
# query flip the defined ordering on the query to start from the
# end, retrieving up to `self._limit` results from the backend.
for order in self._orders:
order.direction = _enum_from_direction(
self.DESCENDING
if order.direction == self.ASCENDING
else self.ASCENDING
)
self._limit_to_last = False

Here is the example of the correct implementation from the nodejs-firestore SDK:

https://github.com/googleapis/nodejs-firestore/blob/a67a12470c2238e6c03f9613de147c525eb7cae5/dev/src/reference.ts#L2147-L2159

@dconeybe
Copy link

dconeybe commented Aug 3, 2022

The good news is that there is a relatively-easy workaround. Limit-to-last queries are implemented entirely in the client, and do not need any special server support. This means that you can implement limit-to-last yourself to work around this bug.

Here is an example. The code below is all pseudocode for simplicity, not real Python code.

Suppose you have these documents in your database:

  • doc1 age=1
  • doc2 age=2
  • doc3 age=3
  • doc4 age=4
  • doc5 age=5
  • doc6 age=6

Suppose you wanted to do limitToLast(3).orderBy("age", "ASC").endBefore(doc5), which would produce the result [doc2, doc3, doc4].

You could instead run this query: limit(3).orderBy("age", "DESC").startAfter(doc5). This query would produce the result [doc4, doc3, doc2] so the final step would be to reverse the order of the result set, to [doc2, doc3, doc4].

Mariatta added a commit that referenced this issue Feb 18, 2023
When limit_to_last was set, we need to reverse the order. However due to error in comparing the order direction, it was not properly set.

comparing `order.direction == self.ASCENDING` is always `False` because there are two different types.

The correct way is by comparing `order.direction.name == self.ASCENDING`

Fixes #536
@Mariatta
Copy link
Contributor

Mariatta commented Feb 18, 2023

The bug is due to this line

order.direction = _enum_from_direction(

The test order.direction == self.ASCENDING is not correct, and always returns False, therefore the order was never properly updated.
order.direction is an enum whereas self.ASCENDING is a type str.

I've just opened a PR to fix it #692

engelke pushed a commit that referenced this issue Feb 21, 2023
When limit_to_last was set, we need to reverse the order. However due to error in comparing the order direction, it was not properly set.

comparing `order.direction == self.ASCENDING` is always `False` because there are two different types.

The correct way is by comparing `order.direction.name == self.ASCENDING`

Fixes #536
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/python-firestore API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
6 participants