datastore: add omitempty support for time.Time - within isEmptyValue() + updated unit tests #131

trakhimenok · 2018-04-11T23:22:19Z

There are 4 advantages of this change:

Saves datastore space (and cost).
Should slightly decrease write time & write ops (and costs) for entities with empty indexed time properties.
Avoids noise in the datastore viewer in the form of 0001-01-01 00:00:00 timestamps.
Eliminates need to implement PropertyLoadSaver to achieve the same.

This change modifies func isEmptyValue(). This requires additional type assertion but only for time.Time fields with omitempty label. Ideally isEmptyValue() should be removed completely as it is called just in 1 place inside func saveStructProperty() and it can be a bit optimized. But that would be a too big change and pros are not too big so I believe this can/should be done as a separate PR if desired.

Closes #98

Alternative to PR #101 that seems to get abandoned and has no unit tests.

googlebot · 2018-04-11T23:22:24Z

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

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

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

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-04-11T23:22:25Z

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

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

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

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

trakhimenok · 2018-04-11T23:25:23Z

I signed it!

googlebot · 2018-04-11T23:25:28Z

CLAs look good, thanks!

googlebot · 2018-04-11T23:25:28Z

CLAs look good, thanks!

trakhimenok · 2018-04-11T23:33:03Z

@sbuss can you please take a look to this PR?

For some reasons CI fails but seems the issue is not related to the change. Related tests are passing:

=== RUN   TestRoundTrip
--- PASS: TestRoundTrip (0.58s)
=== RUN   TestSaveStructOmitEmpty
--- PASS: TestSaveStructOmitEmpty (0.00s)

mtraver · 2018-04-12T18:30:03Z

datastore/prop_test.go

+	// }{
+	// 	NonEmptyValue:      1,
+	// 	OmitEmptyWithValue: 2,
+	// })


Remove this and add a TODO or note in isEmptyValue.

mtraver · 2018-04-12T18:30:12Z

datastore/prop_test.go

+		} else {
+			expected := make([]string, len(expectedPropNames))
+			copy(expected, expectedPropNames)
+		PROPS:


I think it would be more readable to sort expectedPropNames and props and then compare with a single loop.

sort.Slice(props, func(i, j int) bool { return props[i].Name < props[j].Name }) for i, _ := range props { if props[i].Name != expectedPropNames[i] { t.Errorf("Unexpected property: %v", props[i].Name) } }

mtraver · 2018-04-12T18:30:19Z

datastore/prop_test.go

+func TestSaveStructOmitEmpty(t *testing.T) {
+	expectedPropNames := []string{"EmptyValue", "NonEmptyValue", "OmitEmptyWithValue"}
+
+	assert := func(src interface{}) {


Can this be more descriptive, e.g. testFieldsOmitted?

And by "this" I mean assert. This snippet shows more lines than I selected.

Round tripping a time.Time can result in a different time.Location: Local instead of UTC.

trakhimenok · 2018-04-12T22:22:18Z

@mtraver, I've addressed your comments, extended tests and fixed build - tests are passing.

Let me know if I should squash commits or you can do it on merging the PR.

mtraver · 2018-04-16T16:58:10Z

datastore/prop_test.go

+			for i := range props {
+				actualPropNames[i] = props[i].Name
+			}
+			// and sort actuals for comparing with already sorted expected names


nit: // Sort actuals for comparing with already sorted expected names

mtraver · 2018-04-16T16:58:21Z

datastore/save.go

 func isEmptyValue(v reflect.Value) bool {
 	switch v.Kind() {
 	case reflect.Array, reflect.Map, reflect.Slice, reflect.String:
+		// TODO(perfomance): Only relect.String needed, other property types are not supported (copy/paste from json package)


nit: relect -> reflect

mtraver · 2018-04-16T16:58:24Z

datastore/save.go

 		return v.Len() == 0
 	case reflect.Bool:
 		return !v.Bool()
 	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
 		return v.Int() == 0
 	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
+		// TODO(perfomance): Uint* are unsuported property types - should be removed (copy/paste from json package)


nit: unsuported -> unsupported

mtraver · 2018-04-16T16:59:32Z

Just a few nits, but otherwise looks good.

@sbuss, @broady, could one of you review as well?

sbuss · 2018-04-17T19:00:27Z

@adams-sarah Can you take a look at this, please?

trakhimenok · 2018-04-20T09:22:57Z

@mtraver may be you can send formal review request to @adams-sarah as @sbuss asked her to review?

mtraver · 2018-04-20T17:24:16Z

Done

zombiezen · 2018-04-25T15:50:20Z

@jba, would you mind taking a look? This change probably also needs to be cherry picked into datastore for GoogleCloudPlatform/google-cloud-go.

jba · 2018-04-26T20:23:29Z

Thanks for the heads-up. Done in https://code-review.googlesource.com/#/c/gocloud/+/26910.

time.Time fields now support the omitempty tag. A zero time.Time will not be saved. Mirrors the corresponding change in the AppEngine datastore client, from golang/appengine#131. Change-Id: I3ca875ecff865f3b533305091b86bfaec9a135fc Reviewed-on: https://code-review.googlesource.com/26910 Reviewed-by: kokoro <noreply+kokoro@google.com> Reviewed-by: Jean de Klerk <deklerk@google.com>

trakhimenok · 2018-04-27T12:09:02Z

The change was merged in https://code-review.googlesource.com/c/gocloud/+/26910

@jba, @zombiezen can we approve/merge here as well?

datastore: add omitempty support for time.Time

e657b25

mtraver reviewed Apr 12, 2018

View reviewed changes

trakhimenok added 4 commits April 12, 2018 21:53

datastore: add omitempty support for time.Time (followup on review)

7181216

datastore: add omitempty support for time.Time - no t.Helper() v1.6-1.8

5754d91

datastore: add omitempty support for time.Time - use sort.StringSlice

0c8dce7

datastore: add omitempty support for time.Time - rm Time @ NoOmit test

5008077

Round tripping a time.Time can result in a different time.Location: Local instead of UTC.

mtraver reviewed Apr 16, 2018

View reviewed changes

datastore: add omitempty support for time.Time - comments nits & grammar

7305775

mtraver approved these changes Apr 16, 2018

View reviewed changes

mtraver requested review from sbuss and broady April 16, 2018 20:32

mtraver requested a review from s-mang April 20, 2018 17:24

s-mang requested review from zombiezen and removed request for s-mang April 25, 2018 02:19

mtraver merged commit 962cbd1 into golang:master Apr 27, 2018

trakhimenok mentioned this pull request Apr 27, 2018

datastore: add omitempty support for time.Time #101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

datastore: add omitempty support for time.Time - within isEmptyValue() + updated unit tests #131

datastore: add omitempty support for time.Time - within isEmptyValue() + updated unit tests #131

trakhimenok commented Apr 11, 2018

googlebot commented Apr 11, 2018

googlebot commented Apr 11, 2018

trakhimenok commented Apr 11, 2018

googlebot commented Apr 11, 2018

googlebot commented Apr 11, 2018

trakhimenok commented Apr 11, 2018

mtraver Apr 12, 2018 •

edited

mtraver Apr 12, 2018

mtraver Apr 12, 2018

mtraver Apr 12, 2018

trakhimenok commented Apr 12, 2018

mtraver Apr 16, 2018

mtraver Apr 16, 2018

mtraver Apr 16, 2018

mtraver commented Apr 16, 2018

sbuss commented Apr 17, 2018

trakhimenok commented Apr 20, 2018

mtraver commented Apr 20, 2018

zombiezen commented Apr 25, 2018

jba commented Apr 26, 2018

trakhimenok commented Apr 27, 2018

datastore: add omitempty support for time.Time - within isEmptyValue() + updated unit tests #131

datastore: add omitempty support for time.Time - within isEmptyValue() + updated unit tests #131

Conversation

trakhimenok commented Apr 11, 2018

googlebot commented Apr 11, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Apr 11, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

trakhimenok commented Apr 11, 2018

googlebot commented Apr 11, 2018

googlebot commented Apr 11, 2018

trakhimenok commented Apr 11, 2018

mtraver Apr 12, 2018 • edited

Choose a reason for hiding this comment

mtraver Apr 12, 2018

Choose a reason for hiding this comment

mtraver Apr 12, 2018

Choose a reason for hiding this comment

mtraver Apr 12, 2018

Choose a reason for hiding this comment

trakhimenok commented Apr 12, 2018

mtraver Apr 16, 2018

Choose a reason for hiding this comment

mtraver Apr 16, 2018

Choose a reason for hiding this comment

mtraver Apr 16, 2018

Choose a reason for hiding this comment

mtraver commented Apr 16, 2018

sbuss commented Apr 17, 2018

trakhimenok commented Apr 20, 2018

mtraver commented Apr 20, 2018

zombiezen commented Apr 25, 2018

jba commented Apr 26, 2018

trakhimenok commented Apr 27, 2018

mtraver Apr 12, 2018 •

edited