madeintandem / hstore_accessor

Adds typed hstore-backed field support to ActiveRecord models.
MIT License
242 stars 47 forks source link

datetime drops milliseconds from the value #62

Open snappa opened 9 years ago

snappa commented 9 years ago

First let me say that this is a great gem. I found it today and it's really nice and makes hstore use much cleaner.

I need to store DateTime values in an hstore column and defining as follows: hstore_accessor :properties, queued_at: :datetime, started_at: :datetime, finished_at: :datetime

and setting queued_at, started_at, and finished_at drops off the millisecond portion of the DateTime value. I had gotten this to work in my model with the following and was looking forward to removing this code:

properties["queued_at"] = qd_value.strftime("%Q").to_i

and reconstituting using:

Time.strptime(qd_value.to_s, '%Q').to_datetime

This preserved the millisecond value of the timestamp.

crismali commented 9 years ago

Is there a step missing in your description above? I played with this a bit locally in a Rails 4.1 console:

x = Time.now
#=> 2015-03-24 11:56:28 -0500
x.to_i
#=> 1427216188
x.strftime("%Q").to_i
#=> 0
Time.strptime(x.to_s, '%Q').to_datetime
#=> Wed, 31 Dec 1969 18:00:02 -0600
snappa commented 9 years ago

The goal is to be able to compute time deltas in milliseconds.

# Create and store the value in the property
datetime_value = DateTime.now
properties['queued_at'] = datetime_value.strftime("%Q").to_i)

#Retrieve the value and use the DateTime instance to compare against another to get a delta in ms
timeval_str = properties['queued_at']
datetime_value =  Time.strptime(timeval_str, '%Q').to_datetime

# Notice the difference in values when you do:
datetime_value.to_i == 1427218442
# and 
datetime_value.strftime("%Q").to_i == 1427218442783

Does this help? Notice my serializer converts time to microseconds. The serializer in hstore_accessor does just a to_i on the DateTime instance yielding different values. Just using .to_i gives you the number of seconds since the Unix epoch but my method of serialization / deserialization gives you to the millisecond

crismali commented 9 years ago

This issue is difficult to solve without another major version release. We could store datetimes as the number of milliseconds and hack in a solution to make it work with legacy systems that have already stored milliseconds. In theory we could use the number of digits to switch, but that feels a bit hacky (at least at the moment).

Could you try opening up a pull request with possible implementation that does not require a major version release?

jhirn commented 9 years ago

Alternatively, we could introduce another type, perhaps timestamp, which has greater precision and consider deprecating one.

Thoughts?

On Tue, Mar 24, 2015 at 1:18 PM, Michael notifications@github.com wrote:

This issue is difficult to solve without another major version release. We could store datetimes as the number of milliseconds and hack in a solution to make it work with legacy systems that have already stored milliseconds. In theory we could use the number of digits to switch, but that feels a bit hacky (at least at the moment).

Could you try opening up a pull request with possible implementation that does not require a major version release?

— Reply to this email directly or view it on GitHub https://github.com/devmynd/hstore_accessor/issues/62#issuecomment-85629214 .

crismali commented 9 years ago

That could work well. That way when we hit the next major release we could swap drop datetime and just rename timestamp to datetime.

snappa commented 9 years ago

As a consumer of this gem, I'm good with whatever direction you choose. Personally, since it's a type of "datetime" and is associated with ActiveRecord I was expecting it to behave like saving a DateTime instance to a datetime column in Postgres would. Therefore I'd opt to make it save in the finer granularity and keep a single type. Then again, I don't know if this would break other users of your gem. I'm guessing it wouldn't but don't want to presume.

snappa commented 9 years ago

Regardless of the direction you choose, I'm grateful as I was literally about to create a gem like this for a project I'm working on so thank you for your work!

crismali commented 9 years ago

Thanks for using the gem and opening up this issue. We'll play around with this a bit more but I'm leaning towards the new data type option. Seems like the easiest way to go.

jhirn commented 9 years ago

I agree with @snappa on mimicking the natural ActiveRecord behavior in lieu of a new datatype.

Perhaps we can add the new functionality disabled by default which can be enabled via an initializer. This seems like the win-win for all parties.

On Tue, Mar 24, 2015 at 1:39 PM, Michael notifications@github.com wrote:

Thanks for using the gem and opening up this issue. We'll play around with this a bit more but I'm leaning towards the new data type option. Seems like the easiest way to go.

— Reply to this email directly or view it on GitHub https://github.com/devmynd/hstore_accessor/issues/62#issuecomment-85636604 .

snappa commented 9 years ago

Thanks guys. Ideally I'd make the datetime type perform exactly like ActiveRecord's datetime column but if that's going to potentially break code that's out there then that's not good for the masses.

I agree with you looking at the number of digits to determine how the data is saved is a bit of a hack to an already clean gem.

I'd opt for the new type or an be find with an initializer option. I'd also suggest providing a migration for people to update their systems IF you choose to keep one type and make it finer granularity. I'll take a look at this later today when I have a few. Right now I've coded around this and basically made methods that will be removed once hstore_accessor has a fix.

snappa commented 9 years ago

I think the confusion comes from the fact that there's a :datetime type that in ActiveRecord maps to a DateTime instance and to datetime column in most databases. The exception is that db2 and postgresql map them to timestamp columns. Regardless, Rails treats them as DateTime instances.

You have :datetime mapped to :time which all databases with the exception of sqlite and oracle map to a time type column. Oracle maps it to date and sqlite to datetime.

The more I think of this the more I'd have separate types for :time, :datetime, :timestamp, and :date that map to corresponding ActicveRecord types if your goal is to make it easy to have active record like behavior for types stored in an hstore. The list is ":primary_key, :string, :text, :integer, :float, :decimal, :datetime, :time, :date, :binary, :boolean"

Just my $.02 worth.

jhirn commented 9 years ago

Appreciate the input. We'll talk it over and let you know what we come up with.

On Tue, Mar 24, 2015 at 2:46 PM, snappa notifications@github.com wrote:

I think the confusion comes from the fact that there's a :datetime type that in ActiveRecord maps to a DateTime instance and to datetime column in most databases. The exception is that db2 and postgresql map them to timestamp columns. Regardless, Rails treats them as DateTime instances.

You have :datetime mapped to :time which all databases with the exception of sqlite and oracle map to a time type column. Oracle maps it to date and sqlite to datetime.

The more I think of this the more I'd have separate types for :time, :datetime, :timestamp, and :date that map to corresponding ActicveRecord types if your goal is to make it easy to have active record like behavior for types stored in an hstore. The list is ":primary_key, :string, :text, :integer, :float, :decimal, :datetime, :time, :date, :binary, :boolean"

Just my $.02 worth.

— Reply to this email directly or view it on GitHub https://github.com/devmynd/hstore_accessor/issues/62#issuecomment-85665498 .

snappa commented 9 years ago

I'm good with another type. If you add :timestamp for example just make it clear in the documentation what is expected to be stored there. The same goes for the :datetime type as it currently returns a Time instance and not a DateTime as the name implies. This can be solved with documentation and a new type. Your documentation does show the use of Time for datetime but I glossed over it thinking that since this was ActiveRecord column representation I made the assumption it was the same types based on name :datetime