Handling of empty string to datetime conversion

[srfrog]

This PR fixes an issue when converting empty string to datetime value.

• When altering a schema from string to datetime that has empty strings, Dgraph fails because it doesn't know how to convert empty string values to datetime.
• Int and float zero values are converted to zero-time value, and zero when converted back.
• Conversions from string to datetime use zero time value.
• New tests added for Convert cases.

New behavior

int: convert 0 to datetime -> zero time
float: convert 0 to datetime -> zero time
string: convert "" to datetime -> zero time
binary: convert '' to datetime -> zero time

datetime: convert zero time to int -> 0
datetime: convert zero time to float -> 0
datetime: convert zero-time to string -> ""
datetime: convert zero-time to binary -> ''

Closes #2310

---

This change is 

[manishrjain]

int: convert 0 to datetime -> epoch
float: convert 0 to datetime -> epoch
string: convert "" to datetime -> zero time

I'd argue that they should all be zero time. Zero int, Zero float and empty string are default values for each of these types, and they should default to empty time{} struct, which is Zero time.

Looks mostly good. Except this change.

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @srfrog and @manishrjain)

---

types/conversion.go, line 223 at r1 (raw file):

			case BinaryID:
				if vc {
					*res = []byte{1}

How about:

*res = []byte{0}
if vc { *res = []byte{1} }

Avoids the break statements. Here and below.

---

types/conversion_test.go, line 526 at r1 (raw file):

		// 0
		{in: Val{Tid: IntID, Value: []byte{0, 0, 0, 0, 0, 0, 0, 0}},
			out: Val{Tid: DateTimeID, Value: time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC)}},

This should be Zero time.

---

types/conversion_test.go, line 548 at r1 (raw file):

		// 0
		{in: Val{Tid: FloatID, Value: []byte{0, 0, 0, 0, 0, 0, 0, 0}},
			out: Val{Tid: DateTimeID, Value: time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC)}},

Should be Zero time.

[srfrog]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain)

---

types/conversion.go, line 223 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

How about:

*res = []byte{0}
if vc { *res = []byte{1} }

Avoids the break statements. Here and below.

Done.

---

types/conversion_test.go, line 526 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This should be Zero time.

Done.

---

types/conversion_test.go, line 548 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Should be Zero time.

Done.

[manishrjain]

This PR makes tests a lot harder to read. I'm not sure why was there a need to change them?

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain and @srfrog)

---

types/conversion_test.go, line 526 at r2 (raw file):

	}{
		// time.Date(2009, time.November, 10, 23, 0, 0, 0, time.UTC)
		{in: Val{Tid: DateTimeID, Value: []byte{1, 0, 0, 0, 14, 194, 139, 231, 112, 0, 0, 0, 0, 255, 255}},

The tests before were a lot more readable. Byte arrays are impossible to understand. Also, moving time.Date to comment isn't great.

[srfrog]

I'll add some spaces. Most of these tests are new or rewritten, the file was mostly outdated and commented out.

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain and @srfrog)

[srfrog]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain)

---

types/conversion_test.go, line 526 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

The tests before were a lot more readable. Byte arrays are impossible to understand. Also, moving time.Date to comment isn't great.

Done.

[manishrjain]

One comment. See if you'd like to address it before submitting.

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @srfrog)

---

types/conversion.go, line 75 at r3 (raw file):

			case DateTimeID:
				var t time.Time
				if !bytes.Equal(data, []byte("")) {

if len(data) > 0?

---

types/conversion.go, line 285 at r3 (raw file):

				} else {
					secs := t.Unix()
					if secs > math.MaxInt64 || secs < math.MinInt64 {

Not your code, but would this condition ever be true? Think never.

---

types/conversion_test.go, line 398 at r3 (raw file):

		{
			in:  float64(3.0),
			out: true,

I think you should be able to line in a single line, instead of splitting these over 4 lines each.

Would improve the readability considerably. Here and elsewhere.

[srfrog]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain)

---

types/conversion.go, line 75 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if len(data) > 0?

No, data is a slice so nil would be the same as empty slice.

Ex: https://play.golang.camp/p/gQKQuP4fjIw

---

types/conversion.go, line 285 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Not your code, but would this condition ever be true? Think never.

Yes, the first test will fail on "2147483647-12-31 13:00:01.147483647 +0000 UTC" but then we would be dust and it wont matter. The other case will fail if we go back in time before epoch and adjust our clock :)

---

types/conversion_test.go, line 398 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I think you should be able to line in a single line, instead of splitting these over 4 lines each.

Would improve the readability considerably. Here and elsewhere.

Done.

[srfrog]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain)

---

types/conversion.go, line 285 at r3 (raw file):

Previously, srfrog (Gus) wrote…

Yes, the first test will fail on "2147483647-12-31 13:00:01.147483647 +0000 UTC" but then we would be dust and it wont matter. The other case will fail if we go back in time before epoch and adjust our clock :)

Done.