utils: parse using byteslice in parseDateTime

[Code-Hex]

Description

When the time information is passed in byteslice, it takes a long time to parse it. The order in which the current parsing is done is

1. cast dest[i] to byteslice
2. cast byteslice to string
3. parseDateTime

And since the format for returning the time information to the client has been decided, I think it's obviously faster to do the parsing ownself right away.

The benchmark results

$ go test -benchmem . -bench "(DateTime|Cast)$"
goos: darwin
goarch: amd64
pkg: github.com/go-sql-driver/mysql
BenchmarkParseDateTime-4                         5039470               250 ns/op               0 B/op          0 allocs/op
BenchmarkParseByteDateTime-4                    11188809                99.2 ns/op             0 B/op          0 allocs/op
BenchmarkParseByteDateTimeStringCast-4           3342747               356 ns/op              32 B/op          1 allocs/op

Code

func deprecatedParseDateTime(str string, loc *time.Location) (t time.Time, err error) {
	const base = "0000-00-00 00:00:00.000000"
	switch len(str) {
	case 10, 19, 21, 22, 23, 24, 25, 26: // up to "YYYY-MM-DD HH:MM:SS.MMMMMM"
		if str == base[:len(str)] {
			return
		}
		if loc == time.UTC {
			return time.Parse(timeFormat[:len(str)], str)
		}
		return time.ParseInLocation(timeFormat[:len(str)], str, loc)
	default:
		err = fmt.Errorf("invalid time string: %s", str)
		return
	}
}

func BenchmarkParseDateTime(b *testing.B) {
	str := "2020-05-13 21:30:45"
	loc := time.FixedZone("test", 8*60*60)
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_, _ = deprecatedParseDateTime(str, loc)
	}
}

func BenchmarkParseByteDateTime(b *testing.B) {
	bStr := []byte("2020-05-25 23:22:01.159491")
	loc := time.FixedZone("test", 8*60*60)
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_, _ = parseDateTime(bStr, loc)
	}
}

func BenchmarkParseByteDateTimeStringCast(b *testing.B) {
	bStr := []byte("2020-05-25 23:22:01.159491")
	loc := time.FixedZone("test", 8*60*60)
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_, _ = deprecatedParseDateTime(string(bStr), loc)
	}
}

MySQL datetime format
https://dev.mysql.com/doc/refman/8.0/en/datetime.html

Checklist

• Code compiles correctly
• Created tests which fail without the change (if possible)
• All tests passing
• Extended the README / documentation, if necessary
• Added myself / the copyright holder to the AUTHORS file

[methane]

Can we just change the argument type of parseDateTime, instead of adding parseByteDateTime?

[Code-Hex]

@methane Honestly, it bothers me.
I added a new one because there is already a case for handling strings. (I think there's a cost to casting to string to byte slice)

mysql/nulltime.go

Lines 34 to 38 in 8c3a2d9

	case string:
	nt.Time, err = parseDateTime(v, time.UTC)
	nt.Valid = (err == nil)
	return
	}

[methane]

@Code-Hex It is deprecated function (See #960).
I think type conversion (allocation + copy) in the function is reasonable than two same implementation of parseDateTime.

[Code-Hex]

@methane Thanks for your reply.
Sounds good to me. I fix it.

[methane]

Please post the result of this benchmark and remove deprecatedParseDateTime.
We do not keep old implementations only for benchmark.

[Code-Hex]

removed 2c6aa27

[chanxuehong]

nice job!!!

I just thought that the parseDateTime can be optimized in the past few days, so I mentioned a PR #1117, and then I saw this implementation, it's interesting.

Here I have a few questions:

1. Is it better to use bytes.Equal here？？？
   

   mysql/utils.go

   Line 113 in 2c6aa27

   if string(b) == base[:len(b)] {

2. Is it really necessary to check that year(month, day) is equal to 0(in fact they will not be less than 0 here), we have not checked here if month is greater than 12, nor day is greater than 31

mysql/utils.go

Line 121 in 2c6aa27

if year <= 0 {

mysql/utils.go

Line 133 in 2c6aa27

if m <= 0 {

mysql/utils.go

Line 146 in 2c6aa27

if day <= 0 {

[shogo82148]

I have same question with chanxuehong about the range of year(month, day).
Why month 0 is treated as January?
In the current implementation, parseDateTime returns "month out of range" error with month 0.

https://play.golang.org/p/HgQkcLCOGMU

package main

import (
	"fmt"
	"time"
)

func main() {
	fmt.Println(time.Parse("2006-01-02 15:04:05.999999", "0000-00-00 00:00:00.000000"))
	fmt.Println(time.Parse("2006-01-02 15:04:05.999999", "9999-99-99 99:99:99.999999"))
}

0001-01-01 00:00:00 +0000 UTC parsing time "0000-00-00 00:00:00.000000": month out of range
0001-01-01 00:00:00 +0000 UTC parsing time "9999-99-99 99:99:99.999999": month out of range

I think the range checks are not needed, because MySQL returns correct datetime and we don't need to take care of incorrect datetime.
In addition, it is a little complex to calculate the correct range of the day. It varies depending on month and year (there are leap years!).

https://play.golang.org/p/0Z4QX-QtOxn

[Code-Hex]

@chanxuehong @shogo82148 thanks for the mention to me.

1. In my understanding, the maintainer wanted to take readability (I had implemented at utils: parse using byteslice in parseDateTime #1113 (comment)).

When I actually measured it, it wasn't much of a difference, so I thought it was good.

Code & Results

const base = "0000-00-00 00:00:00.000000"

var bbase = []byte(base)

func BenchmarkByteStringEq(b *testing.B) {
	for i := 0; i < b.N; i++ {
		b := []byte("0000-00-00 00:00:00.000000")
		_ = string(b) == base[:len(b)]
	}
}

func BenchmarkByteEq(b *testing.B) {
	for i := 0; i < b.N; i++ {
		b := []byte("0000-00-00 00:00:00.000000")
		_ = bytes.Equal(b, bbase[:len(b)])
	}
}

Result (two times)

$ go test . -bench "BenchmarkByte"
goos: darwin
goarch: amd64
pkg: github.com/go-sql-driver/mysql
BenchmarkByteStringEq-4         171111117                6.86 ns/op
BenchmarkByteEq-4               177546639                6.86 ns/op
PASS
ok      github.com/go-sql-driver/mysql      3.837s

$ go test . -bench "BenchmarkByte"
goos: darwin
goarch: amd64
pkg: github.com/go-sql-driver/mysql
BenchmarkByteStringEq-4         174836755                6.76 ns/op
BenchmarkByteEq-4               168449960                6.94 ns/op
PASS
ok      github.com/go-sql-driver/mysql      3.823s

2. I've been thinking a lot.

Assumptions, I basically believe that we can trust the values that MySQL passes. So I don't think it's really necessary to do a range check here.

The reason for checking here is so that time.Time can be treated as zero value even if the parsed result is less than or equal to zero value. I thought that if we could treat it as a zero value, it would be possible to handle it on the user side at worst.

https://play.golang.org/p/4iQJgoOGYIQ

But in this case, time.Parse can parse and not return an error.

https://play.golang.org/p/wRD2ylQiF_9

---

Anyway, I think it's okay to leave out year, day, month range confirmation logic. I'll fix it if the maintainer is agree.

[shogo82148]

bytes.Equal(a, b []byte) is the same as string(a) == string(b). (I didn't know that!)

https://github.com/golang/go/blob/f1f8f9af9a55d73dfc6603a93bee0559fdc9024d/src/bytes/bytes.go#L18-L21

As the comment says, the Go compiler optimize this comparison.
So no need to use bytes.Equal for the performance.

time.Date(1, 1, 1, 0, 0, 0, 0, loc) is not the zero value if the loc is not time.UTC.

https://play.golang.org/p/T9-R0Bdpfzr

So it is not safe that users treat the zero value.

---

Anyway, my question doesn't block merging.

[shogo82148]

Sorry...
It's my miss operation.

[methane]

I think the range checks are not needed, because MySQL returns correct datetime and we don't need to take care of incorrect datetime.
In addition, it is a little complex to calculate the correct range of the day. It varies depending on month and year (there are leap years!).

You are right. These checks caused regression: #1252.