Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support parse time config #119

Merged
merged 4 commits into from Mar 26, 2017
Merged

support parse time config #119

merged 4 commits into from Mar 26, 2017

Conversation

siddontang
Copy link
Collaborator

@shlomi-noach

I support a ParseTime config to decode timestamp and datetime to a Time structure.

@shlomi-noach
Copy link
Contributor

👍

@siddontang
Copy link
Collaborator Author

@shlomi-noach

I hope this can help you in your project, but we should notice that it is a customized Time which needs to output zero time correctly for MySQL.

@siddontang
Copy link
Collaborator Author

Another way is to return origin golang Time, but for zero time, we return string "0000-00-00 00:00:00" directly.

which way do you think better?

@shlomi-noach
Copy link
Contributor

Another way is to return origin golang Time, but for zero time, we return string "0000-00-00 00:00:00" directly.

That is the approach I took in https://github.com/siddontang/go-mysql/pull/86/files#diff-d9f966f86227646d11c3024f8cb537a8R651

If the date is 0, return a string. If the date is valid, return a Time.

@siddontang
Copy link
Collaborator Author

There does not handle fraction correctly.
If sec is 0 but frac is not, we should return like "0000-00-00 00:00:00.123456", of course, this is not a big problem.

So your prefer return string for zero time? @shlomi-noach

@shlomi-noach
Copy link
Contributor

If sec is 0 but frac is not, we should return like "0000-00-00 00:00:00.123456", of course, this is not a big problem.

You are right. Is it possible to return 0000-00-00 00:00:00.123456 as a Time value? If so, it should be a Time.

For complete zero, I prefer a string, because it isn't actually a real Time. At least this is true for TIMESTAMP values. As for DATETIME, let me think further.

@shlomi-noach
Copy link
Contributor

shlomi-noach commented Mar 20, 2017

Hold on. 0000-00-00 00:00:00 is an invalid value and can never be a real date. (zero month? zero day? Impossible)

Which is why 0000-00-00 00:00:00.123456 is invalid as well.

So yes, 0000-00-00 00:00:00 should be a returned as a string.

The zero-Time is a valid time: https://golang.org/pkg/time/#Time.IsZero, denoting "January 1, year 1, 00:00:00 UTC.". This does not equal 0000-00-00 00:00:00.

@siddontang
Copy link
Collaborator Author

@shlomi-noach

If you create a table with a field datetime(6), you can insert "0000-00-00 00:00:00.123456", and the select result is also "0000-00-00 00:00:00.123456", not "0000-00-00 00:00:00".

Go Time can't handle both "0000-00-00 00:00:00" and "0000-00-00 00:00:00.123456", so we should return string for it, and for other valid time, we can return Time directly.

@shlomi-noach
Copy link
Contributor

Go Time can't handle both "0000-00-00 00:00:00" and "0000-00-00 00:00:00.123456", so we should return string for it, and for other valid time, we can return Time directly.

This is also what I was saying :)

@siddontang
Copy link
Collaborator Author

@shlomi-noach

I have changed the return type, for zero return string, the other return golang time.

@shlomi-noach
Copy link
Contributor

@siddontang if you like to wait I will update my go-mysql dependency in gh-ost and confirm this works well. This would include a very short local-tests cycle but also a several-day production tests cycle.
I will anyhow report back my findings, even if you merge.

@siddontang
Copy link
Collaborator Author

Hi @shlomi-noach

I will wait your good news :-)

@siddontang
Copy link
Collaborator Author

Hi @shlomi-noach

Any update? Can it work well in your production?

@shlomi-noach
Copy link
Contributor

@siddontang I haven't deployed it yet. And it will be at least a week in testing; what i'd suggest is you can merge and I'll open an issue if there's any problems.

@siddontang siddontang merged commit 3ca161f into master Mar 26, 2017
@siddontang siddontang deleted the siddontang/parse-time branch March 26, 2017 14:15
@siddontang
Copy link
Collaborator Author

ok, I have merged and you can use the master now. @shlomi-noach

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.

None yet

2 participants