impl/memory: Fix time serialization encoding.

impl/memory: Fix time serialization encoding.

In a PropertyMap, time is serialized as an integer type. However, for
queries, time is serialized as a time type. This causes all queries on
time fields to select any encoded time properties, since PTTime > PTInt.

This patch causes queries to serialize PTTime properties as PTInts to
match the indexes.

Fixes #34.

Committed: https://github.com/luci/gae/commit/21185c790cd5f81b3a9378109e23aab22bd99356

[dnj, 2015-12-29 07:23:15]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org, vadimsh@chromium.org

[dnj, 2015-12-29 07:23:16]

PTAL. I'm not sure if this is the best fix, but I think it might be.

Integer time properties are coaxed back into a time.Time type at a higher level
by datastore when they are applied to structs.

[dnj, 2015-12-29 16:48:09]

https://chromiumcodereview.appspot.com/1550903002/diff/1/service/datastore/se...
File service/datastore/serialize/serialize.go (right):

https://chromiumcodereview.appspot.com/1550903002/diff/1/service/datastore/se...
service/datastore/serialize/serialize.go:233: case ds.PTTime:
I made this a switch statement in case there were other instances of data types
that made serialization-level type changes. If it's just PTTime, I could
simplify this, although probably with little gain.

[iannucci, 2015-12-29 18:28:56]

On 2015/12/29 at 16:48:09, dnj wrote:
>
https://chromiumcodereview.appspot.com/1550903002/diff/1/service/datastore/se...
> File service/datastore/serialize/serialize.go (right):
> 
>
https://chromiumcodereview.appspot.com/1550903002/diff/1/service/datastore/se...
> service/datastore/serialize/serialize.go:233: case ds.PTTime:
> I made this a switch statement in case there were other instances of data
types that made serialization-level type changes. If it's just PTTime, I could
simplify this, although probably with little gain.

not lgtm, this will cause data corruption (times will no longer round trip from
the datastore).

the bug here is that when we generate the indexes, they should be projecting all
PTTime's to PTInt's first. See the little equality chart at the bottom of
https://cloud.google.com/appengine/docs/go/datastore/entities#Go_Value_type_s....

[iannucci, 2015-12-29 18:29:20]

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

https://chromiumcodereview.appspot.com/1550903002/diff/1/impl/memory/datastor...
impl/memory/datastore_query_execution_test.go:408: indx("Kind!", "Date"),
I think ! is shorthand for ancestor here, which you don't need?

https://chromiumcodereview.appspot.com/1550903002/diff/1/service/datastore/se...
File service/datastore/serialize/serialize.go (right):

https://chromiumcodereview.appspot.com/1550903002/diff/1/service/datastore/se...
service/datastore/serialize/serialize.go:236: typb := byte(effectiveType)
This change is not good; it will call property maps to fail to round-trip to the
datastore.

[dnj, 2015-12-29 19:28:05]

Description was changed from

==========
impl/memory: Fix time serialization encoding.

In a PropertyMap, time is serialized as an integer type. However, for
queries, time is serialized as a time type. This causes all queries on
time fields to select any encoded time properties, since PTTime > PTInt.

This patch causes serialize.ToBytes to attach a PTInt label rather than
a PTTime label when generating the byte sequence for a PTTime property.
It also fixes up associated test expectations and adds a test for time
inequality queries.

Fixes #34.
==========

to

==========
impl/memory: Fix time serialization encoding.

In a PropertyMap, time is serialized as an integer type. However, for
queries, time is serialized as a time type. This causes all queries on
time fields to select any encoded time properties, since PTTime > PTInt.

This patch causes queries to serialize PTTime properties as PTInts to
match the indexes.

Fixes #34.
==========

[dnj, 2015-12-29 19:28:14]

Updated, PTAL

[iannucci, 2015-12-29 19:53:46]

looks much better, but I think there's still a bug for equalities, and the
byte-string-type things.

https://chromiumcodereview.appspot.com/1550903002/diff/20001/impl/memory/data...
File impl/memory/datastore_query.go (right):

https://chromiumcodereview.appspot.com/1550903002/diff/20001/impl/memory/data...
impl/memory/datastore_query.go:110: // them as PTInt in the serialized query.
could you add another case for all the various byte-type things (blobstore keys,
[]byte, string) all get the same collapsing behavior?

https://chromiumcodereview.appspot.com/1550903002/diff/20001/impl/memory/data...
impl/memory/datastore_query.go:120: // GetBinaryBounds gets the binary encoding
of the upper and lower bounds of
this bug probably also affects equality comparisons. See line 198

[dnj, 2015-12-29 21:38:52]

Patchset #3 (id:40001) has been deleted

[dnj, 2015-12-29 21:40:47]

Added blobstore.Key, restructured a tad. PTAL.

https://chromiumcodereview.appspot.com/1550903002/diff/20001/impl/memory/data...
File impl/memory/datastore_query.go (right):

https://chromiumcodereview.appspot.com/1550903002/diff/20001/impl/memory/data...
impl/memory/datastore_query.go:110: // them as PTInt in the serialized query.
On 2015/12/29 19:53:46, iannucci (OOO till Jan 4) wrote:
> could you add another case for all the various byte-type things (blobstore
keys,
> []byte, string) all get the same collapsing behavior?

Sure. AFAICT this only additionally affected blobstore keys.

Since the logic is implemented in serialize, I moved this function into
serialize as well.

https://chromiumcodereview.appspot.com/1550903002/diff/20001/impl/memory/data...
impl/memory/datastore_query.go:120: // GetBinaryBounds gets the binary encoding
of the upper and lower bounds of
On 2015/12/29 19:53:46, iannucci (OOO till Jan 4) wrote:
> this bug probably also affects equality comparisons. See line 198

yarp, nice catch. Added test case and fix.

https://chromiumcodereview.appspot.com/1550903002/diff/60001/service/datastor...
File service/datastore/properties.go (right):

https://chromiumcodereview.appspot.com/1550903002/diff/60001/service/datastor...
service/datastore/properties.go:250: func RoundTime(t time.Time) time.Time {
I'm adding this and exporting it b/c this is something generally useful to
datastore consumers. For example, if I generate a struct w/ a time.Time, copy it
to datastore, load it, and confirm that it matches, this must be applied to the
initial struct's time field.

[iannucci, 2015-12-29 21:41:49]

https://chromiumcodereview.appspot.com/1550903002/diff/60001/service/datastor...
File service/datastore/serialize/serialize.go (right):

https://chromiumcodereview.appspot.com/1550903002/diff/60001/service/datastor...
service/datastore/serialize/serialize.go:291: //   - PTBlobKey is converted to a
string.
and PTBytes too?

[dnj, 2015-12-29 23:04:16]

Much nicer now.

[iannucci, 2015-12-30 23:12:17]

On 2015/12/29 at 23:04:16, dnj wrote:
> Much nicer now.

lgtm, though if we could avoid introducing extra copies, that would be nice.

Oh, derp... yeah I remembered implementing an index conversion function...
that's embarrassing that I forgot to call it :(

[iannucci, 2015-12-30 23:12:24]

https://chromiumcodereview.appspot.com/1550903002/diff/80001/service/datastor...
File service/datastore/serialize/serialize.go (right):

https://chromiumcodereview.appspot.com/1550903002/diff/80001/service/datastor...
service/datastore/serialize/serialize.go:245: p = p.ForIndex()
I think this may end up doing unnecessary copies, e.g. if you switch from []byte
to string, which is why I had the full listing below.

[dnj, 2015-12-31 03:29:24]

Patchset #5 (id:100001) has been deleted

[iannucci, 2015-12-31 23:22:34]

Haha cool :P

WDYT about the suggestion re: Value() below? I don't think it's actually
necessary, especially if you keep ForIndex the same as it was.

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

https://chromiumcodereview.appspot.com/1550903002/diff/140001/service/datasto...
service/datastore/properties.go:181: value interface{}
I would note particularly:

  * all byte sequence types are stored as byteSequence implementations
  * time is stored as int64

https://chromiumcodereview.appspot.com/1550903002/diff/140001/service/datasto...
service/datastore/properties.go:308: func (p *Property) Value() (v interface{})
{
you should now note that Value may return a copy of the data that's in this
Property. Previously the application could continue to 'own' the data in the
Property. E.g. if this returns []byte, and you modify it, it doesn't
(necessarily) change the underlying value.

https://chromiumcodereview.appspot.com/1550903002/diff/140001/service/datasto...
service/datastore/properties.go:397: func (p Property) ForIndex() (PropertyType,
interface{}) {
why change the signature?

https://chromiumcodereview.appspot.com/1550903002/diff/140001/service/datasto...
service/datastore/properties.go:406: return PTString,
p.value.(byteSequence).Value()
why is .Value() necessary? Shouldn't it be .String() anyway?

Note that if you don't change the `ForIndex()` signature, you can just return a
new Property that's a copy of the old one, but with the propType changed (to
avoid copying the actual bytes data).

https://chromiumcodereview.appspot.com/1550903002/diff/140001/service/datasto...
service/datastore/properties.go:442: case pt == PTBytes || pt == PTString || pt
== PTBlobKey:
if you switch pt, this could be `case PTBytes, PTString, PTBlobKey`

[dnj, 2016-01-01 17:43:30]

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

https://chromiumcodereview.appspot.com/1550903002/diff/140001/service/datasto...
service/datastore/properties.go:181: value interface{}
On 2015/12/31 23:22:34, iannucci (OOO till Jan 4) wrote:
> I would note particularly:
> 
>   * all byte sequence types are stored as byteSequence implementations
>   * time is stored as int64

Done.

https://chromiumcodereview.appspot.com/1550903002/diff/140001/service/datasto...
service/datastore/properties.go:308: func (p *Property) Value() (v interface{})
{
On 2015/12/31 23:22:34, iannucci (OOO till Jan 4) wrote:
> you should now note that Value may return a copy of the data that's in this
> Property. Previously the application could continue to 'own' the data in the
> Property. E.g. if this returns []byte, and you modify it, it doesn't
> (necessarily) change the underlying value.

I don't think that's the case. For example, PTByte was installed via []byte
-(SetValue)> bytesByteSequence -(Value)> []byte. It should still be the same
underlying byte array. PTString/PTBlobKey same deal. PTTime was always copied,
since it's passed by-value.

https://chromiumcodereview.appspot.com/1550903002/diff/140001/service/datasto...
service/datastore/properties.go:397: func (p Property) ForIndex() (PropertyType,
interface{}) {
On 2015/12/31 23:22:34, iannucci (OOO till Jan 4) wrote:
> why change the signature?

I was reluctant to export PTString types with []byte contents because it would
create an "always-copy" Property instance. Value(), when called by serialize,
would copy to a string. To avoid that, I'd need a "IndexValue" method that
doesn't do the conversion, and since index generation is internal-ish anyway
this seemed like a less publicly-visible approach.

https://chromiumcodereview.appspot.com/1550903002/diff/140001/service/datasto...
service/datastore/properties.go:406: return PTString,
p.value.(byteSequence).Value()
On 2015/12/31 23:22:34, iannucci (OOO till Jan 4) wrote:
> why is .Value() necessary? Shouldn't it be .String() anyway?

Don't want to return .String() b/c that would induce a copy if the underlying
type is []byte. By maintaining the underlying type, we get zero-copy return
value here. Instead, we return "PTString, []byte".

Another option is to export "byteSequence" and have users handle it, but I think
that's a lot of pain for little gain. ATM we have "ForIndex" for "internal-ish"
index generation, used basically by "serialize", and we have "Value" for public
stuff, which will remain zero-copy and native. Neither is really confusing or
abusable in its current state.

> Note that if you don't change the `ForIndex()` signature, you can just return
a
> new Property that's a copy of the old one, but with the propType changed (to
> avoid copying the actual bytes data).

That was actually the original formulation. Looked good in the code, but see
above for thoughts.

https://chromiumcodereview.appspot.com/1550903002/diff/140001/service/datasto...
service/datastore/properties.go:442: case pt == PTBytes || pt == PTString || pt
== PTBlobKey:
On 2015/12/31 23:22:34, iannucci (OOO till Jan 4) wrote:
> if you switch pt, this could be `case PTBytes, PTString, PTBlobKey`

Done.

[iannucci, 2016-01-02 04:11:07]

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

https://chromiumcodereview.appspot.com/1550903002/diff/160001/service/datasto...
service/datastore/properties.go:411: return PTString,
p.value.(byteSequence).Value()
but this is still wrong... if it's PTBytes, this will return (PTString, []byte),
but if it's PTSTring, it will return (PTString, string)...

[iannucci, 2016-01-02 04:38:29]

On 2016/01/02 at 04:11:07, iannucci (OOO till Jan 4) wrote:
>
https://chromiumcodereview.appspot.com/1550903002/diff/160001/service/datasto...
> File service/datastore/properties.go (right):
> 
>
https://chromiumcodereview.appspot.com/1550903002/diff/160001/service/datasto...
> service/datastore/properties.go:411: return PTString,
p.value.(byteSequence).Value()
> but this is still wrong... if it's PTBytes, this will return (PTString,
[]byte), but if it's PTSTring, it will return (PTString, string)...

(unless I'm horribly mistaken :D)

[dnj, 2016-01-02 06:39:46]

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

https://chromiumcodereview.appspot.com/1550903002/diff/160001/service/datasto...
service/datastore/properties.go:411: return PTString,
p.value.(byteSequence).Value()
On 2016/01/02 04:11:07, iannucci (OOO till Jan 4) wrote:
> but this is still wrong... if it's PTBytes, this will return (PTString,
[]byte),
> but if it's PTSTring, it will return (PTString, string)...

That is correct, and that's a new and now-desired behavior :) If you notice
inside "serialize", WriteProperty now switches on .(type) instead of .Type() for
the value write.

[iannucci, 2016-01-02 07:18:14]

On 2016/01/02 at 06:39:46, dnj wrote:
>
https://chromiumcodereview.appspot.com/1550903002/diff/160001/service/datasto...
> File service/datastore/properties.go (right):
> 
>
https://chromiumcodereview.appspot.com/1550903002/diff/160001/service/datasto...
> service/datastore/properties.go:411: return PTString,
p.value.(byteSequence).Value()
> On 2016/01/02 04:11:07, iannucci (OOO till Jan 4) wrote:
> > but this is still wrong... if it's PTBytes, this will return (PTString,
[]byte),
> > but if it's PTSTring, it will return (PTString, string)...
> 
> That is correct, and that's a new and now-desired behavior :) If you notice
inside "serialize", WriteProperty now switches on .(type) instead of .Type() for
the value write.

Ew... gross... I don't think I like that :( PT* is supposed to be the actual
type of the property. What's it supposed to mean now?

[dnj, 2016-01-02 17:58:10]

>
https://chromiumcodereview.appspot.com/1550903002/diff/160001/service/datasto...
> > service/datastore/properties.go:411: return PTString,
> p.value.(byteSequence).Value()
> > On 2016/01/02 04:11:07, iannucci (OOO till Jan 4) wrote:
> > > but this is still wrong... if it's PTBytes, this will return (PTString,
> []byte),
> > > but if it's PTSTring, it will return (PTString, string)...
> > 
> > That is correct, and that's a new and now-desired behavior :) If you notice
> inside "serialize", WriteProperty now switches on .(type) instead of .Type()
for
> the value write.
> 
> Ew... gross... I don't think I like that :( PT* is supposed to be the actual
> type of the property. What's it supposed to mean now?

That's the main reason I changed ForIndex from returning a Property to just
returning information. Right now, ForIndex says, encode this with this type and
these contents as two separate statements, so we don't ever create a mixed-type
property.

[iannucci, 2016-01-03 09:39:05]

On 2016/01/02 at 17:58:10, dnj wrote:
> >
https://chromiumcodereview.appspot.com/1550903002/diff/160001/service/datasto...
> > > service/datastore/properties.go:411: return PTString,
> > p.value.(byteSequence).Value()
> > > On 2016/01/02 04:11:07, iannucci (OOO till Jan 4) wrote:
> > > > but this is still wrong... if it's PTBytes, this will return (PTString,
> > []byte),
> > > > but if it's PTSTring, it will return (PTString, string)...
> > > 
> > > That is correct, and that's a new and now-desired behavior :) If you
notice
> > inside "serialize", WriteProperty now switches on .(type) instead of .Type()
for
> > the value write.
> > 
> > Ew... gross... I don't think I like that :( PT* is supposed to be the actual
> > type of the property. What's it supposed to mean now?
> 
> That's the main reason I changed ForIndex from returning a Property to just
returning information. Right now, ForIndex says, encode this with this type and
these contents as two separate statements, so we don't ever create a mixed-type
property.

So the type returned is the type of the index entry, even if the value isn't the
same type? It's confusing to me that ForIndex can return some type X with
different typed values... returning a byteSequence with a PTString result would
make more sense than sometimes returning []byte and sometimes returning string,
right?

[dnj, 2016-01-03 16:13:13]

> > That's the main reason I changed ForIndex from returning a Property to just
> returning information. Right now, ForIndex says, encode this with this type
and
> these contents as two separate statements, so we don't ever create a
mixed-type
> property.
> 
> So the type returned is the type of the index entry, even if the value isn't
the
> same type? It's confusing to me that ForIndex can return some type X with
> different typed values... returning a byteSequence with a PTString result
would
> make more sense than sometimes returning []byte and sometimes returning
string,
> right?

For zero-copy, something will have to deal with the underlying exposed type and
choose "WriteBytes" vs. "WriteString". Right now it's type-switched off the
ForIndex interface{} value type alone. If we returned a byteSequence, it would
have to be conditioned off byteSequence.Value() (aka return type), always call
String()/Byte() (which can result in copy), or provided through some other
meta-information. That all seems less direct than the current one to me.

Another approach that comes to mind would be to implement "IndexValue()", which
returns the actual underlying value (converting byteSequence to []byte/string,
time stays as int). Then "serialize" can contain the logic to convert PTBytes ->
PTString and PTTime -> PTInt and serialize by type-switching IndexValue(). I
kind of like this more b/c it removes the need for a Property to know how it
will be stored in serialize's index format.

[dnj, 2016-01-04 20:07:26]

Updated as per discussion, PTAL!

[dnj, 2016-01-04 20:07:26]

Updated as per discussion, PTAL!

[iannucci, 2016-01-05 02:02:58]

lgtm

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

https://chromiumcodereview.appspot.com/1550903002/diff/200001/service/datasto...
service/datastore/properties.go:180: // in its original original value via
Value().
too original?

https://chromiumcodereview.appspot.com/1550903002/diff/200001/service/datasto...
service/datastore/properties.go:343: }
y no default?

[dnj, 2016-01-05 02:46:17]

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

https://chromiumcodereview.appspot.com/1550903002/diff/200001/service/datasto...
service/datastore/properties.go:180: // in its original original value via
Value().
On 2016/01/05 02:02:57, iannucci wrote:
> too original?

Everything is in bits.

https://chromiumcodereview.appspot.com/1550903002/diff/200001/service/datasto...
service/datastore/properties.go:343: }
On 2016/01/05 02:02:57, iannucci wrote:
> y no default?

Done.

[dnj, 2016-01-05 03:04:22]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-01-05 03:04:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1550903002/#ps240001 (title: "Comments,
tune-up.")

[commit-bot: I haz the power, 2016-01-05 03:04:36]

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

[commit-bot: I haz the power, 2016-01-05 03:07:28]

Description was changed from

==========
impl/memory: Fix time serialization encoding.

In a PropertyMap, time is serialized as an integer type. However, for
queries, time is serialized as a time type. This causes all queries on
time fields to select any encoded time properties, since PTTime > PTInt.

This patch causes queries to serialize PTTime properties as PTInts to
match the indexes.

Fixes #34.
==========

to

==========
impl/memory: Fix time serialization encoding.

In a PropertyMap, time is serialized as an integer type. However, for
queries, time is serialized as a time type. This causes all queries on
time fields to select any encoded time properties, since PTTime > PTInt.

This patch causes queries to serialize PTTime properties as PTInts to
match the indexes.

Fixes #34.

Committed:
https://github.com/luci/gae/commit/21185c790cd5f81b3a9378109e23aab22bd99356
==========

[commit-bot: I haz the power, 2016-01-05 03:07:29]

Committed patchset #11 (id:240001) as
https://github.com/luci/gae/commit/21185c790cd5f81b3a9378109e23aab22bd99356