This is a short tutorial about how to make and test changes in Dolt and its Go MySQL engine. We're going to work through fixing and testing a recent bug that was filed against Dolt. In the bug report, a user is using RMariaDB, a database connector library for R, and is seeing some values of certain timestamps fail to insert. The server is returning an error:
time=data.frame(time=as.POSIXct("2014-01-01 19:00:00"))try(dbWriteTable(dolt_conn,"time",time,overwrite=TRUE))#> Error : value "2014-1-2" can't be converted to time.Time [1105]
Let's see if we can make progress on reproducing the bug. Here, we just attempt to insert the same string that appears in the error message into a timestamp column:
~/dolt_workspace $ mkdir test_db~/dolt_workspace $ cd test_db~/dolt_workspace/test_db $ dolt initSuccessfullyinitializeddoltdatarepository.~/dolt_workspace/test_db $ dolt sql# Welcome to the DoltSQL shell.# Statements must be terminated with ';'.# "exit" or "quit" (or Ctrl-D) to exit.test_db> createtablehas_timestamp (t timestamp);test_db> insertintohas_timestampvalues ('2014-1-2');value"2014-1-2"can't be converted to time.Time
Brilliant, we've managed to reproduce the bug. Let's track down where that error is coming from.
Tracking Down the Error
Hopefully this isn't too hard:
so we can open go-mysql-server/sql/datetimetype.go in an editor and look for uses of ErrConvertingToTime. We quickly find the only place it is returned:
and going to the definition of TimestampDatetimeLayouts, we find:
Notably present is a "2006-1-2 15:4:5.999999", which looks like it might handle single-digit months and days in a date that comes with a timestamp. But there is no corresponding "2006-1-2" in that list, which would possibly handle the case we're trying to fix. Let's start by looking for a unit test we could write that would demonstrate the bug in this ConvertWithoutRangeCheck function. We open up datetimetype_test.go, and look through the top level functions:
for our purposes, TestDatetimeConvert looks the most promising, and digging into the implementation, it's a table driven test with a pretty straight forward structure of giving an input to Convert and asserting a matching output and error value. We can just add tests for each of the three handled time types:
and then try running the tests:
Perfect. Let's go ahead and see if we can fix it by adding the new pattern in TimestampDatetimeLayouts.
and test again:
Seems to work. So now we have a unit test demonstrating the problem and we've made that test pass. There's one other type of test we can add if we want to. The original bug was reported in the context of an insert, and it would be great to codify the fact that INSERT INTO table_with_datetime (timestamp) VALUES ("2006-6-3") should always work. For that kind of test, go-mysql-server has a directory of integration tests that lives at //enginetest. If we look around a little, we will find a file called insert_queries.go and we can look through that file for places where we're doing integration tests of inserts into datetime columns. Eventually, we might come with something like:
which hopefully captures the spirit of the behavior we want to verify. We can also verify that this fixes the behavior all the way at the dolt layer:
Committing and Opening a Pull Request
At this point, the bug is fixed in Dolt's SQL engine. Commit and push your changes through git.
$ ~/dolt_workspace $ grep -R "can't be converted to time.Time"
aaronson@Aarons-MacBook-Pro dolt_workspace % grep -R "can\'t be converted to time.Time" .
./go-mysql-server/benchmark/metadata.go: // TODO: value "1996-01-02" can't be converted to time.Time
./go-mysql-server/benchmark/metadata.go: // TODO: value "1996-03-13" can't be converted to time.Time
./go-mysql-server/benchmark/metadata.go: // TODO: value "1996-03-13" can't be converted to time.Time
./go-mysql-server/benchmark/metadata.go: // TODO: value "1996-03-13" can't be converted to time.Time
./go-mysql-server/sql/datetimetype.go: ErrConvertingToTime = errors.NewKind("value %q can't be converted to time.Time")
func (t datetimeType) ConvertWithoutRangeCheck(v interface{}) (time.Time, error) {
var res time.Time
switch value := v.(type) {
case string:
if value == zeroDateStr || value == zeroTimestampDatetimeStr {
return zeroTime, nil
}
parsed := false
for _, fmt := range TimestampDatetimeLayouts {
if t, err := time.Parse(fmt, value); err == nil {
res = t.UTC()
parsed = true
break
}
}
if !parsed {
return zeroTime, ErrConvertingToTime.New(v)
}
// TimestampDatetimeLayouts hold extra timestamps allowed for parsing. It does
// not have all the layouts supported by mysql. Missing are two digit year
// versions of common cases and dates that use non common separators.
//
// https://github.com/MariaDB/server/blob/mysql-5.5.36/sql-common/my_time.c#L124
TimestampDatetimeLayouts = []string{
"2006-01-02 15:04:05.999999",
"2006-01-02",
"2006-1-2 15:4:5.999999",
time.RFC3339,
time.RFC3339Nano,
"2006-01-02T15:04:05",
"20060102150405",
"20060102",
"2006/01/02",
"2006-01-02 15:04:05.999999999 -0700 MST", // represents standard Time.time.UTC()
}
dolt_workspace/go-mysql-server/sql $ go test .
ok github.com/dolthub/go-mysql-server/sql 0.327s
diff --git a/enginetest/insert_queries.go b/enginetest/insert_queries.go
index 5b88fad5..a4daf1f3 100644
--- a/enginetest/insert_queries.go
+++ b/enginetest/insert_queries.go
@@ -190,6 +190,12 @@ var InsertQueries = []WriteQueryTest{
SelectQuery: "SELECT * FROM typestable WHERE id = 999;",
ExpectedSelect: []sql.Row{{int64(999), nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil}},
},
+ {
+ WriteQuery: `INSERT INTO typestable (id, ti, da) VALUES (999, '2021-09-1', '2021-9-01');`,
+ ExpectedWriteResult: []sql.Row{{sql.NewOkResult(1)}},
+ SelectQuery: "SELECT id, ti, da FROM typestable WHERE id = 999;",
+ ExpectedSelect: []sql.Row{{int64(999), sql.MustConvert(sql.Timestamp.Convert("2021-09-01")), sql.MustConvert(sql.Date.Convert("2021-09-01"))}},
+ },
{
WriteQuery: `INSERT INTO typestable SET id=999, i8=null, i16=null, i32=null, i64=null, u8=null, u16=null, u32=null, u64=null,
f32=null, f64=null, ti=null, da=null, te=null, bo=null, js=null, bl=null;`,
~/dolt_workspace/dolt/go $ go install ./cmd/dolt
~/dolt_workspace/dolt/go $ cd ../../test_db
~/dolt_workspace/test_db $ dolt sql
# Welcome to the DoltSQL shell.
# Statements must be terminated with ';'.
# "exit" or "quit" (or Ctrl-D) to exit.
test_db> insert into has_timestamp values ('2014-1-2');
Query OK, 1 row affected
~/dolt_workspace/go-mysql-server $ git commit -am 'Fix string to datetime convert of YYYY-M-D.'
[aaron/fix-2088-time-convert-bug 81dce573] Fix string to datetime convert of YYYY-M-D.
3 files changed, 16 insertions(+)
~/dolt_workspace/go-mysql-server $ git push origin aaron/fix-2088-time-convert-bug:aaron/fix-2088-time-convert-bug
Enumerating objects: 13, done.
Counting objects: 100% (13/13), done.
Delta compression using up to 8 threads
Compressing objects: 100% (3/3), done.
Writing objects: 100% (7/7), 803 bytes | 803.00 KiB/s, done.
Total 7 (delta 6), reused 4 (delta 4)
remote: Resolving deltas: 100% (6/6), completed with 6 local objects.
remote:
remote: Create a pull request for 'aaron/fix-2088-time-convert-bug' on GitHub by visiting:
remote: https://github.com/reltuk/go-mysql-server/pull/new/aaron/fix-2088-time-convert-bug
remote:
To github.com:reltuk/go-mysql-server.git
* [new branch] aaron/fix-2088-time-convert-bug -> aaron/fix-2088-time-convert-bug
