Implement projection queries correctly.

Implement projection queries correctly.

* Add Project and ForIndex to Property in service/datastore
* Remove PTBoolFalse/PTBoolTrue silliness (now just PTBool)
* Remove ByteSlice silliness (now []byte behaves exactly like string w.r.t.
  indexing. If you don't want it indexed, put `gae:",noindex"` on it.)
* Make indexes index 'underlying' types. e.g. PTTime is not distinguished from
  PTInt in the index, nor are the various byte sequence types.
* Make impl/prod correctly extract data from internal datastoreIndex objects
  when doing projection queries. This allows doing a projection into
  []PropertyMap. Since all data is bussed internally as PropertyMap (instead of
  crazy PropertyLoadSaver implementations), this is required to make projection
  queries work. I'm pretty confident that this won't break, however, as it's all
  derived from protobufs which are extremely unlikely to change.

  To make this better, we'd have to implement the protobuf protocol directly in
  luci/gae instead of routing through the SDK. Not as absurd as it sounds, but
  definitely out of scope for this CL :).
* Include extremely basic test for impl/prod using aetest. This test takes 12s
  to run on my laptop. *sigh*.

R=dnj@chromium.org, estaab@chromium.org, maruel@chromium.org, tandrii@chromium.org, vadimsh@chromium.org
BUG=https://github.com/luci/gae/issues/1

Committed: https://github.com/luci/gae/commit/9ce8b9442816469d8c4e3c53572d52aaf4d7378e

[iannucci, 2015-09-10 03:56:58]

PTAL (with self-comments!)

https://codereview.chromium.org/1336443003/diff/1/impl/memory/datastore_index.go
File impl/memory/datastore_index.go (right):

https://codereview.chromium.org/1336443003/diff/1/impl/memory/datastore_index...
impl/memory/datastore_index.go:63: func serializeRow(vals []ds.Property)
serializedPvals {
This was a convenient refactoring. It'll become more apparently useful in a
future CL.

https://codereview.chromium.org/1336443003/diff/1/impl/memory/datastore_index...
File impl/memory/datastore_index_test.go (right):

https://codereview.chromium.org/1336443003/diff/1/impl/memory/datastore_index...
impl/memory/datastore_index_test.go:91: var rgenComplexTimeIdx =
prop(rgenComplexTime).ForIndex()
this is actually a Property of type PTInt now.

https://codereview.chromium.org/1336443003/diff/1/impl/memory/datastore_index...
impl/memory/datastore_index_test.go:185: cat(prop("hat"), icat(prop("value")),
prop(true), prop(fakeKey)),
you can see that []byte and string are no longer distinguished in the index.
This is how it works for realsies.

https://codereview.chromium.org/1336443003/diff/1/impl/memory/datastore_query...
File impl/memory/datastore_query_test.go (right):

https://codereview.chromium.org/1336443003/diff/1/impl/memory/datastore_query...
impl/memory/datastore_query_test.go:45:
`gYAAZzFdTeeb3d9zOxsAAF-v221Xy32_AIGHyIgAAUc32-AFabMAAA==`)
This changed because I removed PTBoolFalse, and so the value of PTKey changed.

https://codereview.chromium.org/1336443003/diff/1/impl/prod/raw_datastore_typ...
File impl/prod/raw_datastore_type_converter.go (right):

https://codereview.chromium.org/1336443003/diff/1/impl/prod/raw_datastore_typ...
impl/prod/raw_datastore_type_converter.go:25: func maybeIndexValue(val
interface{}) interface{} {
This bit of grossness is to extract values from this sucker
https://github.com/golang/appengine/blob/master/datastore/load.go#L324 which
actually just holds a value pointing at
https://github.com/golang/appengine/blob/master/internal/datastore/datastore_...

It would be nice if the SDK exposed a method on this to just get its essential
value. At least the SDK doesn't actively block Projection queries loading into a
PropertyList.

https://codereview.chromium.org/1336443003/diff/1/impl/prod/raw_datastore_typ...
impl/prod/raw_datastore_type_converter.go:153: }
This is now the only place where we have to deal with the ByteString v. []byte
split. I think it "seemed" like []byte being noindex by default would be
'convenient', but really it's just a pain in the butt (and it makes structs
harder to read, because the exception is easy to forget).

https://codereview.chromium.org/1336443003/diff/1/service/datastore/pls_impl.go
File service/datastore/pls_impl.go (right):

https://codereview.chromium.org/1336443003/diff/1/service/datastore/pls_impl....
service/datastore/pls_impl.go:151: set := (func(interface{}))(nil)
Now that we have the Project method, this code got a lot simpler. Basically now
we just switch on the type to figure out what to project it to, project to that,
maybe check overflow, and then set the value. I think that technically these
values could be derived in the codec instead of here, but I'm trying to keep
this CL simple for now.

https://codereview.chromium.org/1336443003/diff/1/service/datastore/pls_test.go
File service/datastore/pls_test.go (right):

https://codereview.chromium.org/1336443003/diff/1/service/datastore/pls_test....
service/datastore/pls_test.go:74: B []byte `gae:",noindex"`
[]byte is indexed by default now.

https://codereview.chromium.org/1336443003/diff/1/service/datastore/pls_test....
service/datastore/pls_test.go:94: B []byte
and ByteString is gone.

https://codereview.chromium.org/1336443003/diff/1/service/datastore/propertie...
File service/datastore/properties.go (left):

https://codereview.chromium.org/1336443003/diff/1/service/datastore/propertie...
service/datastore/properties.go:136: // PropertyConverter).
no more divergence, impl/memory does this correctly now.

https://codereview.chromium.org/1336443003/diff/1/service/datastore/propertie...
service/datastore/properties.go:144: // encoding a 0 or 1 as a byte in the
relevant marshalling routines.
No more divergence, PTBool is the real type now.

https://codereview.chromium.org/1336443003/diff/1/service/datastore/propertie...
File service/datastore/properties.go (right):

https://codereview.chromium.org/1336443003/diff/1/service/datastore/propertie...
service/datastore/properties.go:339: //    (only the first 1500 bytes is
indexable)
Surprise! These are all the same thing in the index :)

[dnj (Google), 2015-09-10 16:26:11]

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

[dnj (Google), 2015-09-10 16:26:12]

Generally lgtm, I really really really don't like the reflection approach, but
that's what you get for trying to wrap an unfriendly package :(

https://codereview.chromium.org/1336443003/diff/1/service/datastore/pls_test.go
File service/datastore/pls_test.go (left):

https://codereview.chromium.org/1336443003/diff/1/service/datastore/pls_test....
service/datastore/pls_test.go:1278: DSBS: ByteString("nerds"),
aaaaaaaaa

https://codereview.chromium.org/1336443003/diff/40001/impl/memory/datastore_i...
File impl/memory/datastore_index.go (right):

https://codereview.chromium.org/1336443003/diff/40001/impl/memory/datastore_i...
impl/memory/datastore_index.go:64: dups := map[string]struct{}{}
https://github.com/luci/luci-go/tree/master/common/stringset ?

https://codereview.chromium.org/1336443003/diff/40001/impl/prod/raw_datastore...
File impl/prod/raw_datastore_type_converter.go (right):

https://codereview.chromium.org/1336443003/diff/40001/impl/prod/raw_datastore...
impl/prod/raw_datastore_type_converter.go:25: func maybeIndexValue(val
interface{}) interface{} {
*sobs*

https://codereview.chromium.org/1336443003/diff/40001/service/datastore/pls_i...
File service/datastore/pls_impl.go (right):

https://codereview.chromium.org/1336443003/diff/40001/service/datastore/pls_i...
service/datastore/pls_impl.go:168: case reflect.Interface:
Before you were checking if the type was Key. Is being a reflect.Interface
sufficient to assume it's a Key now, or should that check be re-added?

https://codereview.chromium.org/1336443003/diff/40001/service/datastore/prope...
File service/datastore/properties.go (right):

https://codereview.chromium.org/1336443003/diff/40001/service/datastore/prope...
service/datastore/properties.go:392: //   PTInt <-> PTTime
Note that you can project a type to itself (identity) as well.

https://codereview.chromium.org/1336443003/diff/40001/service/datastore/seria...
File service/datastore/serialize/serialize.go (right):

https://codereview.chromium.org/1336443003/diff/40001/service/datastore/seria...
service/datastore/serialize/serialize.go:270: if b == 0 {
val = (b != 0)

[iannucci, 2015-09-10 17:29:21]

https://codereview.chromium.org/1336443003/diff/1/service/datastore/pls_test.go
File service/datastore/pls_test.go (left):

https://codereview.chromium.org/1336443003/diff/1/service/datastore/pls_test....
service/datastore/pls_test.go:1278: DSBS: ByteString("nerds"),
On 2015/09/10 16:26:11, dnj (Google) wrote:
> aaaaaaaaa

?

https://codereview.chromium.org/1336443003/diff/40001/impl/memory/datastore_i...
File impl/memory/datastore_index.go (right):

https://codereview.chromium.org/1336443003/diff/40001/impl/memory/datastore_i...
impl/memory/datastore_index.go:64: dups := map[string]struct{}{}
On 2015/09/10 16:26:11, dnj (Google) wrote:
> https://github.com/luci/luci-go/tree/master/common/stringset ?

Yeah, I have a CL for that here:
https://chromiumcodereview.appspot.com/1312433013/

https://codereview.chromium.org/1336443003/diff/40001/impl/memory/datastore_i...
impl/memory/datastore_index.go:64: dups := map[string]struct{}{}
On 2015/09/10 16:26:11, dnj (Google) wrote:
> https://github.com/luci/luci-go/tree/master/common/stringset ?

Yeah, I have a CL for that here:
https://chromiumcodereview.appspot.com/1312433013/

https://codereview.chromium.org/1336443003/diff/40001/impl/prod/raw_datastore...
File impl/prod/raw_datastore_type_converter.go (right):

https://codereview.chromium.org/1336443003/diff/40001/impl/prod/raw_datastore...
impl/prod/raw_datastore_type_converter.go:25: func maybeIndexValue(val
interface{}) interface{} {
On 2015/09/10 16:26:11, dnj (Google) wrote:
> *sobs*

¯\_(ツ)_/¯

https://codereview.chromium.org/1336443003/diff/40001/service/datastore/pls_i...
File service/datastore/pls_impl.go (right):

https://codereview.chromium.org/1336443003/diff/40001/service/datastore/pls_i...
service/datastore/pls_impl.go:168: case reflect.Interface:
On 2015/09/10 16:26:11, dnj (Google) wrote:
> Before you were checking if the type was Key. Is being a reflect.Interface
> sufficient to assume it's a Key now, or should that check be re-added?

No I wasn't. I was casting the property value to a Key. Projecting to PTKey is
the same thing. If this interface was anything else, the codec would have a
permanent error (or this field would have been handled above by doConversion).

https://codereview.chromium.org/1336443003/diff/40001/service/datastore/prope...
File service/datastore/properties.go (right):

https://codereview.chromium.org/1336443003/diff/40001/service/datastore/prope...
service/datastore/properties.go:392: //   PTInt <-> PTTime
On 2015/09/10 16:26:11, dnj (Google) wrote:
> Note that you can project a type to itself (identity) as well.

Oh, right. Done.

https://codereview.chromium.org/1336443003/diff/40001/service/datastore/seria...
File service/datastore/serialize/serialize.go (right):

https://codereview.chromium.org/1336443003/diff/40001/service/datastore/seria...
service/datastore/serialize/serialize.go:270: if b == 0 {
On 2015/09/10 16:26:11, dnj (Google) wrote:
> val = (b != 0)

yay!

[dnj, 2015-09-10 17:32:21]

https://codereview.chromium.org/1336443003/diff/1/service/datastore/pls_test.go
File service/datastore/pls_test.go (left):

https://codereview.chromium.org/1336443003/diff/1/service/datastore/pls_test....
service/datastore/pls_test.go:1278: DSBS: ByteString("nerds"),
On 2015/09/10 17:29:21, iannucci wrote:
> On 2015/09/10 16:26:11, dnj (Google) wrote:
> > aaaaaaaaa
> 
> ?

Just offering alternative stip@-style naming schemes.

[iannucci, 2015-09-10 17:37:38]

haha got it :)

[iannucci, 2015-09-10 17:37:46]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-09-10 17:37:46]

The patchset sent to the CQ was uploaded after l-g-t-m from dnj@google.com
Link to the patchset: https://codereview.chromium.org/1336443003/#ps60001
(title: "fix comments")

[commit-bot: I haz the power, 2015-09-10 17:37:58]

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

[commit-bot: I haz the power, 2015-09-10 17:40:43]

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