Bug #72416 for DateTime: When rounding nanoseconds in strftime, nanoseconds that round to a full second display incorrectly

Tue Jun 07 11:16:44 2011 frioux [...] gmail.com - Ticket #68686: Ticket created

Subject:

%3N does the wrong thing when it should round up

See test

Subject:

fail.t

#!/usr/bin/env perl use strict; use warnings; use Test::More; use DateTime::Format::Strptime; use DateTime; my $format = DateTime::Format::Strptime->new( pattern => '%Y-%m-%d %H:%M:%S.%3N' ); is $format->format_datetime(DateTime->from_epoch( epoch => 1307459483.99950 )), '2011-06-07 15:11:24.000', 'microseconds round up correctly'; done_testing;

Mon Nov 14 12:57:32 2011 shawn.c.carroll [...] gmail.com - Ticket created

I think I found a bug in DateTime's strftime. If you have a nanosecond value that rounds up to a full second, it returns instead a tenth of a second. Pls see the example code and output for details. Am I seeing this correctly? If so I will submit a bug to the maintainers. Sample Code: use strict; use warnings; use DateTime; use DateTime::Format::Strptime; my $val = "2011-11-11 14:30:00.999981"; my $format = new DateTime::Format::Strptime( pattern => '%F %T.%N', time_zone => 'GMT', ); my $date = $format->parse_datetime($val); print $date->strftime("%F %T.%3N %Z")."\n"; Output: 2011-11-11 14:30:00.1000 UTC I would expect: 2011-11-11 14:30:01.000 UTC

Mon Nov 14 13:32:42 2011 DOUGW [...] cpan.org - Correspondence added

Possible fix to truncate rather than round when nanoseconds round up to a second: sub millisecond { $_[0]->_format_nanosecs(3) } sub microsecond { $_[0]->_format_nanosecs(6) } sub _format_nanosecs { my $self = shift; my $precision = @_ ? shift : 9; my $divide_by = 10**( 9 - $precision ); my $t = sprintf( '%0' . $precision . 'u', round( $self->{rd_nanosecs} / $divide_by ) ); return ( length($t) > $precision ) ? "9" x $precision : $t; }

Mon Nov 14 13:32:43 2011 The RT System itself - Status changed from 'new' to 'open'

Mon Nov 14 13:34:57 2011 shawn.c.carroll [...] gmail.com - Correspondence added

Subject:	Nanosecond Rounding issue
From:	shawn.c.carroll [...] gmail.com

If you truncate then you are undoing RT #66744 On Mon Nov 14 13:32:42 2011, DOUGW wrote: Show quoted text

> > Possible fix to truncate rather than round when nanoseconds round up to > a second: > > sub millisecond { $_[0]->_format_nanosecs(3) } > > sub microsecond { $_[0]->_format_nanosecs(6) } > > sub _format_nanosecs { > my $self = shift; > my $precision = @_ ? shift : 9; > > my $divide_by = 10**( 9 - $precision ); > > my $t = sprintf( > '%0' . $precision . 'u', > round( $self->{rd_nanosecs} / $divide_by ) > ); > return ( length($t) > $precision ) ? "9" x $precision : $t; > }

Mon Nov 14 13:38:28 2011 DOUGW [...] cpan.org - Correspondence added

On Mon Nov 14 13:34:57 2011, shawn.c.carroll wrote: Show quoted text

> If you truncate then you are undoing RT #66744 > > On Mon Nov 14 13:32:42 2011, DOUGW wrote:

> > > > Possible fix to truncate rather than round when nanoseconds round up to

FYI, first discussed over at: http://perlmonks.org/?node_id=937996

Mon Nov 14 18:40:16 2011 DOUGW [...] cpan.org - Correspondence added

On Mon Nov 14 13:34:57 2011, shawn.c.carroll wrote: Show quoted text

> If you truncate then you are undoing RT #66744

No, it only truncates if the rounded nanoseconds round up to 1 second. And although not idesl, this fix is simpler than trying to carry over the extra second to the seconds place, and is better than nothing (at least until a better solution is proposed and implemented).

Fri Nov 18 14:25:38 2011 DROLSKY [...] cpan.org - Subject changed from (no value) to 'Nanoseconds which should round up to a full second instead round down to zero'

Tue Nov 22 17:52:05 2011 DOUGW [...] cpan.org - Correspondence added

Just wanted to mention to Mr. Rolsky that the re-title is not really correct. See the first post in this RT thread. If you want to display 3 decimal places, the value gets "rounded" to "1000", which displays as ".1000". You could say it gets "rounded" to 1 + ("9" x $number_of_decimal_places). My proposed patch would round correctly except when rounding up to a second, in which case it would truncate to "9" x $number_of_decimal places. Not ideal, but much better than what it is currently doing, and much simpler to implement in the meantime than the ideal solution.

Sat Nov 26 17:13:54 2011 autarch [...] urth.org - Correspondence added

Subject:	Re: [rt.cpan.org #72416] Nanoseconds which should round up to a full second instead round down to zero
Date:	Sat, 26 Nov 2011 16:13:45 -0600 (CST)
To:	Douglas Wilson via RT <bug-DateTime [...] rt.cpan.org>
From:	Dave Rolsky <autarch [...] urth.org>

On Tue, 22 Nov 2011, Douglas Wilson via RT wrote: Show quoted text

> Just wanted to mention to Mr. Rolsky that the re-title is not really > correct. See the first post in this RT thread. If you want to display 3

Please suggest a better title. Thanks, -dave /*============================================================ http://VegGuide.org http://blog.urth.org Your guide to all that's veg House Absolute(ly Pointless) ============================================================*/

Fri Dec 16 09:27:33 2011 frioux [...] gmail.com - Correspondence added

Show quoted text

> Please suggest a better title.

How about the one I used here: https://rt.cpan.org/Public/Bug/Display.html?id=68686 (%3N does the wrong thing when it should round up)

Fri Dec 16 09:37:51 2011 frioux [...] gmail.com - Correspondence added

Ok, for what it's worth I've included a failing test for this bug. To apply it just download the patch and run "git am $filename"

Subject:

0001-add-test-for-RT-72416-RT-68686.patch

From 6f88973d56d2faab5163102222007c7a6a57ebb7 Mon Sep 17 00:00:00 2001 From: Arthur Axel 'fREW' Schmidt <frioux@gmail.com> Date: Fri, 16 Dec 2011 08:30:24 -0600 Subject: [PATCH] add test for RT#72416/RT#68686 --- t/901_rt_68686.t | 18 ++++++++++++++++++ 1 files changed, 18 insertions(+), 0 deletions(-) create mode 100644 t/901_rt_68686.t diff --git a/t/901_rt_68686.t b/t/901_rt_68686.t new file mode 100644 index 0000000..6aae18d --- /dev/null +++ b/t/901_rt_68686.t @@ -0,0 +1,18 @@ +#!/usr/bin/env perl + +use strict; +use warnings; + +use Test::More; + +use DateTime; + +my $dt = DateTime->from_epoch( epoch => 1307459483.99950 ); + +is( + $dt->strftime('%Y-%m-%d %H:%M:%S.%3N'), + '2011-06-07 15:11:24.000', + 'microseconds round up correctly', +); + +done_testing; -- 1.7.8

Wed Jan 25 15:45:23 2012 DOUGW [...] cpan.org - Correspondence added

On Sat Nov 26 17:13:54 2011, autarch@urth.org wrote: Show quoted text

> On Tue, 22 Nov 2011, Douglas Wilson via RT wrote: > > Please suggest a better title.

I agree with frioux about the title. Also, attached is a patch for DateTime.pm and the strftime tests. This patch should make it so strftime correctly rounds up when fractions of a second round up to a full second.

Subject:

RT68686.patch

diff -r -u DateTime-0.72/lib/DateTime.pm Tst-DateTime-0.72/lib/DateTime.pm --- DateTime-0.72/lib/DateTime.pm Thu Jan 5 12:42:04 2012 +++ Tst-DateTime-0.72/lib/DateTime.pm Wed Jan 25 12:30:30 2012 @@ -1038,7 +1038,7 @@ $strftime_patterns{h} = $strftime_patterns{b}; sub strftime { - my $self = shift; + my $self = shift->clone(); # make a copy or caller's scalars get munged my @patterns = @_; @@ -1045,6 +1045,15 @@ my @r; foreach my $p (@patterns) { + if ( $p =~ /(%+)(\d+)N/ ) { + my ( $pct, $precision ) = ($1, $2); + next unless length($pct) % 2 > 0; + my ($n, $carry) = _format_nanosecs($self, $precision); + $p =~ s/%\d+N/$n/; + $self->add(seconds => 1) if $carry; + } + } + foreach my $p (@patterns) { $p =~ s/ (?: %{(\w+)} # method name like %{day_name} @@ -1303,10 +1312,10 @@ my $divide_by = 10**( 9 - $precision ); - return sprintf( + my $t = sprintf( '%0' . $precision . 'u', - round( $self->{rd_nanosecs} / $divide_by ) - ); + round( $self->{rd_nanosecs} / $divide_by ) ); + return ( length($t) > $precision ) ? ("0" x $precision,1) : $t; } sub epoch { diff -r -u DateTime-0.72/t/13strftime.t Tst-DateTime-0.72/t/13strftime.t --- DateTime-0.72/t/13strftime.t Thu Jan 5 12:42:04 2012 +++ Tst-DateTime-0.72/t/13strftime.t Wed Jan 25 11:31:33 2012 @@ -170,6 +170,17 @@ } } +{ + for my $i (2..9) { + my $dt = DateTime->new( year => 2012, nanosecond => "9" x $i . "0" x (9 - $i) ); + my $precision = $i - 1; + #my $dt = DateTime->new( year => 2012, nanosecond => 999900000 ); + my $spec = '%S.%' . $precision . 'N'; + my $expect = '01.' . "0" x $precision; + is( $dt->strftime($spec), $expect, "strftime $spec nanoseconds rounding up to a second" ); + } +} + done_testing(); # add these if we do roman-numeral stuff

Wed Jan 25 16:05:41 2012 DOUGW [...] cpan.org - Correspondence added

On Wed Jan 25 15:45:23 2012, DOUGW wrote: Show quoted text

> On Sat Nov 26 17:13:54 2011, autarch@urth.org wrote:

> > On Tue, 22 Nov 2011, Douglas Wilson via RT wrote:

> > Also, attached is a patch for DateTime.pm and the strftime tests.

Despite the name of the patch, this RT is 72416, but the patch also fixes RT 68686

Wed Jan 25 16:53:00 2012 DOUGW [...] cpan.org - Correspondence added

On Wed Jan 25 15:45:23 2012, DOUGW wrote: Show quoted text

> On Sat Nov 26 17:13:54 2011, autarch@urth.org wrote: > > Also, attached is a patch for DateTime.pm and the strftime tests.

Darn, this patch does not work if you pass strftime a list of formats. I'll keep trying.

Wed Jan 25 18:27:35 2012 DOUGW [...] cpan.org - Correspondence added

On Wed Jan 25 16:53:00 2012, DOUGW wrote: Show quoted text

> On Wed Jan 25 15:45:23 2012, DOUGW wrote: > > I'll keep trying.

This works. The tests even test a list of formats to strftime.

Subject:

RT72416_patch.diff

diff -r -u DateTime-0.72/lib/DateTime.pm Tst-DateTime-0.72/lib/DateTime.pm --- DateTime-0.72/lib/DateTime.pm Thu Jan 5 12:42:04 2012 +++ Tst-DateTime-0.72/lib/DateTime.pm Wed Jan 25 15:17:30 2012 @@ -999,7 +999,6 @@ 'm' => sub { sprintf( '%02d', $_[0]->month ) }, 'M' => sub { sprintf( '%02d', $_[0]->minute ) }, 'n' => sub {"\n"}, # should this be OS-sensitive? - 'N' => \&_format_nanosecs, 'p' => sub { $_[0]->am_or_pm() }, 'P' => sub { lc $_[0]->am_or_pm() }, 'r' => sub { $_[0]->strftime('%I:%M:%S %p') }, @@ -1045,6 +1044,15 @@ my @r; foreach my $p (@patterns) { + + my $tmp_self = $self; + if ( $p =~ /(?<!%)((?:%%)*)%(\d*)N/ ) { + my ( $pct, $precision ) = ($1, $2); + my ($n, $carry) = _format_nanosecs($self, $precision); + $p =~ s/(?<!%)$pct%${precision}N/$n/; + $tmp_self = $tmp_self->clone->add(seconds => 1) if $carry; + } + $p =~ s/ (?: %{(\w+)} # method name like %{day_name} @@ -1055,11 +1063,11 @@ ) / ( $1 - ? ( $self->can($1) ? $self->$1() : "\%{$1}" ) + ? ( $tmp_self->can($1) ? $tmp_self->$1() : "\%{$1}" ) : $2 - ? ( $strftime_patterns{$2} ? $strftime_patterns{$2}->($self) : "\%$2" ) + ? ( $strftime_patterns{$2} ? $strftime_patterns{$2}->($tmp_self) : "\%$2" ) : $3 - ? $strftime_patterns{N}->($self, $3) + ? $strftime_patterns{N}->($tmp_self, $3) : '' # this won't happen ) /sgex; @@ -1299,14 +1307,14 @@ sub _format_nanosecs { my $self = shift; - my $precision = @_ ? shift : 9; + my $precision = @_ ? shift || 9 : 9; my $divide_by = 10**( 9 - $precision ); - return sprintf( + my $t = sprintf( '%0' . $precision . 'u', - round( $self->{rd_nanosecs} / $divide_by ) - ); + round( $self->{rd_nanosecs} / $divide_by ) ); + return ( length($t) > $precision ) ? ("0" x $precision,1) : $t; } sub epoch { diff -r -u DateTime-0.72/t/13strftime.t Tst-DateTime-0.72/t/13strftime.t --- DateTime-0.72/t/13strftime.t Thu Jan 5 12:42:04 2012 +++ Tst-DateTime-0.72/t/13strftime.t Wed Jan 25 14:35:07 2012 @@ -170,6 +170,22 @@ } } +{ + for my $i (2..9) { + my $dt = DateTime->new( year => 2012, nanosecond => "9" x $i . "0" x (9 - $i) ); + my $precision = $i - 1; + my $spec1 = '%S.%' . $precision . 'N'; + my $spec2 = '%S.%' . ($precision + 1) . 'N'; + my $expect1 = '01.' . "0" x $precision; + my $expect2 = '00.' . "9" x ( $precision + 1 ); + my @dt_str = $dt->strftime($spec1, $spec1, $spec2); + is( scalar(@dt_str), 3, "strftime correct number of results"); + is( $dt_str[0], $expect1, "strftime $spec1 nanoseconds rounding up to a second" ); + is( $dt_str[1], $expect1, "strftime $spec1 nanoseconds rounding up to a second - repeat" ); + is( $dt_str[2], $expect2, "strftime $spec2 nanoseconds no round" ); + } +} + done_testing(); # add these if we do roman-numeral stuff

Thu Jan 26 11:06:19 2012 DOUGW [...] cpan.org - Correspondence added

On Wed Jan 25 18:27:35 2012, DOUGW wrote: Show quoted text

> On Wed Jan 25 16:53:00 2012, DOUGW wrote: > > This works. The tests even test a list of formats to strftime.

Now that I've written that, I'm not even sure that rounding nanoseconds is such a great idea. Say that it's a few nanoseconds before the new year. Do you really want to round up to the next year (or even the next day, etc.)? I'd say that there is no correct behavior, and either truncating or rounding can be a reasonable behavior, but the current behavior is definitely incorrect, so it should be fixed in some way.

Fri Jan 27 11:21:06 2012 DOUGW [...] cpan.org - Correspondence added

On Sat Nov 26 17:13:54 2011, autarch@urth.org wrote: Show quoted text

> On Tue, 22 Nov 2011, Douglas Wilson via RT wrote: > > Please suggest a better title.

How about: Nanoseconds that round to a full second display incorrectly Possibly with (if it's not already too long): ...and do not carry over to seconds

Fri Feb 03 11:17:08 2012 DROLSKY [...] cpan.org - Subject changed from 'Nanoseconds which should round up to a full second instead round down to zero' to 'When rounding nanoseconds in strftime, nanoseconds that round to a full second display incorrectly'

Tue Feb 07 18:37:52 2012 DOUGW [...] cpan.org - Correspondence added

On Thu Jan 26 11:06:19 2012, DOUGW wrote: Show quoted text

> On Wed Jan 25 18:27:35 2012, DOUGW wrote:

> > On Wed Jan 25 16:53:00 2012, DOUGW wrote: > > > > This works. The tests even test a list of formats to strftime.

> > Now that I've written that, I'm not even sure that rounding nanoseconds > is such a great idea.

And here's a patch that rounds, unless it's supposed to round up to a second. Then it truncates to however many 9's the precision dictates. Pick this patch, or the previous, so that strftime does something reasonable :-)

Subject:

RT72416_patch_trunc.diff.txt

diff -r -u DateTime-0.72/lib/DateTime.pm Tst-DateTime-0.72/lib/DateTime.pm --- DateTime-0.72/lib/DateTime.pm Thu Jan 5 12:42:04 2012 +++ Tst-DateTime-0.72/lib/DateTime.pm Wed Jan 25 15:17:30 2012 @@ -1299,14 +1307,14 @@ sub _format_nanosecs { my $self = shift; - my $precision = @_ ? shift : 9; + my $precision = @_ ? shift || 9 : 9; my $divide_by = 10**( 9 - $precision ); - return sprintf( + my $t = sprintf( '%0' . $precision . 'u', - round( $self->{rd_nanosecs} / $divide_by ) - ); + round( $self->{rd_nanosecs} / $divide_by ) ); + return ( length($t) > $precision ) ? "9" x $precision : $t; } sub epoch { diff -r -u DateTime-0.72/t/13strftime.t Tst-DateTime-0.72/t/13strftime.t --- DateTime-0.72/t/13strftime.t Thu Jan 5 12:42:04 2012 +++ Tst-DateTime-0.72/t/13strftime.t Wed Jan 25 14:35:07 2012 @@ -170,6 +170,22 @@ } } +{ + for my $i (2..9) { + my $dt = DateTime->new( year => 2012, nanosecond => "9" x $i . "0" x (9 - $i) ); + my $precision = $i - 1; + my $spec1 = '%S.%' . $precision . 'N'; + my $spec2 = '%S.%' . ($precision + 1) . 'N'; + my $expect1 = '00.' . "9" x $precision; + my $expect2 = '00.' . "9" x ( $precision + 1 ); + my @dt_str = $dt->strftime($spec1, $spec1, $spec2); + is( scalar(@dt_str), 3, "strftime correct number of results"); + is( $dt_str[0], $expect1, "strftime $spec1 nanoseconds rounding up" ); + is( $dt_str[1], $expect1, "strftime $spec1 nanoseconds rounding up - repeat" ); + is( $dt_str[2], $expect2, "strftime $spec2 nanoseconds no round" ); + } +} + done_testing(); # add these if we do roman-numeral stuff

Thu Jul 05 00:48:44 2012 DROLSKY [...] cpan.org - Correspondence added

On Mon Nov 14 12:57:32 2011, shawn.c.carroll wrote: Show quoted text

> I think I found a bug in DateTime's strftime. If you have a nanosecond > value that rounds up to a full second, it returns instead a tenth of a > second. Pls see the example code and output for details. Am I seeing > this correctly? If so I will submit a bug to the maintainers. Sample

Code: Show quoted text

> > use strict; > use warnings; > use DateTime; > use DateTime::Format::Strptime; > my $val = "2011-11-11 14:30:00.999981"; > my $format = new DateTime::Format::Strptime( > pattern => '%F %T.%N', time_zone => 'GMT', ); > my $date = $format->parse_datetime($val); > print $date->strftime("%F %T.%3N %Z")."\n"; > > > Output: > > 2011-11-11 14:30:00.1000 UTC > > > I would expect: > > 2011-11-11 14:30:01.000 UTC

So I was thinking about this recently and I finally figured out the underlying problem. The %N format specifier is something I added to the strftime implementation in DateTime. It doesn't come from C. Clearly, it's not well thought out. What we _really_ need is a way to output seconds _and_ nanoseconds at a fixed precision. In that case, rounding the seconds up to the next integer would make perfect sense. I _don't_ want to add this to the strftime() implementation in DateTime, however. The future of DateTime formatting is the CLDR formats. Unfortunately, this format (as currently) defined has the same stupid problem of having one pattern for seconds and the other for fractional portions of a second.

Sun Jul 15 12:28:30 2012 DROLSKY [...] cpan.org - Status changed from 'open' to 'stalled'

Sun Jul 15 12:30:00 2012 DROLSKY [...] cpan.org - Ticket #68686: Merged into ticket #72416

Sun Jul 15 12:30:00 2012 DROLSKY [...] cpan.org - Merged into ticket #72416

Sun Nov 18 12:39:34 2012 DROLSKY [...] cpan.org - Reference to ticket #79845 added

Sun Nov 18 12:39:47 2012 DROLSKY [...] cpan.org - Status changed from 'stalled' to 'resolved'

Sun Nov 18 12:39:52 2012 DROLSKY [...] cpan.org - Fixed in 0.78 added

Bug #72416 for DateTime: When rounding nanoseconds in strftime, nanoseconds that round to a full second display incorrectly

Preferred bug tracker