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

Removing cursor and more results from query object. #432

Merged
merged 2 commits into from
Dec 19, 2014

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Dec 17, 2014

This is a follow up to a discussion in #423 and #425.

Also

  • Adding fetch_page() method to return the needed properties
    no longer available on the Query object.
  • Made fetch_page() fail if more_results is NOT_FINISHED, which
    seems to be intended for batched requests.
  • Added start_cursor and end_cursor properties to Query.
  • Updated docstrings to reflect change
  • Updated tests to ensure the returned cursor was always set.
  • Streamlined fetch()/fetch_page() tests to run via a single
    tweakable helper.
  • Updated a datastore.connection test to construct and return an
    initialized QueryResultBatch to simulate a valid cursor.
  • Updated regression.clear_datastore and regression.datastore to
    reflect changed API surface.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 17, 2014

@pcostell I'm unclear when NOT_FINISHED would ever occur but the note in the .proto spec:

  enum MoreResultsType {
    NOT_FINISHED = 1;  // There are additional batches to fetch from this query.

seems to indicate it occurs during batching, which is different than a standard fetch IIUC.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 2d40365 on dhermes:remove-cursor-from-query into c2ba130 on GoogleCloudPlatform:master.

@@ -239,8 +238,8 @@ def kind(self, *kinds):
:type kinds: string
:param kinds: The entity kinds for which to query.

:rtype: string or :class:`Query`
:returns: If no arguments, returns the kind.
:rtype: string, list of strings, or :class:`Query`

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@tseaver
Copy link
Contributor

tseaver commented Dec 18, 2014

I wonder if the Query API would be clearer / easier to use if we separated the bits which define the query from the bits which manage the state of a given result into separate instances. In this notion, Query.fetch() would return a Cursor instance, whose protobuf would be populated based on the query's own attributes, and whose start_cursor/end_cursor would be unpopulated until values were fetched from the back-end. The app would then do the paging / batching operations to get entities using the cursor instance, which would know whether more results were expected, etc.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 18, 2014

I have a few thoughts:

  • It could be very convenient to have a place to store the QueryBatchResult data
  • In reasonable-sized applications, cursors are somewhat shortlived or local
  • As a low-level library, data duplication and more classes may be a bad thing (especially if we try to build an ORM on top)
  • Having the cursor perform paging also makes multiple methods / classes for the same purpose, which I'm a little uneasy about

I think overall the added complexity / maintenance may not be worth it, but I am biased based on ndb's implementation.

@tseaver
Copy link
Contributor

tseaver commented Dec 18, 2014

In larger applications, I envision a set of Query objects which are pre-built, something like "prepared statements" in SQL-based apps. Application code would reuse them over and over without needing to re-construct them on the fly.

In the current setup, the application code would have to do that by cloning the queries (even though they weren't going to be modifying the filters). It would be nice if the short-lived / stateful concerns weren't intertwined with the long-lived, stateless ones.

@pcostell
Copy link
Contributor

@dhermes Here's a rundown on all the types of more_results:

NO_MORE_RESULTS: This means that the query successfully returned all results for the query filters. Right now, this is not returned due to some constraints in the backend.

MORE_RESULTS_AFTER_LIMIT: We've successfully returned all the requested results. There may be additional results after the limit (once the backend supports NO_MORE_RESULTS, this result type would indicate there are definitely results after the limit). The distinction between this type and NO_MORE_RESULTS will make it easier to support client-side paging. MORE_RESULTS_AFTER_LIMIT would indicate that you should display a "next page" button, NO_MORE_RESULTS means you shouldn't.

NOT_FINISHED: You've hit some server-side constraint, either message size or a deadline. Because of this, we've returned less results than you asked for, however they may or may not exist (you set a limit of 5 and I returned 4).

@dhermes
Copy link
Contributor Author

dhermes commented Dec 18, 2014

@tseaver Can we file a bug and flesh out what you mean more. I can't quite envision what the limitation / new feature is.

@pcostell So we should treat both MORE_RESULTS_AFTER_LIMIT and NOT_FINISHED as

more_results = True

or you think having all three is necessary? I'm worried that having an enum instead of a boolean will confuse / turn away some users, but I'm also worried that sophisticated users may want the added distinction between the two "unfinished" states.

@pcostell
Copy link
Contributor

@dhermes I think MORE_RESULTS_AFTER_LIMIT and NO_MORE_RESULTS should be more_results = False. It means that the user doesn't need to do any more queries to get the information they asked for (if they asked for 5 and got 5, MORE_RESULTS_AFTER_LIMIT would be true).

That way, more_results = True implies that they must do another query to get what they asked for.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 18, 2014

Thanks @pcostell!

This is a follow up to a discussion in googleapis#423 and googleapis#425.

Also
- Adding fetch_page() method to return the needed properties
  no longer available on the Query object.
- Made fetch_page() fail if more_results is `NOT_FINISHED`, which
  seems to be intended for batched requests.
- Added start_cursor and end_cursor properties to Query.
- Updated docstrings to reflect change
- Updated tests to ensure the returned cursor was always set.
- Streamlined fetch()/fetch_page() tests to run via a single
  tweakable helper.
- Updated a datastore.connection test to construct and return an
  initialized QueryResultBatch to simulate a valid cursor.
- Updated regression.clear_datastore and regression.datastore to
  reflect changed API surface.
Incorporates feedback from @pcostell. In particular, treats
`NOT_FINISHED` as the only enum that needs `more_results=True`
and both `NO_MORE_RESULTS` and `MORE_RESULTS_AFTER_LIMIT` as
responses which don't need paging.
@dhermes
Copy link
Contributor Author

dhermes commented Dec 18, 2014

@tseaver I incorporated feedback in 1c1333e, PTAL.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1c1333e on dhermes:remove-cursor-from-query into 1f2ed88 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Dec 19, 2014

I think we can merge even with my last question un-answered.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 19, 2014

OK cool, LGTY then it LGTM :)

dhermes added a commit that referenced this pull request Dec 19, 2014
Removing cursor and more results from query object.
@dhermes dhermes merged commit 89ded75 into googleapis:master Dec 19, 2014
@dhermes dhermes deleted the remove-cursor-from-query branch December 19, 2014 01:00
@dhermes dhermes added the api: datastore Issues related to the Datastore API. label Dec 31, 2015
atulep pushed a commit that referenced this pull request Apr 3, 2023
Source-Link: googleapis/synthtool@c4dd595
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:ce3c1686bc81145c81dd269bd12c4025c6b275b22d14641358827334fddb1d72
atulep pushed a commit that referenced this pull request Apr 6, 2023
Source-Link: googleapis/synthtool@c4dd595
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:ce3c1686bc81145c81dd269bd12c4025c6b275b22d14641358827334fddb1d72
atulep pushed a commit that referenced this pull request Apr 6, 2023
Source-Link: googleapis/synthtool@c4dd595
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:ce3c1686bc81145c81dd269bd12c4025c6b275b22d14641358827334fddb1d72
atulep pushed a commit that referenced this pull request Apr 18, 2023
Source-Link: googleapis/synthtool@c4dd595
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:ce3c1686bc81145c81dd269bd12c4025c6b275b22d14641358827334fddb1d72
parthea pushed a commit that referenced this pull request Jun 4, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
parthea added a commit that referenced this pull request Aug 15, 2023
* chore(deps): update all dependencies

* revert

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Anthonios Partheniou <partheniou@google.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this pull request Sep 22, 2023
Source-Link: https://github.com/googleapis/synthtool/commit/26c7505b2f76981ec1707b851e1595c8c06e90fc
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:f946c75373c2b0040e8e318c5e85d0cf46bc6e61d0a01f3ef94d8de974ac6790
parthea pushed a commit that referenced this pull request Sep 22, 2023
Source-Link: googleapis/synthtool@fdba3ed
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:1f0dbd02745fb7cf255563dab5968345989308544e52b7f460deadd5e78e63b0
parthea pushed a commit that referenced this pull request Sep 22, 2023
Source-Link: https://github.com/googleapis/synthtool/commit/cb960373d12d20f8dc38beee2bf884d49627165e
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:2d816f26f728ac8b24248741e7d4c461c09764ef9f7be3684d557c9632e46dbd
parthea added a commit that referenced this pull request Oct 21, 2023
* fix(deps): require protobuf <4.0.0dev

* update constraints
parthea pushed a commit that referenced this pull request Oct 21, 2023
Source-Link: googleapis/synthtool@4826337
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:60a63eddf86c87395b4bb394fdddfe30f84a7726ee8fe0b758ea132c2106ac75

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this pull request Oct 22, 2023
Source-Link: googleapis/synthtool@c4dd595
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:ce3c1686bc81145c81dd269bd12c4025c6b275b22d14641358827334fddb1d72
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants