Jacob Lauemøller
2010-Jan-14 15:20 UTC
[PATCH] ActiveRecord timestamp conversions fail for some cases
Hi all,
The microsecond handling in
ActiveRecord::ConnectionAdapters::Column#fast_string_to_time and
ActiveRecord::ConnectionAdapters::Column#microseconds fail for some values.
In slightly more than 1% of all possible 6-digit cases, writing a timestamp to a
database column and then reading it back in results in a different value being
returned to the program.
So, for instance, saving the timestamp
2010-01-12 12:34:56.125014
and then loading it again from the database yields
2010-01-12 12:34:56.125013
The problem occurs when the value read is converted from string form to a Ruby
timestamp, so it is largely database independent (the exception being drivers
that override the methods, or databases that don''t support timestamps
at all).
The underlying problem is the use of to_i to convert from floats to ints inside
the affected methods. As you know, to_i simply truncates the result and in some
cases this causes rounding errors introduced by inherent inaccuracies in the
multiplication operations and decimal representation to bubble up and affect the
least significant digit.
Here''s a simple test that illustrates the problem:
converted =
ActiveRecord::ConnectionAdapters::Column.send(''fast_string_to_time'',
"2010-01-12 12:34:56.125014")
assert_equal 125014, converted.usec
This test case (and a similar one for #microseconds) fail on plain vanilla Rails
2.3.5.
I guess the best solution would be to change the ISO_DATETIME regex used to
extract the microsecond-part from timestamps to not include the decimal point at
all and then avoid the to_f and subsequent floating point multiplication
completely inside the failing methods. However, these regexes are defined as
constants on ActiveRecord::ConnectionAdapters::Column::Format and therefore
publicly available, so the impact of changing these is difficult to ascertain.
A simpler solution is to use round() instead of to_i to convert from the
intermediate floating point result to int. This works (I have verified that the
precision is sufficient for all possible 6-digit cases) but is about 15% slower
than the current method. A small price to pay for correctness, in my opinion.
I have attached a tiny patch (against 2.3.5) that switches the code to using
round() and a test case that verifies that the method works for a few
problematic cases that fail without the patch.
I have also created a Lighthouse ticket #3693:
https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/3693-patch-activerecord-timestamp-conversions-fail-for-some-cases
Could some of you please take a look and see if the patch is acceptable and
maybe carry it into the code base?
Cheers
Jacob
--
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.
Chris Cruft
2010-Jan-15 14:41 UTC
Re: ActiveRecord timestamp conversions fail for some cases
I gave up on that kind of resolution when I found that MySQL doesn''t support it! I''ll try to test the patch though. -Chris On Jan 14, 10:20 am, Jacob Lauemøller <jacob.lauemoel...@iteray.com> wrote:> Hi all, > > The microsecond handling in ActiveRecord::ConnectionAdapters::Column#fast_string_to_time and ActiveRecord::ConnectionAdapters::Column#microseconds fail for some values. > > In slightly more than 1% of all possible 6-digit cases, writing a timestamp to a database column and then reading it back in results in a different value being returned to the program. > > So, for instance, saving the timestamp > > 2010-01-12 12:34:56.125014 > > and then loading it again from the database yields > > 2010-01-12 12:34:56.125013 > > The problem occurs when the value read is converted from string form to a Ruby timestamp, so it is largely database independent (the exception being drivers that override the methods, or databases that don''t support timestamps at all). > > The underlying problem is the use of to_i to convert from floats to ints inside the affected methods. As you know, to_i simply truncates the result and in some cases this causes rounding errors introduced by inherent inaccuracies in the multiplication operations and decimal representation to bubble up and affect the least significant digit. > > Here''s a simple test that illustrates the problem: > > converted = ActiveRecord::ConnectionAdapters::Column.send(''fast_string_to_time'', "2010-01-12 12:34:56.125014") > assert_equal 125014, converted.usec > > This test case (and a similar one for #microseconds) fail on plain vanilla Rails 2.3.5. > > I guess the best solution would be to change the ISO_DATETIME regex used to extract the microsecond-part from timestamps to not include the decimal point at all and then avoid the to_f and subsequent floating point multiplication completely inside the failing methods. However, these regexes are defined as constants on ActiveRecord::ConnectionAdapters::Column::Format and therefore publicly available, so the impact of changing these is difficult to ascertain. > > A simpler solution is to use round() instead of to_i to convert from the intermediate floating point result to int. This works (I have verified that the precision is sufficient for all possible 6-digit cases) but is about 15% slower than the current method. A small price to pay for correctness, in my opinion. > > I have attached a tiny patch (against 2.3.5) that switches the code to using round() and a test case that verifies that the method works for a few problematic cases that fail without the patch. > > I have also created a Lighthouse ticket #3693: > > https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/3... > > Could some of you please take a look and see if the patch is acceptable and maybe carry it into the code base? > > Cheers > Jacob > > fix_microsecond_conversion.diff > 4KViewDownload > >-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Jacob Lauemøller
2010-Jan-15 14:45 UTC
Re: Re: ActiveRecord timestamp conversions fail for some cases
Thanks for the response -- we use PostgreSQL which does store all six microsecond digits. Jacob On 15/01/2010, at 15.41, Chris Cruft wrote:> I gave up on that kind of resolution when I found that MySQL doesn''t > support it! I''ll try to test the patch though. > > -Chris > > On Jan 14, 10:20 am, Jacob Lauemøller <jacob.lauemoel...@iteray.com> > wrote: >> Hi all, >> >> The microsecond handling in ActiveRecord::ConnectionAdapters::Column#fast_string_to_time and ActiveRecord::ConnectionAdapters::Column#microseconds fail for some values. >> >> In slightly more than 1% of all possible 6-digit cases, writing a timestamp to a database column and then reading it back in results in a different value being returned to the program. >> >> So, for instance, saving the timestamp >> >> 2010-01-12 12:34:56.125014 >> >> and then loading it again from the database yields >> >> 2010-01-12 12:34:56.125013 >> >> The problem occurs when the value read is converted from string form to a Ruby timestamp, so it is largely database independent (the exception being drivers that override the methods, or databases that don''t support timestamps at all). >> >> The underlying problem is the use of to_i to convert from floats to ints inside the affected methods. As you know, to_i simply truncates the result and in some cases this causes rounding errors introduced by inherent inaccuracies in the multiplication operations and decimal representation to bubble up and affect the least significant digit. >> >> Here''s a simple test that illustrates the problem: >> >> converted = ActiveRecord::ConnectionAdapters::Column.send(''fast_string_to_time'', "2010-01-12 12:34:56.125014") >> assert_equal 125014, converted.usec >> >> This test case (and a similar one for #microseconds) fail on plain vanilla Rails 2.3.5. >> >> I guess the best solution would be to change the ISO_DATETIME regex used to extract the microsecond-part from timestamps to not include the decimal point at all and then avoid the to_f and subsequent floating point multiplication completely inside the failing methods. However, these regexes are defined as constants on ActiveRecord::ConnectionAdapters::Column::Format and therefore publicly available, so the impact of changing these is difficult to ascertain. >> >> A simpler solution is to use round() instead of to_i to convert from the intermediate floating point result to int. This works (I have verified that the precision is sufficient for all possible 6-digit cases) but is about 15% slower than the current method. A small price to pay for correctness, in my opinion. >> >> I have attached a tiny patch (against 2.3.5) that switches the code to using round() and a test case that verifies that the method works for a few problematic cases that fail without the patch. >> >> I have also created a Lighthouse ticket #3693: >> >> https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/3... >> >> Could some of you please take a look and see if the patch is acceptable and maybe carry it into the code base? >> >> Cheers >> Jacob >> >> fix_microsecond_conversion.diff >> 4KViewDownload >> >> > -- > You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. > To post to this group, send email to rubyonrails-core@googlegroups.com. > To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. > For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en. > >-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Ken Collins
2010-Jan-15 14:56 UTC
Re: Re: ActiveRecord timestamp conversions fail for some cases
I plan on taking a look at this too. I think I had to solve it in the SQL Server adapter in my own way since it only stores milliseconds. If you care to look at our code, here are a few key sections in the tests. http://github.com/rails-sqlserver/2000-2005-adapter/blob/master/test/cases/adapter_test_sqlserver.rb#L271 http://github.com/rails-sqlserver/2000-2005-adapter/blob/master/lib/active_record/connection_adapters/sqlserver_adapter.rb#L328 On Jan 15, 2010, at 9:45 AM, Jacob Lauemøller wrote:> Thanks for the response -- we use PostgreSQL which does store all six microsecond digits. > > Jacob > > > On 15/01/2010, at 15.41, Chris Cruft wrote: > >> I gave up on that kind of resolution when I found that MySQL doesn''t >> support it! I''ll try to test the patch though. >> >> -Chris >> >> On Jan 14, 10:20 am, Jacob Lauemøller <jacob.lauemoel...@iteray.com> >> wrote: >>> Hi all, >>> >>> The microsecond handling in ActiveRecord::ConnectionAdapters::Column#fast_string_to_time and ActiveRecord::ConnectionAdapters::Column#microseconds fail for some values. >>> >>> In slightly more than 1% of all possible 6-digit cases, writing a timestamp to a database column and then reading it back in results in a different value being returned to the program. >>> >>> So, for instance, saving the timestamp >>> >>> 2010-01-12 12:34:56.125014 >>> >>> and then loading it again from the database yields >>> >>> 2010-01-12 12:34:56.125013 >>> >>> The problem occurs when the value read is converted from string form to a Ruby timestamp, so it is largely database independent (the exception being drivers that override the methods, or databases that don''t support timestamps at all). >>> >>> The underlying problem is the use of to_i to convert from floats to ints inside the affected methods. As you know, to_i simply truncates the result and in some cases this causes rounding errors introduced by inherent inaccuracies in the multiplication operations and decimal representation to bubble up and affect the least significant digit. >>> >>> Here''s a simple test that illustrates the problem: >>> >>> converted = ActiveRecord::ConnectionAdapters::Column.send(''fast_string_to_time'', "2010-01-12 12:34:56.125014") >>> assert_equal 125014, converted.usec >>> >>> This test case (and a similar one for #microseconds) fail on plain vanilla Rails 2.3.5. >>> >>> I guess the best solution would be to change the ISO_DATETIME regex used to extract the microsecond-part from timestamps to not include the decimal point at all and then avoid the to_f and subsequent floating point multiplication completely inside the failing methods. However, these regexes are defined as constants on ActiveRecord::ConnectionAdapters::Column::Format and therefore publicly available, so the impact of changing these is difficult to ascertain. >>> >>> A simpler solution is to use round() instead of to_i to convert from the intermediate floating point result to int. This works (I have verified that the precision is sufficient for all possible 6-digit cases) but is about 15% slower than the current method. A small price to pay for correctness, in my opinion. >>> >>> I have attached a tiny patch (against 2.3.5) that switches the code to using round() and a test case that verifies that the method works for a few problematic cases that fail without the patch. >>> >>> I have also created a Lighthouse ticket #3693: >>> >>> https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/3... >>> >>> Could some of you please take a look and see if the patch is acceptable and maybe carry it into the code base? >>> >>> Cheers >>> Jacob >>> >>> fix_microsecond_conversion.diff >>> 4KViewDownload >>> >>> >> -- >> You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. >> To post to this group, send email to rubyonrails-core@googlegroups.com. >> To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. >> For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en. >> >> > > -- > You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. > To post to this group, send email to rubyonrails-core@googlegroups.com. > To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. > For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en. > >-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Jacob Lauemøller
2010-Jan-15 15:24 UTC
Re: Re: ActiveRecord timestamp conversions fail for some cases
The problem only manifests itself if the sub-second part is stored with 4 or more significant digits. So if you only store and retrieve milliseconds (and thats the way I read your code) you should be home safe. On 15/01/2010, at 15.56, Ken Collins wrote:> > I plan on taking a look at this too. I think I had to solve it in the SQL Server adapter in my own way since it only stores milliseconds. If you care to look at our code, here are a few key sections in the tests. > > http://github.com/rails-sqlserver/2000-2005-adapter/blob/master/test/cases/adapter_test_sqlserver.rb#L271 > http://github.com/rails-sqlserver/2000-2005-adapter/blob/master/lib/active_record/connection_adapters/sqlserver_adapter.rb#L328 > > > > On Jan 15, 2010, at 9:45 AM, Jacob Lauemøller wrote: > >> Thanks for the response -- we use PostgreSQL which does store all six microsecond digits. >> >> Jacob >> >> >> On 15/01/2010, at 15.41, Chris Cruft wrote: >> >>> I gave up on that kind of resolution when I found that MySQL doesn''t >>> support it! I''ll try to test the patch though. >>> >>> -Chris >>> >>> On Jan 14, 10:20 am, Jacob Lauemøller <jacob.lauemoel...@iteray.com> >>> wrote: >>>> Hi all, >>>> >>>> The microsecond handling in ActiveRecord::ConnectionAdapters::Column#fast_string_to_time and ActiveRecord::ConnectionAdapters::Column#microseconds fail for some values. >>>> >>>> In slightly more than 1% of all possible 6-digit cases, writing a timestamp to a database column and then reading it back in results in a different value being returned to the program. >>>> >>>> So, for instance, saving the timestamp >>>> >>>> 2010-01-12 12:34:56.125014 >>>> >>>> and then loading it again from the database yields >>>> >>>> 2010-01-12 12:34:56.125013 >>>> >>>> The problem occurs when the value read is converted from string form to a Ruby timestamp, so it is largely database independent (the exception being drivers that override the methods, or databases that don''t support timestamps at all). >>>> >>>> The underlying problem is the use of to_i to convert from floats to ints inside the affected methods. As you know, to_i simply truncates the result and in some cases this causes rounding errors introduced by inherent inaccuracies in the multiplication operations and decimal representation to bubble up and affect the least significant digit. >>>> >>>> Here''s a simple test that illustrates the problem: >>>> >>>> converted = ActiveRecord::ConnectionAdapters::Column.send(''fast_string_to_time'', "2010-01-12 12:34:56.125014") >>>> assert_equal 125014, converted.usec >>>> >>>> This test case (and a similar one for #microseconds) fail on plain vanilla Rails 2.3.5. >>>> >>>> I guess the best solution would be to change the ISO_DATETIME regex used to extract the microsecond-part from timestamps to not include the decimal point at all and then avoid the to_f and subsequent floating point multiplication completely inside the failing methods. However, these regexes are defined as constants on ActiveRecord::ConnectionAdapters::Column::Format and therefore publicly available, so the impact of changing these is difficult to ascertain. >>>> >>>> A simpler solution is to use round() instead of to_i to convert from the intermediate floating point result to int. This works (I have verified that the precision is sufficient for all possible 6-digit cases) but is about 15% slower than the current method. A small price to pay for correctness, in my opinion. >>>> >>>> I have attached a tiny patch (against 2.3.5) that switches the code to using round() and a test case that verifies that the method works for a few problematic cases that fail without the patch. >>>> >>>> I have also created a Lighthouse ticket #3693: >>>> >>>> https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/3... >>>> >>>> Could some of you please take a look and see if the patch is acceptable and maybe carry it into the code base? >>>> >>>> Cheers >>>> Jacob >>>> >>>> fix_microsecond_conversion.diff >>>> 4KViewDownload >>>> >>>> >>> -- >>> You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. >>> To post to this group, send email to rubyonrails-core@googlegroups.com. >>> To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. >>> For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en. >>> >>> >> >> -- >> You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. >> To post to this group, send email to rubyonrails-core@googlegroups.com. >> To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. >> For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en. >> >> > > -- > You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. > To post to this group, send email to rubyonrails-core@googlegroups.com. > To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. > For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en. > >-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Ken Collins
2010-Jan-15 15:28 UTC
Re: Re: ActiveRecord timestamp conversions fail for some cases
Awesome, thanks for the reply. - Ken On Jan 15, 2010, at 10:24 AM, Jacob Lauemøller wrote:> The problem only manifests itself if the sub-second part is stored with 4 or more significant digits. So if you only store and retrieve milliseconds (and thats the way I read your code) you should be home safe. > > On 15/01/2010, at 15.56, Ken Collins wrote: > >> >> I plan on taking a look at this too. I think I had to solve it in the SQL Server adapter in my own way since it only stores milliseconds. If you care to look at our code, here are a few key sections in the tests. >> >> http://github.com/rails-sqlserver/2000-2005-adapter/blob/master/test/cases/adapter_test_sqlserver.rb#L271 >> http://github.com/rails-sqlserver/2000-2005-adapter/blob/master/lib/active_record/connection_adapters/sqlserver_adapter.rb#L328 >> >> >> >> On Jan 15, 2010, at 9:45 AM, Jacob Lauemøller wrote: >> >>> Thanks for the response -- we use PostgreSQL which does store all six microsecond digits. >>> >>> Jacob >>> >>> >>> On 15/01/2010, at 15.41, Chris Cruft wrote: >>> >>>> I gave up on that kind of resolution when I found that MySQL doesn''t >>>> support it! I''ll try to test the patch though. >>>> >>>> -Chris >>>> >>>> On Jan 14, 10:20 am, Jacob Lauemøller <jacob.lauemoel...@iteray.com> >>>> wrote: >>>>> Hi all, >>>>> >>>>> The microsecond handling in ActiveRecord::ConnectionAdapters::Column#fast_string_to_time and ActiveRecord::ConnectionAdapters::Column#microseconds fail for some values. >>>>> >>>>> In slightly more than 1% of all possible 6-digit cases, writing a timestamp to a database column and then reading it back in results in a different value being returned to the program. >>>>> >>>>> So, for instance, saving the timestamp >>>>> >>>>> 2010-01-12 12:34:56.125014 >>>>> >>>>> and then loading it again from the database yields >>>>> >>>>> 2010-01-12 12:34:56.125013 >>>>> >>>>> The problem occurs when the value read is converted from string form to a Ruby timestamp, so it is largely database independent (the exception being drivers that override the methods, or databases that don''t support timestamps at all). >>>>> >>>>> The underlying problem is the use of to_i to convert from floats to ints inside the affected methods. As you know, to_i simply truncates the result and in some cases this causes rounding errors introduced by inherent inaccuracies in the multiplication operations and decimal representation to bubble up and affect the least significant digit. >>>>> >>>>> Here''s a simple test that illustrates the problem: >>>>> >>>>> converted = ActiveRecord::ConnectionAdapters::Column.send(''fast_string_to_time'', "2010-01-12 12:34:56.125014") >>>>> assert_equal 125014, converted.usec >>>>> >>>>> This test case (and a similar one for #microseconds) fail on plain vanilla Rails 2.3.5. >>>>> >>>>> I guess the best solution would be to change the ISO_DATETIME regex used to extract the microsecond-part from timestamps to not include the decimal point at all and then avoid the to_f and subsequent floating point multiplication completely inside the failing methods. However, these regexes are defined as constants on ActiveRecord::ConnectionAdapters::Column::Format and therefore publicly available, so the impact of changing these is difficult to ascertain. >>>>> >>>>> A simpler solution is to use round() instead of to_i to convert from the intermediate floating point result to int. This works (I have verified that the precision is sufficient for all possible 6-digit cases) but is about 15% slower than the current method. A small price to pay for correctness, in my opinion. >>>>> >>>>> I have attached a tiny patch (against 2.3.5) that switches the code to using round() and a test case that verifies that the method works for a few problematic cases that fail without the patch. >>>>> >>>>> I have also created a Lighthouse ticket #3693: >>>>> >>>>> https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/3... >>>>> >>>>> Could some of you please take a look and see if the patch is acceptable and maybe carry it into the code base? >>>>> >>>>> Cheers >>>>> Jacob >>>>> >>>>> fix_microsecond_conversion.diff >>>>> 4KViewDownload >>>>> >>>>> >>>> -- >>>> You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. >>>> To post to this group, send email to rubyonrails-core@googlegroups.com. >>>> To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. >>>> For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en. >>>> >>>> >>> >>> -- >>> You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. >>> To post to this group, send email to rubyonrails-core@googlegroups.com. >>> To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. >>> For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en. >>> >>> >> >> -- >> You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. >> To post to this group, send email to rubyonrails-core@googlegroups.com. >> To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. >> For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en. >> >> > > -- > You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. > To post to this group, send email to rubyonrails-core@googlegroups.com. > To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. > For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en. > >-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Chris Cruft
2010-Jan-15 23:54 UTC
Re: ActiveRecord timestamp conversions fail for some cases
I spent an entirely inappropriate amount of time digging into this issue... Here are some observations: The #microseconds method LOOKS clumsy, but it''s actually quite optimized. Turns out the the sec_fraction component is a Rational less than one. So.... converting as quickly as possible to something other than Rational (Float, in this case) is actually the right thing to do. I tried dozens of alternatives and benchmarked them all and could not beat the current implementation -even after changing the #to_i to #round. Arguably, the modulo division step is superfluous - but only if you always pass in rationals less than one. On the other hand, The regex parsing of time strings in #fast_string_to_time is both clunky and incorrect for some values (as noted and fixed by Jacob). I''ve taken the first approach hinted at by Jacob and used a more direct regex to return only integers. The result is a method that is faster (roughly 5%), accurate (no more truncation issues) and arguably cleaner (no more double conversion). It should be completely backwards compatible as well -I''ve defined a new constant. The benchmarks are here: http://gist.github.com/278185 And the patch is attached to Jacob''s ticket here: https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/3693 On Jan 14, 10:20 am, Jacob Lauemøller <jacob.lauemoel...@iteray.com> wrote:> Hi all, > > The microsecond handling in ActiveRecord::ConnectionAdapters::Column#fast_string_to_time and ActiveRecord::ConnectionAdapters::Column#microseconds fail for some values. > > In slightly more than 1% of all possible 6-digit cases, writing a timestamp to a database column and then reading it back in results in a different value being returned to the program. > > So, for instance, saving the timestamp > > 2010-01-12 12:34:56.125014 > > and then loading it again from the database yields > > 2010-01-12 12:34:56.125013 > > The problem occurs when the value read is converted from string form to a Ruby timestamp, so it is largely database independent (the exception being drivers that override the methods, or databases that don''t support timestamps at all). > > The underlying problem is the use of to_i to convert from floats to ints inside the affected methods. As you know, to_i simply truncates the result and in some cases this causes rounding errors introduced by inherent inaccuracies in the multiplication operations and decimal representation to bubble up and affect the least significant digit. > > Here''s a simple test that illustrates the problem: > > converted = ActiveRecord::ConnectionAdapters::Column.send(''fast_string_to_time'', "2010-01-12 12:34:56.125014") > assert_equal 125014, converted.usec > > This test case (and a similar one for #microseconds) fail on plain vanilla Rails 2.3.5. > > I guess the best solution would be to change the ISO_DATETIME regex used to extract the microsecond-part from timestamps to not include the decimal point at all and then avoid the to_f and subsequent floating point multiplication completely inside the failing methods. However, these regexes are defined as constants on ActiveRecord::ConnectionAdapters::Column::Format and therefore publicly available, so the impact of changing these is difficult to ascertain. > > A simpler solution is to use round() instead of to_i to convert from the intermediate floating point result to int. This works (I have verified that the precision is sufficient for all possible 6-digit cases) but is about 15% slower than the current method. A small price to pay for correctness, in my opinion. > > I have attached a tiny patch (against 2.3.5) that switches the code to using round() and a test case that verifies that the method works for a few problematic cases that fail without the patch. > > I have also created a Lighthouse ticket #3693: > > https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/3... > > Could some of you please take a look and see if the patch is acceptable and maybe carry it into the code base? > > Cheers > Jacob > > fix_microsecond_conversion.diff > 4KViewDownload > >-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.