BUG #2056: to_char no long takes time as input?

On Sun, Nov 20, 2005 at 07:53:50AM +0000, Nick Addington wrote:

The following code works in 8.0.4 but fails in 8.1.0:

select to_char('1:00 pm'::time,'HH:MM AM');

8.1.0 gives this is the error message:
ERROR: invalid format specification for an interval value
HINT: Intervals are not tied to specific calendar dates.

I saw some discussion on the -hackers list about deprecating
to_char(interval, text), but do you really want to chuck to_char(time,
text)? That's a useful function. Or at least, I was using it...

to_char(time,text) doesn't exist, at least not in 7.3 and later --
you can see that with "\df to_char" in psql. If you set debug_print_parse
to on and set client_min_messages to debug1, you'll see that the
function being called is funcid 1768, which is

test=> select 1768::regprocedure;
regprocedure
------------------------
to_char(interval,text)
(1 row)

You'll also see that this function's first argument is a function
expression with funcid 1370, which is

test=> select 1370::regprocedure;
regprocedure
------------------------------------
"interval"(time without time zone)
(1 row)

So the time value is first converted to an interval and then passed
to to_char(interval,text).

test=> select "interval"('1:00 pm'::time);
interval
----------
13:00:00
(1 row)

test=> select to_char('13:00:00'::interval,'HH:MM AM');
ERROR: invalid format specification for an interval value
HINT: Intervals are not tied to specific calendar dates.

This looks like the commit that changed the behavior in 8.1 (the
hint was added later):

http://archives.postgresql.org/pgsql-committers/2005-08/msg00200.php

--
Michael Fuhr

"Nick Addington" <adding@math.wisc.edu> writes:

The following code works in 8.0.4 but fails in 8.1.0:
select to_char('1:00 pm'::time,'HH:MM AM');

I'm inclined to think that disallowing AM/PM for intervals was a mistake
--- if we allow both HH and HH24 (which we do) then the various flavors
of AM/PM seem reasonable to allow as well.

Bruce, did you have a specific rationale for that?

I notice the code now specifically forces HH to be treated as HH24 in
the interval case, which was not so before 8.1; we would need to revert
that change as well to continue supporting TIME cases reasonably.

regards, tom lane

Tom Lane wrote:

"Nick Addington" <adding@math.wisc.edu> writes:

The following code works in 8.0.4 but fails in 8.1.0:
select to_char('1:00 pm'::time,'HH:MM AM');

I'm inclined to think that disallowing AM/PM for intervals was a mistake
--- if we allow both HH and HH24 (which we do) then the various flavors
of AM/PM seem reasonable to allow as well.

Bruce, did you have a specific rationale for that?

I notice the code now specifically forces HH to be treated as HH24 in
the interval case, which was not so before 8.1; we would need to revert
that change as well to continue supporting TIME cases reasonably.

When I coded it I was thinking it doesn't make sense to allow "AM"/"PM" for
something like "5 hours". I disallowed anything that tied to a specific
timestamp value, even BC/AD.

I didn't realize "time" was used as input to to_char() as an interval.
I can see "time" as anchoring to midnight and we could then allow AM/PM.
I see your issue with HH/HH24, but I wanted this to work:

test=> select to_char('14 hours'::interval, 'HH');
to_char
---------
14
(1 row)

With the HH/HH24 change that is going to return 2. Do interval folks
know they would have to use HH24 for intervals? It doesn't seem 100%
clear, but I suppose we could just add a documentation about it, because
consider this:

test=> select to_char('44 hours'::interval, 'HH');
to_char
---------
44
(1 row)

With "HH" changed that would return 32. Ewe. Here is the formatting.c
code:

if (is_interval)
sprintf(inout, "%0*d", S_FM(suf) ? 0 : 2, tm->tm_hour);
else
sprintf(inout, "%0*d", S_FM(suf) ? 0 : 2,
tm->tm_hour == 0 ? 12 :
tm->tm_hour < 13 ? tm->tm_hour : tm->tm_hour - 12);

Should we subtract 12 only if the time is < 24. That also seems
strange. Also, a zero hour interval to HH would return 12, not 0.

It also seems strange to use HH24 for a value that might be greater than
24 (hours), while 'time' can not.

Anyway, I am thinking the easiest solution is to allow 'time' work
easily with HH, and to document that HH24 needs to be used for
intervals.

If this is what we want, I can make the change.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I see your issue with HH/HH24, but I wanted this to work:

test=> select to_char('14 hours'::interval, 'HH');
to_char
---------
14
(1 row)

With the HH/HH24 change that is going to return 2. Do interval folks
know they would have to use HH24 for intervals?

Dunno if they know it, but they always had to do it that way before 8.1,
so it's not a change to require it. I get this in everything back to
7.2:

regression=# select to_char('14 hours'::interval, 'HH');
to_char
---------
02
(1 row)

regression=# select to_char('14 hours'::interval, 'HH24');
to_char
---------
14
(1 row)

and I don't see anything especially wrong with that behavior, as long as
it's documented.

Should we subtract 12 only if the time is < 24. That also seems
strange. Also, a zero hour interval to HH would return 12, not 0.

Offhand I'd vote for making the HH code use a "mod 12" calculation,
and making AM/PM depend on the value "mod 24". This gives at least a
slightly sane behavior for intervals > 24 hours.

regards, tom lane

Here is a patch that allows HH and HH12 to be used with interval, and
hence time. I added documentation to warn users that using those with
intervals are mapped to single-day values.

I will soon apply this to HEAD and 8.1.X.

---------------------------------------------------------------------------

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I see your issue with HH/HH24, but I wanted this to work:

test=> select to_char('14 hours'::interval, 'HH');
to_char
---------
14
(1 row)

With the HH/HH24 change that is going to return 2. Do interval folks
know they would have to use HH24 for intervals?

Dunno if they know it, but they always had to do it that way before 8.1,
so it's not a change to require it. I get this in everything back to
7.2:

regression=# select to_char('14 hours'::interval, 'HH');
to_char
---------
02
(1 row)

regression=# select to_char('14 hours'::interval, 'HH24');
to_char
---------
14
(1 row)

and I don't see anything especially wrong with that behavior, as long as
it's documented.

Should we subtract 12 only if the time is < 24. That also seems
strange. Also, a zero hour interval to HH would return 12, not 0.

Offhand I'd vote for making the HH code use a "mod 12" calculation,
and making AM/PM depend on the value "mod 24". This gives at least a
slightly sane behavior for intervals > 24 hours.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Oh, one more thing. With the new patch I just posted, '0 hours' and
'HH' returns 12:

test=> select to_char('0 hours'::interval, 'HH');
to_char
---------
12
(1 row)

Of course HH24 works fine.

---------------------------------------------------------------------------

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I see your issue with HH/HH24, but I wanted this to work:

test=> select to_char('14 hours'::interval, 'HH');
to_char
---------
14
(1 row)

With the HH/HH24 change that is going to return 2. Do interval folks
know they would have to use HH24 for intervals?

Dunno if they know it, but they always had to do it that way before 8.1,
so it's not a change to require it. I get this in everything back to
7.2:

regression=# select to_char('14 hours'::interval, 'HH');
to_char
---------
02
(1 row)

regression=# select to_char('14 hours'::interval, 'HH24');
to_char
---------
14
(1 row)

and I don't see anything especially wrong with that behavior, as long as
it's documented.

Should we subtract 12 only if the time is < 24. That also seems
strange. Also, a zero hour interval to HH would return 12, not 0.

Offhand I'd vote for making the HH code use a "mod 12" calculation,
and making AM/PM depend on the value "mod 24". This gives at least a
slightly sane behavior for intervals > 24 hours.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Oh, one more thing. With the new patch I just posted, '0 hours' and
'HH' returns 12:

test=> select to_char('0 hours'::interval, 'HH');
to_char
---------
12
(1 row)

Yeah, it's done that in every release since 7.2. 8.1.0 is the only
release that thinks 00 is correct.

regards, tom lane

Patch applied to HEAD and 8.1.X.

---------------------------------------------------------------------------

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I see your issue with HH/HH24, but I wanted this to work:

test=> select to_char('14 hours'::interval, 'HH');
to_char
---------
14
(1 row)

With the HH/HH24 change that is going to return 2. Do interval folks
know they would have to use HH24 for intervals?

Dunno if they know it, but they always had to do it that way before 8.1,
so it's not a change to require it. I get this in everything back to
7.2:

regression=# select to_char('14 hours'::interval, 'HH');
to_char
---------
02
(1 row)

regression=# select to_char('14 hours'::interval, 'HH24');
to_char
---------
14
(1 row)

and I don't see anything especially wrong with that behavior, as long as
it's documented.

Should we subtract 12 only if the time is < 24. That also seems
strange. Also, a zero hour interval to HH would return 12, not 0.

Offhand I'd vote for making the HH code use a "mod 12" calculation,
and making AM/PM depend on the value "mod 24". This gives at least a
slightly sane behavior for intervals > 24 hours.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073