Microseconds #249
Microseconds #249
Conversation
passes all tests except TIME(1) -> string in binary protocol. TIMESTAMP support with microsecond resolution is still incomplete.
|
Travis fails because Go 1.1 does not support positional parameters in fmt.Sprintf yet and the MySQL version is to old to recognize DATETIME/DATE/TIME with fractional seconds syntactically. |
|
phew @julienschmidt PTAL All others: please have a look at it, too. |
| name string | ||
| length uint32 // length as string: DATETIME(4) => 24 |
There was a problem hiding this comment.
Do we need this field? I can only see it being set, not read somewhere.
There was a problem hiding this comment.
For a time when I deeply mistrusted the decimals field, I did use it.
I think we should still keep it for two reasons:
- it provides a hint useable for the maximal allocation size in all types
- with it, mysqlField is perfectly aligned on 64bit boundaries
There was a problem hiding this comment.
We don't use it, we don't need it 😄 #YAGNI
Regarding alignment: The compiler takes care of it if matters on the target architecture. On a 32bit architecture this is just a waste of space.
|
PTAL |
|
Sorry, I don't have forgotten you, but I currently don't have the time to review this properly (exams phase; double credit load). I invite anyone else to review the changes and write their opinion 😉 |
| dstlen = 8 | ||
| case 1, 2, 3, 4, 5, 6: | ||
| dstlen = 8 + 1 + decimals | ||
| } |
There was a problem hiding this comment.
A default case is missing to handle decimals you don't know about (e.g. from garbled protocol data).
If dstlen == 0 is the correct answer already, please make it explicit.
|
PTAL - and thanks for the review, @nightlyone |
|
LGTM, but please wait for @julienschmidt |
|
As by our policy, I will. Again, thanks for the review! |
|
Many thanks from me also! |
| @@ -11,7 +11,6 @@ package mysql | |||
| const ( | |||
| minProtocolVersion byte = 10 | |||
| maxPacketSize = 1<<24 - 1 | |||
| timeFormat = "2006-01-02 15:04:05" | |||
There was a problem hiding this comment.
Why not leave this here instead moving it to utils.go?
There was a problem hiding this comment.
Because I thought it's only used in utils.go, but that's wrong.
It's also used in packets.go.
I'll change that.
|
Did you test this with older MySQL versions? I'm still not convinced that this doesn't break the compatibility, especially the tests 😉 |
|
I did on 5.0.35 - and the Travis version is pretty old, too. rows, err = dbt.db.Query(`SELECT cast("00:00:00.1" as TIME(1)) = "00:00:00.1"`)
if err == nil {
rows.Scan(&withFrac)
rows.Close()
}is the part in the tests allowing for backward compatibility - it skips tests the server doesn't support. |
|
Lets trade: One LGTM for one successful MySQL 4.1 test 😄 |
|
4.1.12 on a Win XP VM successfully tested. |
|
fine 😒 |
|
I checked and confirmed that a timestamp can be stored in a table and retrieved from it, I also started on a test. Will probably commit and merge it tomorrow. Let's turn that frown upside down.. |
first attempt to add full support for microsecond resolution, comes with a major improvement of the tests for TIME, DATE and DATETIME.
At the very least, this can be built upon.