Skip to content

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.

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