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

Fix timestamp issue #27

Closed
wants to merge 2 commits into from

Conversation

nkottary
Copy link
Member

This is a fix for #26

MySQL timestamps were being interpreted as Int32's, but were received as date time strings. Changed datatype from Int32 to MySQLDateTime.

@ViralBShah
Copy link
Member

Can they not be Julia date time objects?

@nkottary
Copy link
Member Author

The Dates.DateTime has a lot more options and takes longer to parse string to datetime.

julia> @time DateTime("2015-03-12 12:30:30", "yyyy-mm-dd HH:MM:SS")
  0.000424 seconds (239 allocations: 13.219 KB)
2015-03-12T12:30:30

julia> @time MySQLDateTime("2015-03-12 12:30:30")
  0.000008 seconds (21 allocations: 848 bytes)
2015-3-12 12:30:30

I have therefore made a simpler MySQLDateTime which does the job of parsing datetime from a string and getting each field as an integer.

@@ -77,3 +79,11 @@ end
function ==(a::MySQLDateTime, b::MySQLDateTime)
a.date == b.date && a.time == b.time
end

@require Dates begin
using Dates
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dates is not that big a dependency, and it is built into base in 0.4. Is Requires.jl worth it here?

@aviks
Copy link
Member

aviks commented Nov 24, 2015

I really don't like the idea of returning a new type. As a user, what am I supposed to do with this new type? I'll have to convert into Base.DateTime first before doing anything useful. So even with the performance difference, I think this is a bad idea.

As for the performance, if you are converting a fixed format, you can use a predefined Dates.DateFormat. Try with that and see how much slower it is. Look here (scroll down a bit) : http://docs.julialang.org/en/release-0.4/manual/dates/#constructors

@nkottary
Copy link
Member Author

@aviks I added Requires.jl because I am supporting v0.3.

I can get rid of MySQLDate and MySQLDateTime and use Date and DateTime respectively like you said.

Is there a julia time type? Right now I have a custom type MySQLTime.

@ViralBShah
Copy link
Member

I think the performance issue should be isolated and upstreamed. @aviks What do you suggest we do for just time?

@aviks
Copy link
Member

aviks commented Nov 24, 2015

For Time type, get this merged: JuliaLang/julia#12274

In the meanwhile, just use DateTime types for types as a workaround. That is what I do at the moment.

@nkottary
Copy link
Member Author

I am discarding this PR since the issue is fixed in #28

@nkottary nkottary closed this Nov 30, 2015
@nkottary nkottary deleted the nk-fix-timestamp branch November 30, 2015 04:12
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.

3 participants