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

Date(::String, ::DateFormat) isn't type stable #13644

Closed
quinnj opened this issue Oct 16, 2015 · 5 comments
Closed

Date(::String, ::DateFormat) isn't type stable #13644

quinnj opened this issue Oct 16, 2015 · 5 comments
Labels
dates Dates, times, and the Dates stdlib module

Comments

@quinnj
Copy link
Member

quinnj commented Oct 16, 2015

Placeholder issue so I remember:

julia> @code_warntype Date("2015-01-01",Dates.DateFormat("yyyy-mm-dd"))
Variables:
  dt::ASCIIString
  df::Base.Dates.DateFormat

Body:
  begin  # dates/io.jl, line 205:
      return (top(_apply))((top(getfield))(Base.Dates,:call)::F,Base.Dates.Date,(Base.Dates.parse)(dt::ASCIIString,df::Base.Dates.DateFormat)::Array{Any,N})::Any
  end::Any
@quinnj quinnj added the dates Dates, times, and the Dates stdlib module label Oct 16, 2015
@amellnik
Copy link
Contributor

Oh man, I think that the DateTime version of this is the cause of a strange issue I've been seeing related to different number of unique rows in the same DataFrame before and after saving to a csv and reloading. Good catch.

@quinnj
Copy link
Member Author

quinnj commented Oct 22, 2015

Umm.....I don't think that should happen. The key issue here is just that type inference isn't able to know that a Date will in fact be returned from Date(::String, ::DateFormat). It shouldn't actually return different values at all. Or maybe I'm misunderstanding; do you have a reproducible case you could share?

@felipenoris
Copy link
Contributor

After 94b2b20, things got a little better:

julia> @code_warntype Date("2015-01-01",Dates.DateFormat("yyyy-mm-dd"))
Variables:
  dt::ASCIIString
  df::Base.Dates.DateFormat

Body:
  begin  # dates/io.jl, line 205:
      return (top(_apply))((top(getfield))(Base.Dates,:call)::F,Base.Dates.Date,(Base.Dates.parse)(dt::ASCIIString,df::Base.Dates.DateFormat)::Array{Any,1})::Any
  end::Any

@amellnik
Copy link
Contributor

@quinnj I wasn't able to reproduce this in a simple case, and I recently moved over to using SQLite as a backend, so I don't see it in my actual data anymore. You're probably right that this was due to something else. My other theory was that it was related to columns with element type Any containing multiple string types, but it looks like that's not the case either. Dunno.

@KristofferC
Copy link
Member

Fixed:

julia> @code_warntype Date("2015-01-01",Dates.DateFormat("yyyy-mm-dd"))
Variables:
  #self#::Any
  dt::String
  df::DateFormat{Symbol("yyyy-mm-dd"),Tuple{Base.Dates.DatePart{'y'},Base.Dates.Delim{Char,1},Base.Dates.DatePart{'m'},Base.Dates.Delim{Char,1},Base.Dates.DatePart{'d'}}}

Body:
  begin 
      return $(Expr(:invoke, MethodInstance for parse(::Type{Date}, ::String, ::DateFormat{Symbol("yyyy-mm-dd"),Tuple{Base.Dates.DatePart{'y'},Base.Dates.Delim{Char,1},Base.Dates.DatePart{'m'},Base.Dates.Delim{Char,1},Base.Dates.DatePart{'d'}}}), :(Base.Dates.parse), :(Base.Dates.Date), :(dt), :(df)))
  end::Date

I believe the recent work on the Date parsing has sufficient coverage for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dates Dates, times, and the Dates stdlib module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants