datastore: enable to set query batch size #90

delphinus · 2017-09-24T14:10:22Z

This adds q.BatchSize() method to avoid the error that claims too many
datastore.next() calls due to the lack of specifying the count
property (= batch size).

Fixes #88

This adds q.BatchSize() method to avoid the error that claims too many datastore.next() calls due to the lack of specifying the `count` property (= batch size). Fixes golang#88

googlebot · 2017-09-24T14:10:27Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

delphinus · 2017-09-24T14:18:59Z

I signed it!

googlebot · 2017-09-24T14:19:01Z

CLAs look good, thanks!

delphinus · 2017-10-26T10:32:54Z

Any response? Can anyone review this??

s-mang · 2017-10-26T17:28:09Z

Hey, thanks for the PR. Can you add some tests?

delphinus · 2017-10-27T05:52:13Z

@adams-sarah Thanks for reviewing.

I have added one test case for checking the created proto to include Count with a valid value if specified the BatchSize method in the Query. Should I add more tests?

s-mang · 2017-10-27T17:41:08Z

Oh my apologies I missed that.

s-mang · 2017-11-03T17:59:11Z

hey sorry it's taking me so long to review this. want to make sure we don't mess this up.
i'll try to get to this soon!

trakhimenok · 2018-02-09T11:10:24Z

datastore/query.go

+// at once. This value should be equal to or less than the Limit.
+func (q *Query) BatchSize(size int) *Query {
+	q = q.clone()
+	if size < math.MinInt32 || size > math.MaxInt32 {


Should the check be if size < 0 || size > math.MaxInt32?

Sorry for late reply. Yes, it seems the negative values has no means. I rewrite this.

mtraver · 2018-04-16T20:22:55Z

datastore/query.go

@@ -536,9 +553,13 @@ func (q *Query) Run(c context.Context) *Iterator {
 		return t
 	}
 	offset := q.offset - t.res.GetSkippedResults()
+	count := t.limit
+	if t.count > 0 && (count == 0 || t.count < count) {


I understand the intent of this as, "If count is set, and either the query is unlimited or the count is under the limit, use the count." Correct me if that's wrong.

Based on the above, the condition should be t.count > 0 && (count < 0 || t.count < count), right? The limit can be less than 0 (comment for limit field in Iterator says A negative value means unlimited., and NewQuery defaults the query's limit to -1). If count is positive and limit is negative we want to use the count.

I think assigning t.limit to local var count and then checking for it is a bit confusing here.

Would not it be better to simply use t.limit in if?

var count int if t.count > 0 && (t.limit < 0 || t.count < t.limit) { count = t.count } else { count = t.limit }

@mtraver will appreciate if you can point out what is purpose allowing to pass 0 as count. It may result in unnecessary datastore request that returns nothing. Wouldn't it be more logical to to treat 0s as unlimited in which case to not pass it over wire in *pb.Query and if a negative number passed to Limit() or Count() either return error or assign 0 internally? It's not obviously clear from current implementation what happens if you pass count=0 to internal.Call(c, "datastore_v3", "RunQuery", &req, &t.res) - would it return 0 results or be treated as unlimited?

I agree re: the local var. Your suggestion is more readable. Could also be

count := t.limit if t.count > 0 && (t.limit < 0 || t.count < t.limit) { count = t.count }

Yes, a count of 0 doesn't make sense, at least when passed in by the user. If a value <= 0 is passed to BatchSize then an error should be returned.

A limit of 0 is ok though, and it's actually used internally. See Query's Count method, which includes this comment:

We also set the limit to zero, as we don't want any actual entity data, just the number of skipped results.

@mtraver thanks, -1 and negative limit in query now makes sense.

Thanks for review and rewrite to deal with negative values for t.limit.

mtraver · 2018-04-16T20:23:03Z

datastore/query.go

@@ -566,6 +587,8 @@ type Iterator struct {
 	// limit is the limit on the number of results this iterator should return.
 	// A negative value means unlimited.
 	limit int32
+	// count is the number of results this iterator should fetch at once.


Document behavior for negative values and zero.

t.count can take equal to or greater than zero. Negative values are rejected when BatchSize() sets q.count, and t.count is set as to be equal to q.count when constructing &Iterator{}. So I add note that it can take zero & positive values.

I'm not sure 0 makes sense for count. In fact, the if block we've been discussing checks that it's greater than 0. I feel like BatchSize should reject count values <= 0.

Indeed, that's correct. I fix BatchSize.

mtraver · 2018-04-16T20:23:07Z

datastore/query.go

@@ -605,7 +628,11 @@ func (t *Iterator) next() (*Key, *pb.EntityProto, error) {
 			return nil, nil, t.err
 		}
 		t.prevCC = t.res.CompiledCursor
-		if err := callNext(t.c, &t.res, 0, t.limit); err != nil {
+		count := t.limit
+		if t.count > 0 && (count == 0 || t.count < count) {


Ditto comment above

I think this logic should be changed as the same if block in Run()?

var count int32 if t.count > 0 && (t.limit < 0 || t.count < t.limit) { count = t.count } else { count = t.limit }

and clean up to be more readable

delphinus · 2018-04-27T02:33:55Z

Fixed all points. 👍

zombiezen · 2018-04-27T18:12:35Z

I'm not really the right person to review anymore. If you have specific questions, happy to answer, but if it looks good to you, looks good to me. :)

mtraver · 2018-04-27T18:16:26Z

@zombiezen Ok sounds good. I was just looking for a second LGTM.

@broady, could you please review?

lgrote · 2018-05-15T07:15:10Z

Hi, I'm waiting for this PR and I'm very happy to see that it is almost there. Any idea when this will be merged?
Thx to all who worked on this!

sbuss · 2018-05-16T23:35:31Z

For some reason travis is now broken. I don't think this PR was the culprit but I am reverting it just in case.

This reverts commit b9aad5d.

datastore: enable to set query batch size

08ae4b5

This adds q.BatchSize() method to avoid the error that claims too many datastore.next() calls due to the lack of specifying the `count` property (= batch size). Fixes golang#88

trakhimenok reviewed Feb 9, 2018

View reviewed changes

Disallow negative values for BatchSize

2876df0

mtraver reviewed Apr 16, 2018

View reviewed changes

mtraver requested a review from broady April 16, 2018 20:50

delphinus added 4 commits April 20, 2018 21:22

Deal with negative value for t.limit

54b52fd

and clean up to be more readable

Add comment about possible values for t.count

8b220aa

Use the same if block as in Run() for count

bb74ddb

Make BatchSize restrict to be greater than zero

a9c71df

mtraver requested a review from zombiezen April 27, 2018 18:08

mtraver removed the request for review from zombiezen April 27, 2018 18:16

sbuss merged commit b9aad5d into golang:master May 16, 2018

sbuss added a commit that referenced this pull request May 16, 2018

Revert "datastore: enable to set query batch size (#90)"

02a77d2

This reverts commit b9aad5d.

sbuss mentioned this pull request May 16, 2018

Revert "datastore: enable to set query batch size" #138

Closed

delphinus deleted the feature/enable-to-set-query-batch-size branch May 17, 2018 01:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

datastore: enable to set query batch size #90

datastore: enable to set query batch size #90

delphinus commented Sep 24, 2017

googlebot commented Sep 24, 2017

delphinus commented Sep 24, 2017

googlebot commented Sep 24, 2017

delphinus commented Oct 26, 2017

s-mang commented Oct 26, 2017

delphinus commented Oct 27, 2017

s-mang commented Oct 27, 2017

s-mang commented Nov 3, 2017

trakhimenok Feb 9, 2018

delphinus Apr 14, 2018

mtraver Apr 16, 2018

trakhimenok Apr 16, 2018

mtraver Apr 16, 2018

trakhimenok Apr 16, 2018

delphinus Apr 20, 2018

mtraver Apr 16, 2018

delphinus Apr 20, 2018

mtraver Apr 26, 2018

delphinus Apr 27, 2018

mtraver Apr 16, 2018

delphinus Apr 20, 2018

mtraver Apr 26, 2018

delphinus commented Apr 27, 2018

zombiezen commented Apr 27, 2018

mtraver commented Apr 27, 2018

lgrote commented May 15, 2018

sbuss commented May 16, 2018

datastore: enable to set query batch size #90

datastore: enable to set query batch size #90

Conversation

delphinus commented Sep 24, 2017

googlebot commented Sep 24, 2017

delphinus commented Sep 24, 2017

googlebot commented Sep 24, 2017

delphinus commented Oct 26, 2017

s-mang commented Oct 26, 2017

delphinus commented Oct 27, 2017

s-mang commented Oct 27, 2017

s-mang commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delphinus commented Apr 27, 2018

zombiezen commented Apr 27, 2018

mtraver commented Apr 27, 2018

lgrote commented May 15, 2018

sbuss commented May 16, 2018