Date field type not supported?

[baldwindavid]

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 March by_month queries.

describe Appointment, '#by_month' do

  it 'returns appointments within the month' do
    Appointment.create date: "Feb 1, 2014"
    Appointment.create date: "Feb 28, 2014"
    Appointment.create date: "Mar 1, 2014"

    february_query = Appointment.by_month(2, year: 2014, field: :date)
    expect(february_query.collect {|a| a.date.to_s}).to eq [
      "2014-02-01",
      "2014-02-28"
    ] # fails, because it includes "2014-03-01"

    march_query = Appointment.by_month(3, year: 2014, field: :date)
    expect(march_query.collect {|a| a.date.to_s}).to eq [
      "2014-03-01"
    ]
  end

end

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.

[baldwindavid]

This functionality could probably be added by calling to_date on the comparison times prior to building the SQL statement. For example, in ByStar::ActiveRecord::ClassMethods.between_times_query there is a start and finish. If we know that the column type of start_field and end_field is a Date, then start and finish could be set to start.to_date and finish.to_date. Either an explicit option like :field_type could be added or perhaps it could dynamically get the column type with something like self.columns_hash[start_field].type. I can look at this if there is any appetite for this to be added to the codebase.

[johnnyshields]

This could be simply an issue with inclusive versus exclusive... in theory this should work for dates.

[baldwindavid]

Hm. Perhaps I am doing something wrong, but I added the specs below to the by_star test suite in spec/integration/shared/by_month.rb with the time locked down and in specific time zones. Also, a date column was added to the posts table...

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 to_date on the start and finish as so...

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.

[johnnyshields]

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.

1. If column is a Date, we need to convert times from local to the their UTC date.
2. If column is a Time, we need to Dates to beginning_of_day in local time, (or end_of_day in some contexts.)

Is this a correct understanding? If so, I believe case #2 is handled throughout the library currently, but case #1 is not.

[baldwindavid]

Yes, I think that is generally it. I'm not sure about the "UTC" part of this statement though...

1) If column is a Date, we need to convert times from local to the their UTC date.

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 US Eastern Standard Time...

t = Time.zone.parse "Dec 31, 2014 11pm" #=> Wed, 31 Dec 2014 23:00:00 EST -05:00

If you now call utc on that and then to_date, you'll actually get January 1st, 2015...

t.utc.to_date #=> Thu, 01 Jan 2015

So it seems like maybe just to_date would be okay?

t = Time.zone.parse("Dec 31, 2014 11pm").to_date #=> Wed, 31 Dec 2014

[johnnyshields]

Yes you're correct I meant to say local instead of UTC.

[gamov]

Hi,
I've got bitten with this too. I have a shipment with an ETD (Date type) of 2015.8.31. It shows up when I call:

Shipment.by_month(8, year: 2015, field: :etd) 

but doesn't when I do

Shipment.between_times(Date.new(2015), Date.new(2015,8,31), field: :etd)

IMO, it should be consistent. Not converting dates to times would resolve the issue, I think.

[johnnyshields]

I'd gladly accept that PR that looks at the type of the field and implements the appropriate date / time conversion logic.

[johnnyshields]

Not converting dates to times would resolve the issue, I think.

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.

[johnnyshields]

This is now properly supported on master, and there's also a new between_dates method. Note that it uses the current system time zone.