-
Notifications
You must be signed in to change notification settings - Fork 73
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 field type not supported? #49
Comments
This functionality could probably be added by calling |
This could be simply an issue with inclusive versus exclusive... in theory this should work for dates. |
Hm. Perhaps I am doing something wrong, but I added the specs below to the by_star test suite in create_table :posts, :force => true do |t|
t.timestamps
t.integer :day_of_month
t.date :start_date
end If the Active Record tests are run in the New York timezone, the posts on January 1st are not included and the post on February 1st is included. it 'returns the wrong records in New York' do
Time.use_zone "America/New_York" do
Timecop.freeze Time.zone.parse("Jan 1, 2014") do
expect(Post.by_month(field: 'start_date').collect {|e| e.start_date.to_s }).to eq [
"2014-01-05", "2014-01-10", "2014-01-12", "2014-01-20", "2014-02-01"]
end
end
end In looking at the generated SQL, this makes sense as a date of January 1, 2014 is going to be less than '2014-01-01 05:00:00.000000' and February 1 is going to be less than '2014-02-01 04:59:59.999999'. "SELECT \"posts\".* FROM \"posts\" WHERE (start_date >= '2014-01-01 05:00:00.000000' AND start_date <= '2014-02-01 04:59:59.999999')" Now my assumption would be that it would work okay in UTC, since there will not be a comparison with an offset. Unfortunately, it also does not catch the 2 records on January 1st... it 'returns the wrong records in UTC' do
Time.use_zone "UTC" do
Timecop.freeze Time.zone.parse("Jan 1, 2014") do
expect(Post.by_month(field: 'start_date').collect {|e| e.start_date.to_s }).to eq [
"2014-01-05", "2014-01-10", "2014-01-12", "2014-01-20"]
end
end
end The SQL suggests it might work... "SELECT \"posts\".* FROM \"posts\" WHERE (start_date >= '2014-01-01 00:00:00.000000' AND start_date <= '2014-01-31 23:59:59.999999')" And in Ruby if you compare a Date and Time at UTC, it suggests that the >= will work... Date.current >= Time.now.utc.at_beginning_of_day #=> true So, I'm guessing, that the same comparison between dates and times does not hold true with the database. If the line that generates the SQL is changed to call scope.where("#{start_field} >= ? AND #{end_field} <= ?", start.to_date, finish.to_date) ...the following SQL is generated... "SELECT \"posts\".* FROM \"posts\" WHERE (start_date >= '2014-01-01' AND start_date <= '2014-01-31')" And that returns all the appropriate records regardless of time zone. |
Got it, so the issue is UTC comparison--Dates are converted to times at "beginning_of_day" in the local time. Actually this one is a bit tricky, what we really need to do is look at the type of ActiveRecord column.
Is this a correct understanding? If so, I believe case #2 is handled throughout the library currently, but case #1 is not. |
Yes, I think that is generally it. I'm not sure about the "UTC" part of this statement though...
Wouldn't it just be...If column is a Date, we need to convert times from local to the their local date? With UTC, suppose you have something like the following in the t = Time.zone.parse "Dec 31, 2014 11pm" #=> Wed, 31 Dec 2014 23:00:00 EST -05:00 If you now call t.utc.to_date #=> Thu, 01 Jan 2015 So it seems like maybe just t = Time.zone.parse("Dec 31, 2014 11pm").to_date #=> Wed, 31 Dec 2014 |
Yes you're correct I meant to say local instead of UTC. |
Hi,
but doesn't when I do
IMO, it should be consistent. Not converting dates to times would resolve the issue, I think. |
I'd gladly accept that PR that looks at the type of the field and implements the appropriate date / time conversion logic. |
Not quite. We need to be aware of the field type being used on the backend. If it's a Date, then we convert to date, and if it's a Time, then we convert to Time. |
This is now properly supported on master, and there's also a new |
Am I correct in assuming that the
:field
option is not intended to support a Date column? In my test below, the March 1st appointment shows up in both the February and Marchby_month
queries.Never mind these being in a date column is not ideal in the first place, but just wanting to make sure that this is a known and intended behavior. Thanks.
The text was updated successfully, but these errors were encountered: