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

Support DocumentSnapshot cursors #56

Closed
schmidt-sebastian opened this issue Jun 4, 2020 · 6 comments
Closed

Support DocumentSnapshot cursors #56

schmidt-sebastian opened this issue Jun 4, 2020 · 6 comments
Assignees
Labels
api: firestore Issues related to the googleapis/python-firestore API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@schmidt-sebastian
Copy link

The Python client should add support for DocumentSnapshot-based cursors.

See https://github.com/googleapis/java-firestore/blob/master/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java#L853 or https://github.com/googleapis/nodejs-firestore/blob/master/dev/src/reference.ts#L1663

@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/python-firestore API. label Jun 4, 2020
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jun 4, 2020
@IlyaFaer IlyaFaer added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed triage me I really want to be triaged. labels Jun 5, 2020
@HemangChothani HemangChothani self-assigned this Jun 5, 2020
@HemangChothani
Copy link
Contributor

@schmidt-sebastian As per my research i think python supports DocumentSnapshot-based cursors, but the confusion is just because of naming convention, will update the name of the parameter so it could minimize the confusion.Let me know if i misunderstood anything or missing something.

def start_at(self, document_fields):

Args:
document_fields
(Union[:class:`~google.cloud.firestore_v1.document.DocumentSnapshot`, dict, list, tuple]):
a document snapshot or a dictionary/list/tuple of fields
representing a query results cursor. A cursor is a collection
of values that represent a position in a query result set.

@schmidt-sebastian
Copy link
Author

@HemangChothani Sorry, I missed this completely. Would it be possible to rename document_fields? That name caught my eyes and I immediately thought Python does not support these cursors.

The reason I looked at this was to figure out whether the Python SDK considers "IN" filters to be equality cursors. I am not able to decipher the code to provide an answer (

if _has_snapshot_cursor:
). Only the operations GREATER_THAN, GREATER_THAN_OR_EQUALS, LESS_THAN or LESS_THAN_OR_EQUALS should be used to compute the implicit cursor. See also the fix here: googleapis/java-firestore#216. Do we need to port this fix?

@HemangChothani
Copy link
Contributor

Would it be possible to rename document_fields?

@schmidt-sebastian Yes, I have opened a PR #58

Do we need to port this fix?

I think "IN" filter is not considered as a equality cursors. If i compare the python code it is similar to the old java code, but still need a conformation.

This line is not going to execute for == and array_contains operator.

if filter_.op in should_order and field not in order_keys:
orders.append(self._make_order(field, "ASCENDING"))

For better view i think need to concern @crwilcox

@HemangChothani
Copy link
Contributor

@crwilcox Any suggestion on Python SDK considers "IN" filters to be equality cursors?

@HemangChothani
Copy link
Contributor

@schmidt-sebastian I thinks this issue is resolved and only the operations GREATER_THAN, GREATER_THAN_OR_EQUALS, LESS_THAN or LESS_THAN_OR_EQUALS should be used to compute the implicit cursor.
As we can see here:

if _has_snapshot_cursor:
should_order = [
_enum_from_op_string(key)
for key in _COMPARISON_OPERATORS
if key not in (_EQ_OP, "array_contains")
]
order_keys = [order.field.field_path for order in orders]
for filter_ in self._field_filters:

so closing this issue for now, feel free to reopen if it doesn't fulfill the requirements.

@schmidt-sebastian
Copy link
Author

Sounds good. Thanks for clarifying and sorry for the confusion.

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. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

4 participants