SQLite3 doesn't actually support the 'time' type.

[erichmenge]

Relates to #6247.

SQLite3 does not support the 'time' data type. I feel like we shouldn't pretend it does.

As it is now, if you create a new record with time only, the year gets set as the current year in the database. However, as in issue #6247 when you do Alert.all it shows the year as being 2000. Subsequently you get this behavior:

irb(main):028:0> Alert.create(start: Time.now)
   (0.1ms)  begin transaction
  SQL (0.8ms)  INSERT INTO "alerts" ("created_at", "start", "updated_at") VALUES (?, ?, ?)  [["created_at", Wed, 16 May 2012 03:54:09 UTC +00:00], ["start", 2012-05-15 22:54:09 -0500], ["updated_at", Wed, 16 May 2012 03:54:09 UTC +00:00]]
   (2.4ms)  commit transaction
=> #<Alert id: 7, start: "2012-05-15 22:54:09", created_at: "2012-05-16 03:54:09", updated_at: "2012-05-16 03:54:09">
irb(main):029:0> Alert.all
  Alert Load (0.3ms)  SELECT "alerts".* FROM "alerts" 
=> [#<Alert id: 7, start: "2000-01-01 03:54:09", created_at: "2012-05-16 03:54:09", updated_at: "2012-05-16 03:54:09">]
irb(main):030:0> Alert.last.start < Time.utc(2000, 1, 1, 3, 55, 0)
  Alert Load (0.2ms)  SELECT "alerts".* FROM "alerts" ORDER BY "alerts"."id" DESC LIMIT 1
=> true
irb(main):031:0> Alert.where('start < ?', Time.utc(2000, 1, 1, 3, 55, 0))
  Alert Load (0.3ms)  SELECT "alerts".* FROM "alerts" WHERE (start < '2000-01-01 03:55:00.000000')
=> []

It isn't very consistent to show start: "2000-01-01" and yet not find it. The reason is it really has year 2012 in the database.

I realize the year is just a dummy year since Ruby doesn't support any concept of time only. But the difference is MySQL and PostgreSQL don't actually store the year in the database, where as SQLite does, and it doesn't match what Rails displays.

As an aside, I had to add:

ActiveRecord::Base.time_zone_aware_attributes = false
ActiveRecord::Base.default_timezone = :local
Time.zone = nil

to a couple of the tests, because when I changed line 955 to skip SQLite3 adapter, those lines getting skipped at the end were causing those other two tests to fail. I don't see any tests that set those attributes and not set them back, but I guess something funny is going on there.

[tenderlove]

I'm not sure that this was a good idea. Now you can't use time datatypes in a production PG database, and use sqlite3 in test. The reason is because if you create a time column, it will just be dumped to schema.rb as a datetime.

I'm not 100% sure what we should do about the situation. If this example wasn't using string substitution, we could detect the column type as time and apply the appropriate sqlite3 function in order to ensure the query returns the right results.

[erichmenge]

Isn't it best to use the same database in testing and production?

[erichmenge]

I should say same database type. There are a number of differences between SQLite and Postgres. It seems like a bad idea to test in a different database type. Plus it seems like it would add extra complexity automatically using SQLite functions. But I don't have the experience you have, if you think we should do it a different way I'd be happy to work on another approach with you. It would be a good opportunity for me to get my hands dirty going a bit deeper into ActiveRecord.

[tenderlove]

Well, this is related to #6825 and pull request #6835. @mauricio do you have any ideas how we could handle this? We use SQLite3 in dev and Oracle in production (setting up Oracle is too hard). That said, we don't have any time only columns at work.

@erichmenge I'm not sure if I have a better approach. TBH, it seems like if we can't truly support time in sqlite3, we probably shouldn't allow people to use it in migrations. I think what I'm trying to say is "the type you use in your migration should be the same as the type generated in scheme.rb". If we can't deal with times in sqlite3, maybe we raise an exception?

OTOH, we could just say "this is a place where we can't make databases behave the same" and skip tests in sqlite3 on #6835.

@erichmenge thoughts?

[mauricio]

@tenderlove I'm +1 for not failing silently as it is today. Letting someone use a time field in SQLite and then generating a schema.rb with datetime without the user knowing is as dangerous as the original issue that generated the pull request. it might even cause issues if someone relies on the column information (as I am relying on my PR) in their code.

Imagine a form generator that checks the columns and their types, I did define this field to be a time, but when I query this information from the AR columns hash the field comes back as datetime and in my view I will generate a datetime form field which is going to make me wonder WTF is happening with my code, as my migration is correct (but my schema.rb will not be and it might take some time for me to figure this out).

If you are using SQLite you should probably be aware of this interesting behavior of your database and you should be able to work around it yourself (not being able to cleanly handle dates and times in general). We can even add this information cleanly at the AR documentation so everyone is aware that while you can define a time field when using SQLite, it will be stored as a full timestamp and you should work around this issue properly (or even be smart and just create two fields, one for the hour and another for the minutes).

I think throwing an exception might be too harsh, specially for people like @tenderlove that use SQLite in development and something else in production (which is pretty common, I have done this myself and my target was MS SQL Server), as they would have to work some magic to get time definitions to work. Having the information cleanly specified so that people understand the issues they will face seems to be good enough.

Other databases will handle his case correctly (MySQL and PostgreSQL do have time fields and will just work).

[drogus]

Sorry for not thinking about this more thoroughly before merging.

I would do 2 things:

• revert this pull request
• show a warning when you use time with Sqlite

I would not raise, as @mauricio says, because people might still want to use this if the know that it needs workaround.

[pixeltrix]

There's no problem with using the :time data type in Sqlite 3 databases - you can use whatever type name you want and it will faithfully record it in the table information.The problem comes from when the data is inserted in the database. The full date and time string is sent to the database and it's stored literally - even the word 'now' is stored literally, which can be exploited to always put something first when sorting by reverse date order.

When a time columns is read from the database it has it's date changed to 1/1/2000 by Rails, but it's not changed when when the value is written (we should do to fix #3145). Whether by design or luck this is the date that Sqlite assumes when given a time string of 'HH:MM:SS' so we should probably send that instead. The problem is how to handle querying - we can cast to a time string when using where(start: Time.current) or even where(start: Time.current..1.hour.from_now) but where('start > ?', Time.current) is tricky.

So we need to revert this commit and address how we are sending time strings to the database.

[pixeltrix]

Just to clarify schema.rb records the column as :time correctly so there's no issue with it not matching the migration.

[drogus]

I've reverted the commit, because it seems that it's the best thing we can do, regardless sqlite3 behavior is fixed or not.

[erichmenge]

When I originally made the PR it occurred to me that we could make the necessary SQLite function calls to support Time. As @tenderlove points out in #6835 it is already done for other column types in SQLite. But it seemed to me that it would be a complicated endeavor as Time related things can be.

@pixeltrix Would it be better to build this functionality into Rails or issue a warning and allow the developer to make the necessary adjustments to queries to use the SQLite date/time functions? If you want to bake this support into Rails I'd be happy to help, though I may need a bit of guidance.

[tenderlove]

IMO, we should just emit a warning during migrations or add some documentation (or both). There's no way we'll be able to "do the right thing" when someone issues a query like where('start > ?', Time.now). The business of SQL parsing is not a good one.

[drogus]

Or: whoever codes this feature is responsible for fixing all of the related bug reports

[erichmenge]

Okay, it sounds like the warning is the way to go. I'll get that taken care of and submit a new PR.

Thanks for the feedback everyone, sorry I missed the schema discrepancy consideration on the original PR.

[pixeltrix]

We can't issue a warning when someone does a where('start > ?', Time.current) because it might be against a datetime column - if we knew that it was a time column then we could format it correctly. I think documentation is probably the best solution - maybe issue a warning when migrating a time column on Sqlite?

I did toy with the idea of creating a new class to represent a time only but I guess this is something we wouldn't want to do - I can't even think of a good class name :-)

[mauricio]

I think the warning is mostly for the t.time :bonus_time migration than for querying (at least that's what I was thinking about when I proposed it). If you're querying you're on your own already.

@pixeltrix LocalTime maybe? Having a class for this would be nice, a very simple implementation would just have the same methods Time has but would issue deprecation warnings for them and would only answer correctly to hour, minutes, seconds (or even milliseconds). Date and Time handling is usually a pain in the arse on all platforms.

[erichmenge]

Since warnings are silenced for rake tasks, should this warning be emitted only for the logger? Or should the warning be issued to stderr anyway? I feel like the messages might be a bit noisy with no way to turn them off.

On the other hand if they are issued to the logger only, who will see them?