Reducing limit over the life of a datastore query iterator. by dhermes · Pull Request #1787 · googleapis/google-cloud-python

dhermes · 2016-05-11T18:22:26Z

dhermes · 2016-05-11T18:22:35Z

Sorry for the delay @pcostell!

Sign in to view

 break
+ if self._limit is not None:
+ self._limit -= len(self._page)
 self.next_page()


dhermes · 2016-05-12T23:52:17Z

@pcostell PTAL, I also added logic for the offset

Sign in to view

+ if self._limit is not None:
+ self._limit -= num_results
+ if self._offset is not None:
+ self._offset += num_results


dhermes · 2016-05-13T00:31:39Z

@pcostell You just mentioned that offset shouldn't be used to continue a query. You mean offset on it's own? Like without the end cursor?

Sign in to view

+ if self._offset is not None:
+ # NOTE: The offset goes down relative to the location
+ # because we are updating the cursor each time.
+ self._offset -= num_results


pcostell · 2016-05-13T00:35:40Z

Sorry: When a query is continued, it may contain an offset. But like the limit the offset should always be decreasing. What I really mean:

Always use a cursor to step to the next batch of a query, never an offset.

Sometimes you need to go through batches of offset, in which you'll issue multiple queries with decreasing offsets. This is ok.

dhermes · 2016-05-17T17:13:45Z

@pcostell PTAL.

Sign in to view

@@ -382,7 +381,7 @@ class Iterator(object):
 _query_pb2.QueryResultBatch.MORE_RESULTS_AFTER_LIMIT,


dhermes · 2016-05-17T17:56:56Z

@tseaver I've got the LGTM from @pcostell. Can you give this a final pass?

Sign in to view

 self.assertTrue(iterator._query is query)
 self.assertEqual(iterator._limit, None)
- self.assertEqual(iterator._offset, 0)
+ self.assertEqual(iterator._offset, None)


tseaver · 2016-05-17T20:56:38Z

Tests for next_page should assert correct values for self._skipped_results.

dhermes · 2016-05-17T21:51:00Z

@tseaver Good call. Check out the latest commit.

tseaver · 2016-05-17T23:43:16Z

LGTM

In the process, also keeping tracking of the number of skipped results (so that we can update the offset). Fixesgoogleapis#1763.

dhermes · 2016-05-17T23:52:08Z

Squashed commits. Will merge once Travis passes.

dhermes assigned tseaver May 11, 2016

dhermes added the api: datastore Issues related to the Datastore API. label May 11, 2016

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 11, 2016

pcostell reviewed May 11, 2016
View reviewed changes

Comment threadgcloud/datastore/query.py Outdated
break
if self._limit is not None:
self._limit -= len(self._page)
self.next_page()
This comment was marked as spam.
Sign in to view

dhermes force-pushed the fix-1763 branch from 1ad2799 to 49d9503Compare May 12, 2016 23:51

pcostell reviewed May 13, 2016
View reviewed changes

Comment threadgcloud/datastore/query.py Outdated
if self._offset is not None:
# NOTE: The offset goes down relative to the location
# because we are updating the cursor each time.
self._offset -= num_results
This comment was marked as spam.
Sign in to view

dhermes force-pushed the fix-1763 branch from 4d853dc to 3c0b3e1Compare May 17, 2016 17:13

pcostell reviewed May 17, 2016
View reviewed changes

Comment threadgcloud/datastore/query.py
@@ -382,7 +381,7 @@ class Iterator(object):
_query_pb2.QueryResultBatch.MORE_RESULTS_AFTER_LIMIT,
This comment was marked as spam.
Sign in to view
This comment was marked as spam.
Sign in to view

tseaver reviewed May 17, 2016
View reviewed changes

Comment threadgcloud/datastore/test_query.py
self.assertTrue(iterator._query is query)
self.assertEqual(iterator._limit, None)
self.assertEqual(iterator._offset, 0)
self.assertEqual(iterator._offset, None)
This comment was marked as spam.
Sign in to view

dhermes force-pushed the fix-1763 branch from 5c389c9 to a7332ddCompare May 17, 2016 21:50

dhermes force-pushed the fix-1763 branch from a7332dd to a03a95eCompare May 17, 2016 23:49

Reducing limit and offset over the life of a datastore query iterator.
74026f4
In the process, also keeping tracking of the number of skipped results (so that we can update the offset). Fixesgoogleapis#1763.

dhermes force-pushed the fix-1763 branch from a03a95e to 74026f4Compare May 17, 2016 23:51

dhermes merged commit d956136 into googleapis:masterMay 18, 2016

		@@ -382,7 +381,7 @@ class Iterator(object):
		_query_pb2.QueryResultBatch.MORE_RESULTS_AFTER_LIMIT,

Uh oh!

Conversation

dhermes commented May 11, 2016

Uh oh!

dhermes commented May 11, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

dhermes commented May 12, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

dhermes commented May 13, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

pcostell commented May 13, 2016

Uh oh!

dhermes commented May 17, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

dhermes commented May 17, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

tseaver commented May 17, 2016

Uh oh!

dhermes commented May 17, 2016

Uh oh!

tseaver commented May 17, 2016

Uh oh!

dhermes commented May 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants