impl/memory: Implement Queries

impl/memory: Implement Queries

R=dnj@chromium.org, estaab@chromium.org, tandrii@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://github.com/luci/gae/commit/34775cb901c24926e3de841e4f95907866c0e21e

[iannucci, 2015-08-20 10:29:22]

On 2015/08/20 10:28:44, iannucci wrote:

Not sufficiently tested, but I don't expect the code that's here to change
significantly as I add tests.

[iannucci, 2015-08-20 10:32:13]

https://chromiumcodereview.appspot.com/1302813003/diff/1/impl/memory/datastor...
File impl/memory/datastore_index_selection.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/1/impl/memory/datastor...
impl/memory/datastore_index_selection.go:18: type reducedQuery struct {
this structure is the key to this CL; all the code in datastore_query necks down
to this reduced query thingy. All the code in this file is devoted to taking
this distilled query object, and finding indexes that can fulfill it
(essentially: all indexes which have precisely the same suffixFormat, and as
many of those indexes as are necessary to put every eqFilter in the prefix of
some index at least once).

[iannucci, 2015-08-22 05:42:48]

Ok, this CL got a bit bigger, but I think this is actually all that's needed to
make queries work!

No tests yet, but I'll be adding them shortly. I think this is basically it!

[iannucci, 2015-08-22 13:02:43]

On 2015/08/22 05:42:48, iannucci wrote:
> Ok, this CL got a bit bigger, but I think this is actually all that's needed
to
> make queries work!
> 
> No tests yet, but I'll be adding them shortly. I think this is basically it!

Ok, got some basic tests in there (see datastore_query_execution_test.go)... It
like... works! :D

[dnj (Google), 2015-08-23 06:50:05]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2015-08-23 06:50:07]

So far looks solid. I haven't had a chance to look at index selection yet, but
here are some comments since you're moving fast with patch sets :)

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
File impl/memory/datastore_index_selection.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_index_selection.go:17: // reducedQuery contains only the
pieces of the query necessary to iteratate for
ITERATATE!

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
File impl/memory/datastore_query.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:69: //   IndexColumn will always contain __key__
as the last column, and so #orders
Do we need to encode the key column, then, or can we implicitly add it to the
end when encoding/decoding the cursor and save a few bytes?

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:74: d, err := base64.URLEncoding.DecodeString(s)
Ack - I suspect that the recent cursor change I committed will double-B64
encode/decode this. If I had to pick, though, I'd remove the second layer from
here instead of the interface.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:93: return nil, nil, fmt.Errorf("invalid cursor")
Since you're generating errors here, you should be more specific about which
condition was violated in the errors.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:96: if count > ds.MaxIndexColumns {
if count <= 0 || count > ds.MaxIndexColumns {
  return nil, nil, fmt.Errorf("invalid cursor: bad column count (%d)", count)

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:107: return nil, nil, fmt.Errorf("invalid
cursor")
Note which index failed and why in the returned error.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:112: return nil, nil, fmt.Errorf("invalid
cursor")
"column set must end with the key". If key is implicit, though, it's two fewer
possible failure conditions :)

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:209: } else if isTxn &&
q.eqFilters["__ancestor__"] == nil {
IMO the "else" chaining here is ugly and unnecessary now that you're returning
the error in the conditional logic.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:320: if ret.start != nil {
How about:
(a) Don't assign "ret.start" or "ret.end" above.
(b) Rewrite this as:

ret.start = q.ineqFilter.start
if q.startCursor != nil {
  if !sortOrders...{}
  if ret.start == nil || bytes.Compare(ret.start, q.startCursor)  < 0 {
    ret.start = q.startCursor
  }
}

(Same for end).

Reads cleaner as: default is whatever the inequality filter is using, but if
there's a cursor bound and it's greater than the inequality filter's start,
start there.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:432: if _, ok := q.eqFilters[prop]; !ok {
Retain the equality filter map from the initial lookup:
if eqFilter, ok := q.eqFilters[prop]; !ok {
  // Initialize/assign
} else {
  // assign
}

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:458: err = p.SetValue(val, ds.ShouldIndex)
if err := p.SetValue(...); err != nil {

Why did this switch from NoIndex to ShouldIndex?

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
File impl/memory/datastore_query_execution.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:121: return newProjectionStrategy(q,
rq, cb)
No need for "else" here.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:136: buf.SetInvert(needInvert)
Just inline the condition here: buf.SetInvert(suffixFormat[i].Direction ==
ds.DESCENDING)

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:142: raw[i] =
invert(suffix[prevOffset:offset])
Consider walking suffix:
suffix = suffix[:offset]

IMO would make this a little cleaner, no need for prevOffset, etc.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:155: return fmt.Errorf("unsupported
Query implementation: %#v", q)
%T?

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:177: hasLimit := limit < 0
There's a limit if either (a) limit < 0, or (b) limit > 0; therefore, "hasLimit"
should be "limit != 0", no?

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:200: } else if err != nil {
No need for "else".

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:216: rawData, data := parseSuffix(ns,
rq.suffixFormat, suffix)
I don't think "data" is a good name here. Maybe "decodedProps".

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:220: impossible(fmt.Errorf("decoded
index row doesn't end with a Key: %#v", data))
%T?

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
File impl/memory/gkvlite_iter.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
impl/memory/gkvlite_iter.go:14: type iterDefinition struct {
Nice combination.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
impl/memory/gkvlite_iter.go:49: defer ts[i].stop()
This "i" is bound do the last value held by the iterator. You need to rebind in
the loop interior:
i := i

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
impl/memory/gkvlite_iter.go:126: copy(start, def.prefix)
Might as well use "bjoin" here.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
impl/memory/gkvlite_iter.go:131: end = make([]byte,
len(def.prefix)+len(def.end))
Might as well use "bjoin" here.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
impl/memory/gkvlite_iter.go:154: defer ret.stop()
You should do this one before the loop.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
impl/memory/gkvlite_iter.go:164: // we need to start a new ascention function
ascension

Is this actually a condition that can happen? It seems like gkvlite should
reliably avoid this possibility. If not, I'd kill this logic altogether, as it
would avoid a full byte compare per round and also remove the "terminalCallback"
logic.

https://chromiumcodereview.appspot.com/1302813003/diff/170001/impl/memory/dat...
File impl/memory/datastore.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/170001/impl/memory/dat...
impl/memory/datastore.go:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
Perhaps you already have, but would be worthwhile to fully document (in detail)
the outcome of the immense amount of research that you performed, and
specifically the conditions and rules that AppEngine's queries follow. I know AE
docs cover some of this, but I feel like you can add depths of information to
their overview.

https://chromiumcodereview.appspot.com/1302813003/diff/170001/impl/memory/dat...
File impl/memory/datastore_query.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/170001/impl/memory/dat...
impl/memory/datastore_query.go:430: func (q *queryImpl) addEqFilt(prop string, p
ds.Property) []byte {
Does anything consume the return value? If not, you can just cast the result of
"serialize.ToBytes" to a string at the top.

https://chromiumcodereview.appspot.com/1302813003/diff/170001/service/datasto...
File service/datastore/index.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/170001/service/datasto...
service/datastore/index.go:49: func (id *IndexDefinition) Equal(o
*IndexDefinition) bool {
Does "gofmt" really not complain about a lack of comments? Not that I feel one
is explicitly needed with a function this obvious, but I'm used to complaints
about missing comments on exported methods of exported structures.

https://chromiumcodereview.appspot.com/1302813003/diff/170001/service/datasto...
File service/datastore/properties.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/170001/service/datasto...
service/datastore/properties.go:154: if PTUnknown > 0x7f {
IMO put this check in "serialize", as it owns the code that actually depends on
this condition.

[iannucci, 2015-08-23 18:19:43]

Thanks for the early review :)

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
File impl/memory/datastore_index_selection.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_index_selection.go:17: // reducedQuery contains only the
pieces of the query necessary to iteratate for
On 2015/08/23 06:50:06, dnj (Google) wrote:
> ITERATATE!

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
File impl/memory/datastore_query.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:69: //   IndexColumn will always contain __key__
as the last column, and so #orders
On 2015/08/23 06:50:06, dnj (Google) wrote:
> Do we need to encode the key column, then, or can we implicitly add it to the
> end when encoding/decoding the cursor and save a few bytes?

Yeah, we do, because it may not have an ASCENDING order. We could customize it
to just store the ASCENDING/DESCENDING bit, but the irregularity of that would
be annoying I think.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:74: d, err := base64.URLEncoding.DecodeString(s)
On 2015/08/23 06:50:06, dnj (Google) wrote:
> Ack - I suspect that the recent cursor change I committed will double-B64
> encode/decode this. If I had to pick, though, I'd remove the second layer from
> here instead of the interface.

Nah, I think it's fine here. This is parsing a cursor from the outside world.
The 'decode' at the bottom is making sure it is a series of encoded IndexColumns

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:93: return nil, nil, fmt.Errorf("invalid cursor")
On 2015/08/23 06:50:06, dnj (Google) wrote:
> Since you're generating errors here, you should be more specific about which
> condition was violated in the errors.

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:96: if count > ds.MaxIndexColumns {
On 2015/08/23 06:50:06, dnj (Google) wrote:
> if count <= 0 || count > ds.MaxIndexColumns {
>   return nil, nil, fmt.Errorf("invalid cursor: bad column count (%d)", count)

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:107: return nil, nil, fmt.Errorf("invalid
cursor")
On 2015/08/23 06:50:06, dnj (Google) wrote:
> Note which index failed and why in the returned error.

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:112: return nil, nil, fmt.Errorf("invalid
cursor")
On 2015/08/23 06:50:06, dnj (Google) wrote:
> "column set must end with the key". If key is implicit, though, it's two fewer
> possible failure conditions :)

Acknowledged.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:209: } else if isTxn &&
q.eqFilters["__ancestor__"] == nil {
On 2015/08/23 06:50:06, dnj (Google) wrote:
> IMO the "else" chaining here is ugly and unnecessary now that you're returning
> the error in the conditional logic.

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:320: if ret.start != nil {
On 2015/08/23 06:50:06, dnj (Google) wrote:
> How about:
> (a) Don't assign "ret.start" or "ret.end" above.
> (b) Rewrite this as:
> 
> ret.start = q.ineqFilter.start
> if q.startCursor != nil {
>   if !sortOrders...{}
>   if ret.start == nil || bytes.Compare(ret.start, q.startCursor)  < 0 {
>     ret.start = q.startCursor
>   }
> }
> 
> (Same for end).
> 
> Reads cleaner as: default is whatever the inequality filter is using, but if
> there's a cursor bound and it's greater than the inequality filter's start,
> start there.

Much better! Thanks...

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:432: if _, ok := q.eqFilters[prop]; !ok {
On 2015/08/23 06:50:06, dnj (Google) wrote:
> Retain the equality filter map from the initial lookup:
> if eqFilter, ok := q.eqFilters[prop]; !ok {
>   // Initialize/assign
> } else {
>   // assign
> }

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query.go:458: err = p.SetValue(val, ds.ShouldIndex)
On 2015/08/23 06:50:06, dnj (Google) wrote:
> if err := p.SetValue(...); err != nil {
> 
> Why did this switch from NoIndex to ShouldIndex?

Because we use this value directly as one of the values in the index key. The
index keys are a series of concatenated `ShouldIndex` Property []byte's.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
File impl/memory/datastore_query_execution.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:121: return newProjectionStrategy(q,
rq, cb)
On 2015/08/23 06:50:06, dnj (Google) wrote:
> No need for "else" here.

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:136: buf.SetInvert(needInvert)
On 2015/08/23 06:50:06, dnj (Google) wrote:
> Just inline the condition here: buf.SetInvert(suffixFormat[i].Direction ==
> ds.DESCENDING)

Need this below (line 141) as well

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:142: raw[i] =
invert(suffix[prevOffset:offset])
On 2015/08/23 06:50:06, dnj (Google) wrote:
> Consider walking suffix:
> suffix = suffix[:offset]
> 
> IMO would make this a little cleaner, no need for prevOffset, etc.

ya. done

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:155: return fmt.Errorf("unsupported
Query implementation: %#v", q)
On 2015/08/23 06:50:06, dnj (Google) wrote:
> %T?

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:177: hasLimit := limit < 0
On 2015/08/23 06:50:06, dnj (Google) wrote:
> There's a limit if either (a) limit < 0, or (b) limit > 0; therefore,
"hasLimit"
> should be "limit != 0", no?

:)

"https://cloud.google.com/appengine/docs/go/datastore/reference#Query.Limit"

See my facepalm on (*queryImpl).Limit

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:200: } else if err != nil {
On 2015/08/23 06:50:06, dnj (Google) wrote:
> No need for "else".

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:216: rawData, data := parseSuffix(ns,
rq.suffixFormat, suffix)
On 2015/08/23 06:50:06, dnj (Google) wrote:
> I don't think "data" is a good name here. Maybe "decodedProps".

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/dat...
impl/memory/datastore_query_execution.go:220: impossible(fmt.Errorf("decoded
index row doesn't end with a Key: %#v", data))
On 2015/08/23 06:50:06, dnj (Google) wrote:
> %T?

Nah. Changed to keyProp tho.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
File impl/memory/gkvlite_iter.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
impl/memory/gkvlite_iter.go:49: defer ts[i].stop()
On 2015/08/23 06:50:07, dnj (Google) wrote:
> This "i" is bound do the last value held by the iterator. You need to rebind
in
> the loop interior:
> i := i

good catch! Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
impl/memory/gkvlite_iter.go:126: copy(start, def.prefix)
On 2015/08/23 06:50:07, dnj (Google) wrote:
> Might as well use "bjoin" here.

Oh ya!

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
impl/memory/gkvlite_iter.go:131: end = make([]byte,
len(def.prefix)+len(def.end))
On 2015/08/23 06:50:07, dnj (Google) wrote:
> Might as well use "bjoin" here.

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
impl/memory/gkvlite_iter.go:154: defer ret.stop()
On 2015/08/23 06:50:07, dnj (Google) wrote:
> You should do this one before the loop.

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/150001/impl/memory/gkv...
impl/memory/gkvlite_iter.go:164: // we need to start a new ascention function
On 2015/08/23 06:50:07, dnj (Google) wrote:
> ascension
> 
> Is this actually a condition that can happen? It seems like gkvlite should
> reliably avoid this possibility. If not, I'd kill this logic altogether, as it
> would avoid a full byte compare per round and also remove the
"terminalCallback"
> logic.

? This is when we have an iterator over

1
2
3
4
5

which we start at 1, but then subsequently ask for 4. It has nothing to do with
gkvlite?

https://chromiumcodereview.appspot.com/1302813003/diff/170001/impl/memory/dat...
File impl/memory/datastore.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/170001/impl/memory/dat...
impl/memory/datastore.go:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
On 2015/08/23 06:50:07, dnj (Google) wrote:
> Perhaps you already have, but would be worthwhile to fully document (in
detail)
> the outcome of the immense amount of research that you performed, and
> specifically the conditions and rules that AppEngine's queries follow. I know
AE
> docs cover some of this, but I feel like you can add depths of information to
> their overview.

Yeah, it would be. I was thinking about doing that after this lands (as prep for
the brownbag)

https://chromiumcodereview.appspot.com/1302813003/diff/170001/impl/memory/dat...
File impl/memory/datastore_query.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/170001/impl/memory/dat...
impl/memory/datastore_query.go:430: func (q *queryImpl) addEqFilt(prop string, p
ds.Property) []byte {
On 2015/08/23 06:50:07, dnj (Google) wrote:
> Does anything consume the return value? If not, you can just cast the result
of
> "serialize.ToBytes" to a string at the top.

heh, old codes.

https://chromiumcodereview.appspot.com/1302813003/diff/170001/service/datasto...
File service/datastore/index.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/170001/service/datasto...
service/datastore/index.go:49: func (id *IndexDefinition) Equal(o
*IndexDefinition) bool {
On 2015/08/23 06:50:07, dnj (Google) wrote:
> Does "gofmt" really not complain about a lack of comments? Not that I feel one
> is explicitly needed with a function this obvious, but I'm used to complaints
> about missing comments on exported methods of exported structures.

yeah I haven't been running golint on this repo (gofmt never cares about
comments). I assumed it was part of pcg, but I guess I need to configure it.

https://chromiumcodereview.appspot.com/1302813003/diff/170001/service/datasto...
File service/datastore/properties.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/170001/service/datasto...
service/datastore/properties.go:154: if PTUnknown > 0x7f {
On 2015/08/23 06:50:07, dnj (Google) wrote:
> IMO put this check in "serialize", as it owns the code that actually depends
on
> this condition.

It's better here. There are actually 2 modules that rely on this behavior. I'd
rather keep with the assertion with the data that can cause the assertion
anyway. Otherwise you can change this module, and all will be well, but some
other package will break.

[iannucci, 2015-08-25 00:21:06]

Ok. I declare this sufficiently tested. I didn't get every single nook and
cranny, but you can take a look at `datastore_query_execution_test.go` to see
the queries that I'm testing for. If you have a particular favorite query you
want a test for, please let me know.

R

[iannucci, 2015-08-26 04:22:24]

OK, rewrote the ancient README.md to have lots of details about what the heck is
going on inside of the datastore implementation. I hope you like it, because I
can't write any more :)

[dnj, 2015-08-28 16:37:54]

Posting in multiple waves b/c apparently Rietveld doesn't persist comments
between browser sessions. This is mostly nits.

My current impression on CL (still a lot to review) is that it's very solid.
I'll have a bag of nits but so far nothing glaringly problematic.

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/README.md
File impl/memory/README.md (right):

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/README.md#...
impl/memory/README.md:26: compared only using bytes.Compare (aka `memcmp`). This
doc will assume that
"Compare (aka `memcmp`)"?

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/README.md#...
impl/memory/README.md:30: All encoded Property values used in Keys are
serialized `WithoutContext`, and
Wording.

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/README.md#...
impl/memory/README.md:105: `/Some,1/Thing,2`, and `/Some,1/Thing,2/Else,3` as
value in the ancestor column
Explain why?

Also the key notation, I'm assuming that "Some,1" is the entity partial key
component? Can you add a small paragraph explaining?

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/README.md#...
impl/memory/README.md:132: Index updates are pretty straightforward. On a
multation to the primary entity
mutation**

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/README.md#...
impl/memory/README.md:136: exist in the old, but not new entities are deleted,
entries that exist in the
Kind of awkward wording. Maybe: "...but not new are deleted"

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/README.md#...
impl/memory/README.md:181: If we fail to find sufficient indexes to fulfil the
query, we generate the
fulfill

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/README.md#...
impl/memory/README.md:182: 'missing' index by concatenating all missing equality
filters (in ascending
Not clear, maybe "a suggested index description which would be sufficient for
presentation to the user".

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/binary_too...
File impl/memory/binary_tools.go (right):

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/binary_too...
impl/memory/binary_tools.go:25: if bs == nil {
if len(bs) == 0

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/binary_too...
impl/memory/binary_tools.go:36: if bstr == nil {
len(bstr) == 0

But one cannot increment zero-length, correct? I'd panic here.

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/binary_too...
impl/memory/binary_tools.go:51: // This byte string was ALL FF's. The only safe
incrementation to do here
0xFF (you use 0x prefix elsewhere)

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/binary_too...
impl/memory/binary_tools.go:63: // are never equal to 0xFF, since they have the
higbit set (so either they're
high bit**

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/context.go
File impl/memory/context.go (right):

https://codereview.chromium.org/1302813003/diff/430001/impl/memory/context.go...
impl/memory/context.go:15: var serializationDeterministic = false
:D

[dnj (Google), 2015-08-28 17:54:22]

I think that's everything.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/REA...
File impl/memory/README.md (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/REA...
impl/memory/README.md:92: ### Index Tables
Comment about repeated tags and padding.

e.g., "tag tag tag bar".

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
File impl/memory/datastore_data.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_data.go:332: impossible(fmt.Errorf("cannot creat a
recursive transaction"))
"create"

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_data.go:335: impossible(fmt.Errorf("cannot creat a
recursive transaction"))
"create"

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
File impl/memory/datastore_index.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index.go:185: idxColl.Set(idxBin, []byte{})
Use []byte(nil) here, as []byte{} allocates an empty slice. If you ever append
to it, you'll use same semantics and end up allocating the same thing (or
better) regardless. This goes for other []byte{}.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index.go:218: it.stop()
It'd be better to arrange to do this via defer, as you leave a goroutine lying
around and several memory references otherwise.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index.go:227: ks := prefix + string(k)
Since you do this a lot, you should probably allocate a single outer buffer and
reuse. The current model has you doing two allocations (string(k), prefix +) per
round, but if you use a Buffer you can get away with one (buffer -> string).

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
File impl/memory/datastore_index_selection.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:28: eqFilters
map[string]map[string]struct{}
nit: Consider defining a type for "map[string]struct{}", maybe just "markerMap".
Has "put(k string)" and "has(k string)". I think the code would appear cleaner.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:94: } else if col.Direction ==
ds.DESCENDING && ocol.Direction == ds.DESCENDING {
Why "false" if we're both descending? Did you mean "ASCENDING" for the latter
condition?

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:150: requireSomeEqFilter :=
len(q.eqFilters) > 1
if len(q.eqFilters) > 1 || (len(q.eqFilters) == 1 && q.eqFilters["..."] == nil {
  return false
}

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:177: coll := s.GetCollection(
Need to test if "coll" is nil?

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:210: // getRelevantIndicies retrieves
the relevant indices which could be used to
Indexes?

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:299: remains.SortBy =
remains.SortBy[:len(remains.SortBy)-1]
Can just assign to "last".

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:306: "Your indicies are insufficient!
Try adding:\n  %s", remains)
indexes! Also not sure this is the best way to return this information. Consider
a custom "error" implementation that actually includes the list so the caller
can type assert and programmatically pull it apart. The Error() function can
still generate this string :)

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:319: toJoin := [][]byte{}
Can pre-allocate capacity to len(idx.eqFilts)

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:347: // chop the terminal null byte off
the q.ancestor key... we can accept
You don't do this here.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:395: "end suffix for implied ancestor
doesn't start with ancestor! start:%v ancestor:%v",
end:

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:428: ret = row[idx%len(row)]
Please document why this is done (pseudorandom padding field). Also maybe add a
PRNG instance to "constraints" and just use.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:469: func getIndicies(q *reducedQuery,
s *memStore) ([]*iterDefinition, error) {
Indexes

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:491: // TODO(riannucci): When filling
from residual, choose pseudorandom elements
You do this with the aforementioned modulus, so no longer TODO?

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
File impl/memory/datastore_query.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_query.go:629: if q.limitSet {
nit: worth checking to see if the value is the same before erroring? Don't
really care, but it's probably a good idea in case the user sets the limit in a
loop or something.

[iannucci, 2015-08-28 19:48:56]

Thanks, PTAL

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/REA...
File impl/memory/README.md (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/REA...
impl/memory/README.md:26: compared only using bytes.Compare (aka `memcmp`). This
doc will assume that
On 2015/08/28 at 16:37:54, dnj wrote:
> "Compare (aka `memcmp`)"?

? it's `bytes.Compare`

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/REA...
impl/memory/README.md:30: All encoded Property values used in Keys are
serialized `WithoutContext`, and
On 2015/08/28 at 16:37:54, dnj wrote:
> Wording.

Done I think?

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/REA...
impl/memory/README.md:92: ### Index Tables
On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> Comment about repeated tags and padding.
> 
> e.g., "tag tag tag bar".

Done. I think we should actually implement that technique in a later CL. The
memory savings could be pretty dramatic.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/REA...
impl/memory/README.md:105: `/Some,1/Thing,2`, and `/Some,1/Thing,2/Else,3` as
value in the ancestor column
On 2015/08/28 at 16:37:54, dnj wrote:
> Explain why?
> 
> Also the key notation, I'm assuming that "Some,1" is the entity partial key
component? Can you add a small paragraph explaining?

"/Some,1" is a key with no parent. It's the root of the full key.

It's the same format as the Key.String() method :)

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/REA...
impl/memory/README.md:132: Index updates are pretty straightforward. On a
multation to the primary entity
On 2015/08/28 at 16:37:54, dnj wrote:
> mutation**

done

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/REA...
impl/memory/README.md:136: exist in the old, but not new entities are deleted,
entries that exist in the
On 2015/08/28 at 16:37:54, dnj wrote:
> Kind of awkward wording. Maybe: "...but not new are deleted"

Better?

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/REA...
impl/memory/README.md:181: If we fail to find sufficient indexes to fulfil the
query, we generate the
On 2015/08/28 at 16:37:54, dnj wrote:
> fulfill

done

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/REA...
impl/memory/README.md:182: 'missing' index by concatenating all missing equality
filters (in ascending
On 2015/08/28 at 16:37:54, dnj wrote:
> Not clear, maybe "a suggested index description which would be sufficient for
presentation to the user".

How about this?

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/bin...
File impl/memory/binary_tools.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/bin...
impl/memory/binary_tools.go:25: if bs == nil {
On 2015/08/28 at 16:37:54, dnj wrote:
> if len(bs) == 0

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/bin...
impl/memory/binary_tools.go:36: if bstr == nil {
On 2015/08/28 at 16:37:54, dnj wrote:
> len(bstr) == 0
> 
> But one cannot increment zero-length, correct? I'd panic here.

Done.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/bin...
impl/memory/binary_tools.go:51: // This byte string was ALL FF's. The only safe
incrementation to do here
On 2015/08/28 at 16:37:54, dnj wrote:
> 0xFF (you use 0x prefix elsewhere)

done

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/bin...
impl/memory/binary_tools.go:63: // are never equal to 0xFF, since they have the
higbit set (so either they're
On 2015/08/28 at 16:37:54, dnj wrote:
> high bit**

Done

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/con...
File impl/memory/context.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/con...
impl/memory/context.go:15: var serializationDeterministic = false
On 2015/08/28 at 16:37:54, dnj wrote:
> :D

Yep, just for tests. This doesn't affect anything user-facing.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
File impl/memory/datastore_data.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_data.go:332: impossible(fmt.Errorf("cannot creat a
recursive transaction"))
On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> "create"

done

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_data.go:335: impossible(fmt.Errorf("cannot creat a
recursive transaction"))
On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> "create"


done

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
File impl/memory/datastore_index.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index.go:185: idxColl.Set(idxBin, []byte{})
On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> Use []byte(nil) here, as []byte{} allocates an empty slice. If you ever append
to it, you'll use same semantics and end up allocating the same thing (or
better) regardless. This goes for other []byte{}.

If gkvlite didn't panic on nil values, I would totally do that :)

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index.go:218: it.stop()
On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> It'd be better to arrange to do this via defer, as you leave a goroutine lying
around and several memory references otherwise.

I think you mean 'additionally do this via a defer'? Done.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index.go:227: ks := prefix + string(k)
On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> Since you do this a lot, you should probably allocate a single outer buffer
and reuse. The current model has you doing two allocations (string(k), prefix +)
per round, but if you use a Buffer you can get away with one (buffer -> string).

done. though this is definitely a micro-optimization.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
File impl/memory/datastore_index_selection.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:28: eqFilters
map[string]map[string]struct{}
On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> nit: Consider defining a type for "map[string]struct{}", maybe just
"markerMap". Has "put(k string)" and "has(k string)". I think the code would
appear cleaner.

done, though it doesn't really do too much.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:94: } else if col.Direction ==
ds.DESCENDING && ocol.Direction == ds.DESCENDING {
On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> Why "false" if we're both descending? Did you mean "ASCENDING" for the latter
condition?

oops. Done.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:150: requireSomeEqFilter :=
len(q.eqFilters) > 1
On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> if len(q.eqFilters) > 1 || (len(q.eqFilters) == 1 && q.eqFilters["..."] == nil
{
>   return false
> }

done

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:177: coll := s.GetCollection(
On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> Need to test if "coll" is nil?

nope. Clarified in comment.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:210: // getRelevantIndicies retrieves
the relevant indices which could be used to
On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> Indexes?

done

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:299: remains.SortBy =
remains.SortBy[:len(remains.SortBy)-1]
On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> Can just assign to "last".

That would have no effect? Did you mean something else?

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:306: "Your indicies are insufficient!
Try adding:\n  %s", remains)
On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> indexes! Also not sure this is the best way to return this information.
Consider a custom "error" implementation that actually includes the list so the
caller can type assert and programmatically pull it apart. The Error() function
can still generate this string :)

yes, I was planning on changing that in a later CL (in fact, the next CL).

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:319: toJoin := [][]byte{}
On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> Can pre-allocate capacity to len(idx.eqFilts)

Done

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:347: // chop the terminal null byte off
the q.ancestor key... we can accept
On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> You don't do this here.

oops. moved comment.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:395: "end suffix for implied ancestor
doesn't start with ancestor! start:%v ancestor:%v",
On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> end:

done

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:428: ret = row[idx%len(row)]
On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> Please document why this is done (pseudorandom padding field). Also maybe add
a PRNG instance to "constraints" and just use.

It's not random. A PRNG would be slower and serve no purpose. The only reason I
round robin (besides being deterministic) is so that we'll 'hopefully' end up
picking a residual value which scans fewer rows. Until we actually execute the
query, all values are equally likely to be the best choice. A PRNG would just
make this slower and more complicated without actually adding any value.  The
reason I said that in the TODO is because I hadn't thought it through completely
(hence the TODO :)).

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:469: func getIndicies(q *reducedQuery,
s *memStore) ([]*iterDefinition, error) {
On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> Indexes

You know that both spellings are correct, right?

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:491: // TODO(riannucci): When filling
from residual, choose pseudorandom elements
On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> You do this with the aforementioned modulus, so no longer TODO?

yes

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
File impl/memory/datastore_query.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_query.go:629: if q.limitSet {
On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> nit: worth checking to see if the value is the same before erroring? Don't
really care, but it's probably a good idea in case the user sets the limit in a
loop or something.

If they set it in a loop then they're doing it weird. What would the use case
for that be? Note that every time they call Limit they get a brand new Query
back. At the very least they're allocating a whole bunch more queries than they
need.

[dnj, 2015-08-28 19:57:53]

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
File impl/memory/datastore_index_selection.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:299: remains.SortBy =
remains.SortBy[:len(remains.SortBy)-1]
On 2015/08/28 at 19:48:55, iannucci wrote:
> On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> > Can just assign to "last".
> 
> That would have no effect? Did you mean something else?

Oh nope, just overlooked the colon. Nevermind.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_index_selection.go:469: func getIndicies(q *reducedQuery,
s *memStore) ([]*iterDefinition, error) {
On 2015/08/28 at 19:48:55, iannucci wrote:
> On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> > Indexes
> 
> You know that both spellings are correct, right?

Yes, but you've been generally consistent using "indexes" with a few spot
exceptions.

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
File impl/memory/datastore_query.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
impl/memory/datastore_query.go:629: if q.limitSet {
On 2015/08/28 at 19:48:56, iannucci wrote:
> On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> > nit: worth checking to see if the value is the same before erroring? Don't
really care, but it's probably a good idea in case the user sets the limit in a
loop or something.
> 
> If they set it in a loop then they're doing it weird. What would the use case
for that be? Note that every time they call Limit they get a brand new Query
back. At the very least they're allocating a whole bunch more queries than they
need.

One use case might be starting with a standard query/limit setup and then later
expanding the limit later based on some logic or an optimization. Actually,
AppEngine allows this and just updates the limit value regardless.

[iannucci, 2015-08-28 20:02:12]

On 2015/08/28 at 19:57:53, dnj wrote:
>
https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
> File impl/memory/datastore_index_selection.go (right):
> 
>
https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
> impl/memory/datastore_index_selection.go:299: remains.SortBy =
remains.SortBy[:len(remains.SortBy)-1]
> On 2015/08/28 at 19:48:55, iannucci wrote:
> > On 2015/08/28 at 17:54:21, dnj (Google) wrote:
> > > Can just assign to "last".
> > 
> > That would have no effect? Did you mean something else?
> 
> Oh nope, just overlooked the colon. Nevermind.
> 
>
https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
> impl/memory/datastore_index_selection.go:469: func getIndicies(q
*reducedQuery, s *memStore) ([]*iterDefinition, error) {
> On 2015/08/28 at 19:48:55, iannucci wrote:
> > On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> > > Indexes
> > 
> > You know that both spellings are correct, right?
> 
> Yes, but you've been generally consistent using "indexes" with a few spot
exceptions.
> 
>
https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
> File impl/memory/datastore_query.go (right):
> 
>
https://chromiumcodereview.appspot.com/1302813003/diff/430001/impl/memory/dat...
> impl/memory/datastore_query.go:629: if q.limitSet {
> On 2015/08/28 at 19:48:56, iannucci wrote:
> > On 2015/08/28 at 17:54:22, dnj (Google) wrote:
> > > nit: worth checking to see if the value is the same before erroring? Don't
really care, but it's probably a good idea in case the user sets the limit in a
loop or something.
> > 
> > If they set it in a loop then they're doing it weird. What would the use
case for that be? Note that every time they call Limit they get a brand new
Query back. At the very least they're allocating a whole bunch more queries than
they need.
> 
> One use case might be starting with a standard query/limit setup and then
later expanding the limit later based on some logic or an optimization.
Actually, AppEngine allows this and just updates the limit value regardless.

Hm... I still think it would be better to set the limit once... I fixed it
anyway :)

[dnj, 2015-08-28 20:15:04]

lgtm w/ some nits

https://chromiumcodereview.appspot.com/1302813003/diff/470001/impl/memory/dat...
File impl/memory/datastore_index_selection.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/470001/impl/memory/dat...
impl/memory/datastore_index_selection.go:209: // getRelevantIndicies retrieves
the relevant indexes which could be used to
nit: I was going function name, too :)

But if you like "indices" I don't really care; just attempting to alert on an
inconsistency in an otherwise consistent CL.

https://chromiumcodereview.appspot.com/1302813003/diff/470001/impl/memory/str...
File impl/memory/stringset.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/470001/impl/memory/str...
impl/memory/stringset.go:9: func (s stringSet) Add(value string) {
nit: Methods should not be exported, since stringSet is not exported.

nit: IMO would increase value of this if you added a "func (...) contains(value
string) {...}

Then logic using it would look like:
if !sset.contains("myvalue") {
  // do stuff
  sset.add("myvalue")
}

etc., which reads cleaner than:
if _, ok := sset["myvalue"]; ok {
  // do stuff
  sset["myvalue"] = struct{}{}
}

[iannucci, 2015-08-28 20:38:50]

https://chromiumcodereview.appspot.com/1302813003/diff/470001/impl/memory/dat...
File impl/memory/datastore_index_selection.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/470001/impl/memory/dat...
impl/memory/datastore_index_selection.go:209: // getRelevantIndicies retrieves
the relevant indexes which could be used to
On 2015/08/28 at 20:15:04, dnj wrote:
> nit: I was going function name, too :)
> 
> But if you like "indices" I don't really care; just attempting to alert on an
inconsistency in an otherwise consistent CL.

Oops... My grep missed it. Fixed now.

https://chromiumcodereview.appspot.com/1302813003/diff/470001/impl/memory/str...
File impl/memory/stringset.go (right):

https://chromiumcodereview.appspot.com/1302813003/diff/470001/impl/memory/str...
impl/memory/stringset.go:9: func (s stringSet) Add(value string) {
On 2015/08/28 at 20:15:04, dnj wrote:
> nit: Methods should not be exported, since stringSet is not exported.
> 

done. though I usually prefer to export them like this when it seems likely that
the type could conceivably be in a different package and is prone to be lifted
later.

> nit: IMO would increase value of this if you added a "func (...)
contains(value string) {...}
> 
> Then logic using it would look like:
> if !sset.contains("myvalue") {
>   // do stuff
>   sset.add("myvalue")
> }
> 
> etc., which reads cleaner than:
> if _, ok := sset["myvalue"]; ok {
>   // do stuff
>   sset["myvalue"] = struct{}{}
> }

Done and added some other goodies.

[dnj, 2015-08-28 23:21:35]

> Done and added some other goodies.

Great, that's more or less exactly what I had in mind. Still LGTM!

[iannucci, 2015-08-29 00:11:30]

On 2015/08/28 at 23:21:35, dnj wrote:
> > Done and added some other goodies.
> 
> Great, that's more or less exactly what I had in mind. Still LGTM!

Committing this so I can roll infra.git. If anyone else has more comments, I'm
more than happy to make them in followup CLs :)

[iannucci, 2015-08-29 00:11:34]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2015-08-29 00:11:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1302813003/510001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1302813003/510001

[commit-bot: I haz the power, 2015-08-29 00:13:57]

Committed patchset #27 (id:510001) as
https://github.com/luci/gae/commit/34775cb901c24926e3de841e4f95907866c0e21e