This project is archived and is in readonly mode.
[PATCH] ActiveRecord timestamp conversions fail for some cases
-
Chris Hapgood
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
I've attached a new patch with extensive tests.
-
Levent
Pratik seemed to have reverted this commit with 0ab3063 so I guess the issue still stands in 2.3.x and 3.x
commit 0ab30637dd5bc7536c5accd66b45ce0263134a14 Author: Pratik Naik <pratiknaik@gmail.com> Date: Sun Jan 17 03:04:11 2010 +0530 Revert "Fix #microseconds conversion and #fast_string_to_time" This reverts commit 717a2941e15b32d07cc456bb0d81742ecfc5b4a3. Bunch of failures when running postgresql tests. -
Chris Hapgood
- Assigned user set to Pratik
Pratik, do you have any insight as to why this failed on postgresql?
-
Andrea Campi
- Tag changed from rails 2.3.5 timestamp bug patch to 2.3.5, patch, timestamp
