Refactor keys and queries in datastore service and implementation.

Refactor keys and queries in datastore service and implementation.
  * datastore.Query and datastore.FinalizedQuery are now first class objects
    in the service/datastore api.
  * datastore.Key is similarly a first class object. All luci/gae users will
    need to interact with `*ds.Key` now. This object gained all the superpowers
    that previously existed in the dskey subpackage of service/datastore.

The new Query objects are inspectable, and adjustable. Additionally, this
homogenizes they query generation between different implementations. The
normalization which normally occurs on the server-side is now done client-side.
This should ensure consistency between 'prod' and 'memory' implementations.

Additionally, the new FinalizedQuery object gains a GQL() method which should
(hopefully) allow trivial debugging by printing a query. You should be able to
copy and paste the generated GQL* into the appengine dashboard and see what
your request was querying for.

* With the exception of Cursors, which currently have no textual format in the
GQL schema.

R=dnj@chromium.org, estaab@chromium.org, maruel@chromium.org, tandrii@chromium.org, vadimsh@chromium.org
BUG=533024
Committed: https://github.com/luci/gae/commit/f8e86bf226d6985065d19449e21d63398be39a67

[iannucci, 2015-09-18 04:02:27]

On 2015/09/18 04:00:29, iannucci wrote:

Tests are currently all passing. I'm going to add some more, but feel free to
start reviewing now.

[iannucci, 2015-09-18 04:31:53]

some notes

https://chromiumcodereview.appspot.com/1355783002/diff/1/filter/count/rds.go
File filter/count/rds.go (left):

https://chromiumcodereview.appspot.com/1355783002/diff/1/filter/count/rds.go#...
filter/count/rds.go:18: NewQuery         Entry
these aren't part of RawInterface any more.

https://chromiumcodereview.appspot.com/1355783002/diff/1/filter/dscache/dscac...
File filter/dscache/dscache_test.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/filter/dscache/dscac...
filter/dscache/dscache_test.go:74: last := k.Last()
Key changed a bit. Now instead of k.Kind(), you get the last token with Last(),
and then that has Kind, StringID and IntID. You can also get the full list of
all the tokens in the key. Or you can recurse down it with Parent, but I wanted
to make that usecase a bit more awkward, because it involves a lot of extra
(avoidable) allocations

https://chromiumcodereview.appspot.com/1355783002/diff/1/filter/featureBreake...
File filter/featureBreaker/rds.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/filter/featureBreake...
filter/featureBreaker/rds.go:28: func (r *dsState) Run(q *ds.FinalizedQuery, cb
ds.RawRunCB) error {
turns out I was missing this method entirely because ds.RawInterface was an
anonymous embedded field above :/

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
File impl/memory/datastore_data.go (left):

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_data.go:441: }
stale code is stale!

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
File impl/memory/datastore_query.go (left):

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_query.go:319: 
all this missing stuff is now common code in
"service/datastore".Query.Finalize()

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

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_query.go:206: }
This changed because we don't serialize properties in service/datastore (that
felt a bit too cheeky). Instead, service/datastore keeps them as regular
Property objects, so we have to sort and serialize them here now.

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

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_query_execution_test.go:262: "$key", key("Kind", 2,
"__entity_group__", 1), NEXT,
key (which is an alias for datastore.MakeKey above), means that we no longer
have to ("backwards", ("and", ("nested", ("keys", ("our", ("write"))))))

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_query_execution_test.go:281: q :=
nq("Something").Eq("Does", 2).Order("Not", "Work")
I made Order accept multiples, just like Project.

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_query_execution_test.go:297: {q: nq("Kind").Eq("Val", 1,
3), get: []ds.PropertyMap{
Eq can accept multiples too.

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

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_query_test.go:14: 
I still need to fill this in. You can see some of the commented-out test cases
at the bottom.

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

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_test.go:67: So(k.String(), ShouldEqual, "dev~app::/Foo,1")
I got pissed off at the Key string format not actually containing all the
information for the key. So now it's 
  <aid>:<ns>:/kind,id/kind,id

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/testing_...
File impl/memory/testing_utils_test.go (left):

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/testing_...
impl/memory/testing_utils_test.go:94: func key(kind string, id interface{},
parent ...ds.Key) ds.Key {
I had like 5 copies of this function all over the place. I think that qualifies
as 'useful enough to be a real function'

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/testing_...
File impl/memory/testing_utils_test.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/testing_...
impl/memory/testing_utils_test.go:66: return ds.NewQuery(kind)
Querys no longer embed the namespace and appid inside of them. Instead, the
implementation validates that the Query is valid in the current appid and
namespace right before execution.

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/datastore_...
File impl/prod/datastore_key.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/datastore_...
impl/prod/datastore_key.go:44: _, ns, toks := k.Split()
We drop the appid here... we can't actually control it, and the keys generated
here never leave this package and are just fed to the actual production
datastore.

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/raw_datast...
File impl/prod/raw_datastore.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/raw_datast...
impl/prod/raw_datastore.go:95: func (d rdsImpl) fixQuery(fq *ds.FinalizedQuery)
(*datastore.Query, error) {
Now that we don't have the thin wrapper query thing, this function converts a
FinalizedQuery into the lame Query object that the SDK wants.

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/raw_datast...
File impl/prod/raw_datastore_type_converter.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/raw_datast...
impl/prod/raw_datastore_type_converter.go:103: func dsR2FProp(in
datastore.Property) (ds.Property, error) {
I extracted these functions out from the PropertyMap Load/Save functions so that
we can use them in fixQuery (which needs to fix things like Ancestor filter
keys, and equality filters (e.g. for GeoPoints, BlobKeys, etc.))

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/da...
File service/datastore/datastore.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/da...
service/datastore/datastore.go:142: errs := map[int]error{}
this was actually just a flat-out bug that would have bit us. I'm not sure why I
decided it was OK to use LazyMultiError here when slice.Len() is passed in by
the user and is PROBABLY ZERO. The old code works fine as long as you don't have
any errors. Derp.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/da...
File service/datastore/datastore_test.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/da...
service/datastore/datastore_test.go:257: So(ds.KeyForObj(&idStruct{}).String(),
ShouldEqual, `s~aid:ns:/SuperKind,"wut"`)
oh, and I disambiguate between StringID and IntID too.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/da...
service/datastore/datastore_test.go:768: q = q.Eq("$err_single", "Query
fail").Eq("$err_single_idx", 3)
because the RawInterface no longer mints Query objects, I use this hack to
communicate failure modes to my crappy fake RawInterface implementation above.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
File service/datastore/finalized_query.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:42: func (q *FinalizedQuery) Original()
*Query {
yeah, I know I need docstrings on all of these.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:42: func (q *FinalizedQuery) Original()
*Query {
yeah, I know I need docstrings on all of these.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/pr...
File service/datastore/properties.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/pr...
service/datastore/properties.go:455: func cmpVals(a, b interface{}, t
PropertyType) int {
does anyone know how to write this thing better? ugh...

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ra...
File service/datastore/raw_interface.go (left):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ra...
service/datastore/raw_interface.go:158: NewQuery(kind string) Query
bye!!!!

[dnj, 2015-09-18 16:47:59]

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

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_query.go:136: _, startOp, startV := fq.IneqFilterLow()
Maybe comment that the goal here is to bound >=, < as well.

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_query.go:160: //   still using natrual bytes ordering)
Spaces at beginning of comment.

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/datastore_...
File impl/prod/datastore_key.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/datastore_...
impl/prod/datastore_key.go:25: toks := []ds.KeyTok(nil)
Since this will be executed frequently, it may be worth doing a counting
traversal first so you can allocate "toks" to the correct size.

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/datastore_...
impl/prod/datastore_key.go:44: _, ns, toks := k.Split()
On 2015/09/18 04:31:52, iannucci wrote:
> We drop the appid here... we can't actually control it, and the keys generated
> here never leave this package and are just fed to the actual production
> datastore.

Please document this in a formal comment.

Note that k.Split() can return a zero-length array if key is non-nil and the
KeyTok array is empty. You ensure that this cannot be the case by construction,
but I'd formally document the expectation that this is the case somewhere.

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/datastore_...
impl/prod/datastore_key.go:61: err := error(nil)
Does it make sense to use a MultiError here?

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/raw_datast...
File impl/prod/raw_datastore.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/raw_datast...
impl/prod/raw_datastore.go:87: k := (*ds.Key)(nil)
As an aside, I really hate this notation for nil-value declarations as opposed
to:
var k *ds.Key

But whatever.

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/raw_datast...
impl/prod/raw_datastore.go:95: func (d rdsImpl) fixQuery(fq *ds.FinalizedQuery)
(*datastore.Query, error) {
On 2015/09/18 04:31:53, iannucci wrote:
> Now that we don't have the thin wrapper query thing, this function converts a
> FinalizedQuery into the lame Query object that the SDK wants.

Maybe call it "breakQuery" then. "lameify"? :)

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
File service/datastore/finalized_query.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:42: func (q *FinalizedQuery) Original()
*Query {
On 2015/09/18 04:31:53, iannucci wrote:
> yeah, I know I need docstrings on all of these.

yeah, I know you know you need docstrings on all of these.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:55: ret := make([]string, len(q.project))
Maybe an allocation-avoiding shortcut here:
if len(q.project) == 0 {
  return nil
}

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:117: op = ">"
Since "op" is a return value, you can avoid the double-assignment with an
if/else.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:129: op = "<"
Since "op" is a return value, you can avoid the double-assignment with an
if/else.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:160: func (q *FinalizedQuery) GQL() string
{
Make sure you mention which dialect of GQL you're using.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:173: ret.WriteString(strings.Join(proj, ",
"))
nit: Since we're already leaning heavily on the Buffer's resizing, I'm not sure
there's much value to separate alloc + Join instead of:
for i, p := range q.project {
  if i > 0 {
    ret.WriteString(", ")
  } else {
    ret.WriteString(" ")
  }
  ret.WriteString(gqlQuoteName(p))
}

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:186: if k == "__ancestor__" {
Retain this for later so you don't have to double-lookup. Also adds some
continuity.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:212: if anc, ok :=
q.eqFilts["__ancestor__"]; ok {
Use retained value here.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/in...
File service/datastore/interface.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/in...
service/datastore/interface.go:42: MakeKey(elems ...interface{}) *Key
WDYT about making these helper methods instead of interface methods? IMO
convenience methods should not belong to the interface.
ds.MakeKey(ctx, ...)

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
File service/datastore/key.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:42: if k.Kind < other.Kind {
(see note about double-compare below)

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:55: toks      []KeyTok
Note that by construction, this array is expected to have nonzero length if the
key is not nil.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:74: copy(newToks, toks)
Since a KeyTok's fields are mutable, should this be a deep copy?

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:119: for i := 0; i < len(elems); i += 2 {
Consider rewriting using slice-based traversal. I've always found that a bit
cleaner.

for len(elems) > 0 {
  knd, ok := elems[0].(string)
  ...
  tok := &toks[tokIdx++]
  // operate on tok, avoids all the repeated division/indexing.

  elems = elems[2:]
}

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:180: func (k *Key) AppID() string { return k.appID }
(*Key)(nil) is treated inconsistently. For some methods it returns a default
value, for others it would clearly panic. I generally don't like the pattern of
calling methods on nil objects, but I see the appeal WRT allocating empty keys.
This seems like an uncommon case, though, so WDYT about
s/nil-key/key-with-no-tokens/?

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:191: b := bytes.NewBuffer(make([]byte, 0, 512))
I prefer:
bytes.Buffer{}
b.Grow(512)

But really why preallocate at all? String() is not a necessary function, so IMO
let the Buffer grow on demand.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:268: // It's encoded with the urlsafe base64 table.
... with padding stripped off.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:329: }
They could still both be nil. Do:
if k == nil {
  return other != nil
}

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:331: if k.appID < other.appID {
Slightly better to avoid double string compare and do:
cmp := strings.Compare(k.appID, other.appID)
switch cmp {
  case -1:
    return true
  case 1:
    return false
}

"" for other string comparisons here.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/mu...
File service/datastore/multiarg.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/mu...
service/datastore/multiarg.go:255: lst := key.Last()
key.Last can return nil if the key is empty, no?

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/pr...
File service/datastore/properties.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/pr...
service/datastore/properties.go:211: func MkProperty(val interface{}) Property {
Now that Property is a struct, this should probably return a *Property (same for
the rest).

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/pr...
service/datastore/properties.go:455: func cmpVals(a, b interface{}, t
PropertyType) int {
On 2015/09/18 04:31:53, iannucci wrote:
> does anyone know how to write this thing better? ugh...

*shrug* inner switch statements could make it a little nicer, I suppose.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/pr...
service/datastore/properties.go:482: if a == b {
Use strings.Compare here.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/pr...
service/datastore/properties.go:502: cmp := cmpVals(a.Lat, b.Lat, PTFloat)
This seems unnecessarily reflective.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/pr...
service/datastore/properties.go:585: return fmt.Sprintf("DATETIME(%s)",
p.value.(time.Time).Format("2006-01-02T15:04:05.000000Z"))
time.RFC3339?

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
File service/datastore/query.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:74: if q.eqFilts != nil {
len(q.eqFilts) > 0

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:86: func (q *Query) Kind(kind string) *Query {
Worth doing pre-mod identity checks for these? e.g.,
if kind != q.kind {
  ...
}

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:156: func (q *Query) RevertOrder() *Query {
ClearOrder()

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:186: func (q *Query) RevertProject() *Query {
ClearProject

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:208: q.eqFilts = map[string]PropertySlice{}
make(..., 1)

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:238: if strings.HasPrefix(field, "__") &&
strings.HasSuffix(field, "__") {
Is "_ _ _" (without spaces) reserved? Granted it's a stupid field name...

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:266: if err == nil && q.ineqFiltHighSet {
IMO the auto-adjustment is convenient, but it should be an error to set multiple
inequality filters on a Query.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:362: func (q *Query) RevertFilters() *Query {
ClearFilters

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:557: }
ineqFiltHighSet?

[iannucci, 2015-09-18 22:25:49]

PTAL

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

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_query.go:136: _, startOp, startV := fq.IneqFilterLow()
On 2015/09/18 at 16:47:57, dnj wrote:
> Maybe comment that the goal here is to bound >=, < as well.

Done I think

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_query.go:160: //   still using natrual bytes ordering)
On 2015/09/18 at 16:47:57, dnj wrote:
> Spaces at beginning of comment.

done

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

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/memory/datastor...
impl/memory/datastore_query_test.go:14: 
On 2015/09/18 at 04:31:52, iannucci wrote:
> I still need to fill this in. You can see some of the commented-out test cases
at the bottom.

Done

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/datastore_...
File impl/prod/datastore_key.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/datastore_...
impl/prod/datastore_key.go:25: toks := []ds.KeyTok(nil)
On 2015/09/18 at 16:47:57, dnj wrote:
> Since this will be executed frequently, it may be worth doing a counting
traversal first so you can allocate "toks" to the correct size.

done

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/datastore_...
impl/prod/datastore_key.go:44: _, ns, toks := k.Split()
On 2015/09/18 at 16:47:57, dnj wrote:
> On 2015/09/18 04:31:52, iannucci wrote:
> > We drop the appid here... we can't actually control it, and the keys
generated
> > here never leave this package and are just fed to the actual production
> > datastore.
> 
> Please document this in a formal comment.

Noted in doc.go as a BUG.

> 
> Note that k.Split() can return a zero-length array if key is non-nil and the
KeyTok array is empty. You ensure that this cannot be the case by construction,
but I'd formally document the expectation that this is the case somewhere.

Noted on service/datastore.Key

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/datastore_...
impl/prod/datastore_key.go:61: err := error(nil)
On 2015/09/18 at 16:47:57, dnj wrote:
> Does it make sense to use a MultiError here?

yuuuup

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/raw_datast...
File impl/prod/raw_datastore.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/impl/prod/raw_datast...
impl/prod/raw_datastore.go:87: k := (*ds.Key)(nil)
On 2015/09/18 at 16:47:57, dnj wrote:
> As an aside, I really hate this notation for nil-value declarations as opposed
to:
> var k *ds.Key
> 
> But whatever.

I actually really hate the other one because it's inconsistent :(

I actually wish `var Type name` was always `name := Type(nil)`, including `name
:= int64(nil)`, and that the var syntax didn't exist. Same with the new syntax
as well... the only time you need it is when you're new'ing a number (which I
think `&100` should totally work).

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
File service/datastore/finalized_query.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:55: ret := make([]string, len(q.project))
On 2015/09/18 at 16:47:57, dnj wrote:
> Maybe an allocation-avoiding shortcut here:
> if len(q.project) == 0 {
>   return nil
> }

done

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:117: op = ">"
On 2015/09/18 at 16:47:57, dnj wrote:
> Since "op" is a return value, you can avoid the double-assignment with an
if/else.

¯\_(ツ)_/¯

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:173: ret.WriteString(strings.Join(proj, ",
"))
On 2015/09/18 at 16:47:57, dnj wrote:
> nit: Since we're already leaning heavily on the Buffer's resizing, I'm not
sure there's much value to separate alloc + Join instead of:
> for i, p := range q.project {
>   if i > 0 {
>     ret.WriteString(", ")
>   } else {
>     ret.WriteString(" ")
>   }
>   ret.WriteString(gqlQuoteName(p))
> }

if there are a bunch of projections, that's a lot of potential re-allocations.
If buffer was internally using a `rope` structure, then yeah, but it's not 
(that'd be a good optimization though...)

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:186: if k == "__ancestor__" {
On 2015/09/18 at 16:47:57, dnj wrote:
> Retain this for later so you don't have to double-lookup. Also adds some
continuity.

done

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/in...
File service/datastore/interface.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/in...
service/datastore/interface.go:42: MakeKey(elems ...interface{}) *Key
On 2015/09/18 at 16:47:57, dnj wrote:
> WDYT about making these helper methods instead of interface methods? IMO
convenience methods should not belong to the interface.
> ds.MakeKey(ctx, ...)

discussed offline; Interface /is/ the convenience method holder :). It could
also probably just be a struct too, since its not actually meant to have more
than one implementation, and the implementation lives in this package anyway...

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
File service/datastore/key.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:55: toks      []KeyTok
On 2015/09/18 at 16:47:58, dnj wrote:
> Note that by construction, this array is expected to have nonzero length if
the key is not nil.

done

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:74: copy(newToks, toks)
On 2015/09/18 at 16:47:58, dnj wrote:
> Since a KeyTok's fields are mutable, should this be a deep copy?

It is a deep copy, since there are no pointers involved :)

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:119: for i := 0; i < len(elems); i += 2 {
On 2015/09/18 at 16:47:58, dnj wrote:
> Consider rewriting using slice-based traversal. I've always found that a bit
cleaner.
> 
> for len(elems) > 0 {
>   knd, ok := elems[0].(string)
>   ...
>   tok := &toks[tokIdx++]
>   // operate on tok, avoids all the repeated division/indexing.
> 
>   elems = elems[2:]
> }

Done

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:180: func (k *Key) AppID() string { return k.appID }
On 2015/09/18 at 16:47:58, dnj wrote:
> (*Key)(nil) is treated inconsistently. For some methods it returns a default
value, for others it would clearly panic. I generally don't like the pattern of
calling methods on nil objects, but I see the appeal WRT allocating empty keys.
This seems like an uncommon case, though, so WDYT about
s/nil-key/key-with-no-tokens/?

In the SDK it's impossible to have a non-nil key without values. I'd rather
retain that. I just let everything panic when k is nil now.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:191: b := bytes.NewBuffer(make([]byte, 0, 512))
On 2015/09/18 at 16:47:57, dnj wrote:
> I prefer:
> bytes.Buffer{}
> b.Grow(512)
> 
> But really why preallocate at all? String() is not a necessary function, so
IMO let the Buffer grow on demand.

This is a carry over from
https://github.com/golang/appengine/blob/master/datastore/key.go#L129 . I think
I'll just leave it in.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:268: // It's encoded with the urlsafe base64 table.
On 2015/09/18 at 16:47:58, dnj wrote:
> ... with padding stripped off.

Done

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:329: }
On 2015/09/18 at 16:47:58, dnj wrote:
> They could still both be nil. Do:
> if k == nil {
>   return other != nil
> }

none of this exists anymore. You have nil you get boom.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:331: if k.appID < other.appID {
On 2015/09/18 at 16:47:58, dnj wrote:
> Slightly better to avoid double string compare and do:
> cmp := strings.Compare(k.appID, other.appID)

Which doesn't exist in 1.4 and whose 1.5 implementation does the exact same
thing as I do here: http://golang.org/src/strings/compare.go?s=491:520#L3

> switch cmp {
>   case -1:
>     return true
>   case 1:
>     return false
> }
> 
> "" for other string comparisons here.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/mu...
File service/datastore/multiarg.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/mu...
service/datastore/multiarg.go:255: lst := key.Last()
On 2015/09/18 at 16:47:58, dnj wrote:
> key.Last can return nil if the key is empty, no?

nil key gets boom

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/pr...
File service/datastore/properties.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/pr...
service/datastore/properties.go:211: func MkProperty(val interface{}) Property {
On 2015/09/18 at 16:47:58, dnj wrote:
> Now that Property is a struct, this should probably return a *Property (same
for the rest).

It has always been a struct. I chose not to pass *Property because in the
current model you can never have an invalid property (by construction). If I
change that, I'd rather do it in a different CL as it would involve putting a
lot of `if p == nil { return error}` crap all over the place. I'm inclined not
to do it at all.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/pr...
service/datastore/properties.go:502: cmp := cmpVals(a.Lat, b.Lat, PTFloat)
On 2015/09/18 at 16:47:58, dnj wrote:
> This seems unnecessarily reflective.

functionized

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/pr...
service/datastore/properties.go:585: return fmt.Sprintf("DATETIME(%s)",
p.value.(time.Time).Format("2006-01-02T15:04:05.000000Z"))
On 2015/09/18 at 16:47:58, dnj wrote:
> time.RFC3339?

Nano, and thanks

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
File service/datastore/query.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:74: if q.eqFilts != nil {
On 2015/09/18 at 16:47:58, dnj wrote:
> len(q.eqFilts) > 0

done

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:86: func (q *Query) Kind(kind string) *Query {
On 2015/09/18 at 16:47:58, dnj wrote:
> Worth doing pre-mod identity checks for these? e.g.,
> if kind != q.kind {
>   ...
> }

Not really. If you have allocation bottlenecks in your query construction,
you're doing it really, really, really wrong. This is already over-optimized.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:208: q.eqFilts = map[string]PropertySlice{}
On 2015/09/18 at 16:47:58, dnj wrote:
> make(..., 1)

has anyone told you that you're the king of warrantless pre-micro-optimization
:)?

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:238: if strings.HasPrefix(field, "__") &&
strings.HasSuffix(field, "__") {
On 2015/09/18 at 16:47:58, dnj wrote:
> Is "_ _ _" (without spaces) reserved? Granted it's a stupid field name...

They're not actually reserved in the underlying thing, but I don't want
conflation with "__ancestor__", and I may need additional things later. If it
becomes a conflict for something, we can change this logic to a whitelist... or
something. don't overthink it :p

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:266: if err == nil && q.ineqFiltHighSet {
On 2015/09/18 at 16:47:59, dnj wrote:
> IMO the auto-adjustment is convenient, but it should be an error to set
multiple inequality filters on a Query.

yep, it does in q.ineqOK (which modifies q.err as a side effect)

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:362: func (q *Query) RevertFilters() *Query {
On 2015/09/18 at 16:47:58, dnj wrote:
> ClearFilters

done for all

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/qu...
service/datastore/query.go:557: }
On 2015/09/18 at 16:47:58, dnj wrote:
> ineqFiltHighSet?

oops

[dnj, 2015-09-18 22:45:13]

lgtm % comments

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
File service/datastore/finalized_query.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
service/datastore/finalized_query.go:55: ret := make([]string, len(q.project))
On 2015/09/18 22:25:48, iannucci wrote:
> On 2015/09/18 at 16:47:57, dnj wrote:
> > Maybe an allocation-avoiding shortcut here:
> > if len(q.project) == 0 {
> >   return nil
> > }
> 
> done

I think you missed this.

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
File service/datastore/key.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
service/datastore/key.go:331: if k.appID < other.appID {
On 2015/09/18 22:25:48, iannucci wrote:
> On 2015/09/18 at 16:47:58, dnj wrote:
> > Slightly better to avoid double string compare and do:
> > cmp := strings.Compare(k.appID, other.appID)
> 
> Which doesn't exist in 1.4 and whose 1.5 implementation does the exact same
> thing as I do here: http://golang.org/src/strings/compare.go?s=491:520#L3
> 
> > switch cmp {
> >   case -1:
> >     return true
> >   case 1:
> >     return false
> > }
> > 
> > "" for other string comparisons here.
> 

>< boo @Go. IMO use it anyway, they could optimize later.

https://chromiumcodereview.appspot.com/1355783002/diff/20001/impl/prod/datast...
File impl/prod/datastore_key.go (right):

https://chromiumcodereview.appspot.com/1355783002/diff/20001/impl/prod/datast...
impl/prod/datastore_key.go:35: count--
noice

[iannucci, 2015-09-19 02:06:15]

On 2015/09/18 at 22:45:13, dnj wrote:
> lgtm % comments
> 
>
https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
> File service/datastore/finalized_query.go (right):
> 
>
https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
> service/datastore/finalized_query.go:55: ret := make([]string, len(q.project))
> On 2015/09/18 22:25:48, iannucci wrote:
> > On 2015/09/18 at 16:47:57, dnj wrote:
> > > Maybe an allocation-avoiding shortcut here:
> > > if len(q.project) == 0 {
> > >   return nil
> > > }
> > 
> > done
> 
> I think you missed this.
> 
>
https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
> File service/datastore/key.go (right):
> 
>
https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
> service/datastore/key.go:331: if k.appID < other.appID {
> On 2015/09/18 22:25:48, iannucci wrote:
> > On 2015/09/18 at 16:47:58, dnj wrote:
> > > Slightly better to avoid double string compare and do:
> > > cmp := strings.Compare(k.appID, other.appID)
> > 
> > Which doesn't exist in 1.4 and whose 1.5 implementation does the exact same
> > thing as I do here: http://golang.org/src/strings/compare.go?s=491:520#L3
> > 
> > > switch cmp {
> > >   case -1:
> > >     return true
> > >   case 1:
> > >     return false
> > > }
> > > 
> > > "" for other string comparisons here.
> > 
> 
> >< boo @Go. IMO use it anyway, they could optimize later.

but... we're still on 1.4 :D

> 
>
https://chromiumcodereview.appspot.com/1355783002/diff/20001/impl/prod/datast...
> File impl/prod/datastore_key.go (right):
> 
>
https://chromiumcodereview.appspot.com/1355783002/diff/20001/impl/prod/datast...
> impl/prod/datastore_key.go:35: count--
> noice

[iannucci, 2015-09-19 02:08:06]

On 2015/09/19 at 02:06:15, iannucci wrote:
> On 2015/09/18 at 22:45:13, dnj wrote:
> > lgtm % comments
> > 
> >
https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
> > File service/datastore/finalized_query.go (right):
> > 
> >
https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/fi...
> > service/datastore/finalized_query.go:55: ret := make([]string,
len(q.project))
> > On 2015/09/18 22:25:48, iannucci wrote:
> > > On 2015/09/18 at 16:47:57, dnj wrote:
> > > > Maybe an allocation-avoiding shortcut here:
> > > > if len(q.project) == 0 {
> > > >   return nil
> > > > }
> > > 
> > > done
> > 
> > I think you missed this.

Hm.. it shows up for me?

> > 
> >
https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
> > File service/datastore/key.go (right):
> > 
> >
https://chromiumcodereview.appspot.com/1355783002/diff/1/service/datastore/ke...
> > service/datastore/key.go:331: if k.appID < other.appID {
> > On 2015/09/18 22:25:48, iannucci wrote:
> > > On 2015/09/18 at 16:47:58, dnj wrote:
> > > > Slightly better to avoid double string compare and do:
> > > > cmp := strings.Compare(k.appID, other.appID)
> > > 
> > > Which doesn't exist in 1.4 and whose 1.5 implementation does the exact
same
> > > thing as I do here: http://golang.org/src/strings/compare.go?s=491:520#L3
> > > 
> > > > switch cmp {
> > > >   case -1:
> > > >     return true
> > > >   case 1:
> > > >     return false
> > > > }
> > > > 
> > > > "" for other string comparisons here.
> > > 
> > 
> > >< boo @Go. IMO use it anyway, they could optimize later.
> 
> but... we're still on 1.4 :D
> 
> > 
> >
https://chromiumcodereview.appspot.com/1355783002/diff/20001/impl/prod/datast...
> > File impl/prod/datastore_key.go (right):
> > 
> >
https://chromiumcodereview.appspot.com/1355783002/diff/20001/impl/prod/datast...
> > impl/prod/datastore_key.go:35: count--
> > noice

[iannucci, 2015-09-19 02:09:46]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-09-19 02:09:46]

The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org
Link to the patchset: https://codereview.chromium.org/1355783002/#ps40001
(title: "golint")

[commit-bot: I haz the power, 2015-09-19 02:10:00]

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

[commit-bot: I haz the power, 2015-09-19 02:12:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-19 02:12:47]

Try jobs failed on following builders:
  Luci-GAE Linux Trusty 64 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-GAE%20Linux%20Trus...)
  Luci-GAE Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-GAE%20Presubmit/bu...)

[iannucci, 2015-09-19 05:10:10]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-09-19 05:10:10]

The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org
Link to the patchset: https://codereview.chromium.org/1355783002/#ps60001
(title: "appease errcheck")

[commit-bot: I haz the power, 2015-09-19 05:10:21]

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

[commit-bot: I haz the power, 2015-09-19 05:11:58]

Committed patchset #4 (id:60001) as
https://github.com/luci/gae/commit/f8e86bf226d6985065d19449e21d63398be39a67