Skip to content

Conversation

@FluffyEscargot
Copy link
Contributor

@FluffyEscargot FluffyEscargot commented Feb 15, 2024

  • enable time parsing in mysql connection option
  • add parsing of time values for printing dump file
  • adjust integration test to use: TIMESTAMP, DATETIME, DATE and TIME fields

Integration test currently fails, as DATE and TIME fields are printed as full datetime strings.

INSERT INTO `t1` (`id`, `name`, `test_timestamp`, `test_datetime`, `test_date`, `test_time`)
VALUES (1,'John','2012-11-01 00:15:00','2012-11-01 00:15:00','2012-11-01','00:15:00');
INSERT INTO `t1` (`id`, `name`, `test_timestamp`, `test_datetime`, `test_date`, `test_time`)
VALUES (1,'John','2012-11-01 00:15:00','2012-11-01 00:15:00','2012-11-01 00:00:00',_binary '00:15:00');

To fix this, the RowBuffer method would have to be aware of the SQL field type, to distinguish between e.g DATE and DATETIME for the formatting. This would be a bigger change @deitch maybe you have an idea on how to handle that, or just adjust the test to ignore it.
The currently produced output still works when restoring the backup.

Close #272

- enable time parsing in mysql connection option
- add parsing of time values for printing dump file
- adjust integration test to use: TIMESTAMP, DATETIME, DATE and TIME fields

Integration test currently fails, as DATE and TIME fields are printed as full datetime strings.
@deitch
Copy link
Collaborator

deitch commented Feb 15, 2024

First of all, this makes sense, thank you.

The issue you raise with integration test is because the dumped data ends up as a single time.Time (ok, NullTime, but same idea), without distinguishing between TIMESTAMP, DATETIME, DATE and TIME? And so they all look the same?

When you dump these in normal mysqldump, what does the output look like? How does it know how to distinguish between? Is it inherent to the format of the data itself - which we could leverage here - or is it that a restore looks at the type of the field to which it restores and thus determines how to handle the content which will restore to a field?

@deitch
Copy link
Collaborator

deitch commented Feb 15, 2024

Also, what is config.ParseTime for? Wouldn't it always?

@FluffyEscargot
Copy link
Contributor Author

So config.ParseTime is needed, as it will fail otherwise on DATETIME, DATE and TIMESTAMP, as mentioned in #272 . I don't fully understand, why it doesn't just default to another datatype, like rawbytes or something. (It does for TIME fields).
The two INSERT INTO strings above are from the output of the failing integration test. So the first one is normal mysqldump which knows the difference between DATETIME, DATE and TIME. The second one is from the mysql-backup tool, specifically with my change to RowBuffer so it prints the datetimes more nicely, otherwise it looks like this: '&{2023-05-09 00:00:00 +0000 UTC %!s(bool=true)}' which is just the go time.Time object. You already have a list of the column names in table.cols which can be accessed, but we would also need something like table.col_types which represents the actual SQL types. This would allow for a switch statement to print DATETIME and TIMESTAMP with date and time, DATE just the date and TIME just the time. I haven't done that yet because it would require a bunch more changes, but I think its not possible to solve it otherwise.

@deitch
Copy link
Collaborator

deitch commented Feb 15, 2024

I don't fully understand, why it doesn't just default to another datatype, like rawbytes or something. (It does for TIME fields

I don't know, I don't care. If what you did solves it without breaking anything else, I just smile, say "thank you" and take it.

The two INSERT INTO strings above are from the output of the failing integration test. So the first one is normal mysqldump which knows the difference between DATETIME, DATE and TIME. The second one is from the mysql-backup tool, specifically with my change to RowBuffer so it prints the datetimes more nicely, otherwise it looks like this: '&{2023-05-09 00:00:00 +0000 UTC %!s(bool=true)}' which is just the go time.Time object

Based on this, the RowBuffer() needs to know what the field type actually is, and then choose how to print it out? So something like:

		case *sql.NullTime:
                        switch {
                             case !s.Valid:
   				  b.WriteString(nullType)
                             case fieldType == typeDateTime:
				  fmt.Fprintf(&b, "'%s'", sanitize(s.Time.Format("2006-01-02 15:04:05")))
                             case fieldType == typeDate:
				  fmt.Fprintf(&b, "'%s'", sanitize(s.Time.Format("2006-01-02")))
                             case fieldType == typeTime:
				  fmt.Fprintf(&b, "'%s'", sanitize(s.Time.Format("15:04:05")))
                        }

?

@deitch deitch mentioned this pull request Mar 26, 2024
@deitch
Copy link
Collaborator

deitch commented Mar 26, 2024

I adopted your work into #283 and merged it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Go version: Crashes on DATE, DATETIME and TIMESTAMP fields

2 participants