Re: cast from integer to money

On the open items list, we have:

conversion from integer literals to money type
http://archives.postgresql.org/pgsql-testers/2011-01/msg00000.php

What this is really complaining about is that we added a cast from
numeric to money, but not from integer to money. This isn't really a
bug: the fact that we added one cast doesn't oblige us to add two. On
the other hand, the change is probably harmless and straightforward,
and might reduce user confusion. Right now:

rhaas=# select 1::money;
ERROR: cannot cast type integer to money
LINE 1: select 1::money;
^
rhaas=# select 1.0::money;
money
-------
$1.00
(1 row)

Does anyone care enough about this to put in the effort to fix it, or
should we just let it go? Does anyone see a reason why we wouldn't
want to do this, if someone's motivated to code it up?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On the open items list, we have:
>
> conversion from integer literals to money type
> http://archives.postgresql.org/pgsql-testers/2011-01/msg00000.php
>
> What this is really complaining about is that we added a cast from
> numeric to money, but not from integer to money. This isn't
> really a bug: the fact that we added one cast doesn't oblige us to
> add two. On the other hand, the change is probably harmless and
> straightforward, and might reduce user confusion.

There were reasonable arguments made why this could be a bad idea --
primarily around the question of whether '395' represented $3.95 or
$395.00. Going the other way has issues with truncation of
fractions and the number of digits which can be handled. I'm not
convinced it's sane, and I feel strongly it's too late in the cycle
to try to implement.

-Kevin

On Thu, Mar 31, 2011 at 4:58 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On the open items list, we have:
>>
>> conversion from integer literals to money type
>> http://archives.postgresql.org/pgsql-testers/2011-01/msg00000.php
>>
>> What this is really complaining about is that we added a cast from
>> numeric to money, but not from integer to money.  This isn't
>> really a bug: the fact that we added one cast doesn't oblige us to
>> add two. On the other hand, the change is probably harmless and
>> straightforward, and might reduce user confusion.
>
> There were reasonable arguments made why this could be a bad idea --
> primarily around the question of whether '395' represented $3.95 or
> $395.00.

That's not too hard to figure out, right? If 1.00 means $1.00, 1 had
better not mean $0.01, or there will be riots in the streets.

> Going the other way has issues with truncation of
> fractions and the number of digits which can be handled.

Notice I didn't propose that.

> I'm not
> convinced it's sane, and I feel strongly it's too late in the cycle
> to try to implement.

Fair enough. Any contrary votes?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

>> There were reasonable arguments made why this could be a bad idea
>> -- primarily around the question of whether '395' represented
>> $3.95 or $395.00.
>
> That's not too hard to figure out, right? If 1.00 means $1.00, 1
> had better not mean $0.01, or there will be riots in the streets.
>
>> Going the other way has issues with truncation of
>> fractions and the number of digits which can be handled.
>
> Notice I didn't propose that.

If you're just talking about going in the one direction, I might be
persuaded that's sane, especially because of the case of literals,
and especially since there are currencies where fractional amounts
aren't used in the conventional representation.

-Kevin

* Kevin Grittner (Kevin(dot)Grittner(at)wicourts(dot)gov) wrote:
> If you're just talking about going in the one direction, I might be
> persuaded that's sane, especially because of the case of literals,
> and especially since there are currencies where fractional amounts
> aren't used in the conventional representation.

Going just integer->money, with the "1" -> "$1.00", seems completely
reasonable to me. As for being too late in the cycle.. if someone's
willing to do the work, I can't imagine it breaking anything, so I
wouldn't be against putting it in. It really should be before the
first beta tho.

Thanks,

Stephen

On Mar 31, 2011, at 6:39 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Kevin Grittner (Kevin(dot)Grittner(at)wicourts(dot)gov) wrote:
>> If you're just talking about going in the one direction, I might be
>> persuaded that's sane, especially because of the case of literals,
>> and especially since there are currencies where fractional amounts
>> aren't used in the conventional representation.
>
> Going just integer->money, with the "1" -> "$1.00", seems completely
> reasonable to me. As for being too late in the cycle.. if someone's
> willing to do the work, I can't imagine it breaking anything, so I
> wouldn't be against putting it in. It really should be before the
> first beta tho.

Agreed, emphatically.

...Robert

On Thu, Mar 31, 2011 at 6:39 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Going just integer->money, with the "1" -> "$1.00", seems completely
> reasonable to me.  As for being too late in the cycle..  if someone's
> willing to do the work, I can't imagine it breaking anything, so I
> wouldn't be against putting it in.  It really should be before the
> first beta tho.

Attached is a patch which enables casting int2/int4/int8 to money,
with the same scaling as numeric uses. Hence, 1::money yields '$1.00'
. The only other numeric types (other than oid, cardinal_number,
etc.) that can't be casted directly to money are float4 and float8,
and I suspect this is intentional.

The patch includes tests, but does not update the documentation.
Should the docs be updated where it reads "Values of the numeric data
type can be cast to money. Other numeric types can be converted to
money by casting to numeric first" ?

Because this change adds rows to the pg_proc and pg_cast catalogs,
applying this patch for 9.1 will require alpha users to initdb again.
Is that acceptable?

Regards,
Joey Adams

On Fri, Apr 1, 2011 at 10:33 PM, Joseph Adams
<joeyadams3(dot)14159(at)gmail(dot)com> wrote:
> On Thu, Mar 31, 2011 at 6:39 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> Going just integer->money, with the "1" -> "$1.00", seems completely
>> reasonable to me.  As for being too late in the cycle..  if someone's
>> willing to do the work, I can't imagine it breaking anything, so I
>> wouldn't be against putting it in.  It really should be before the
>> first beta tho.
>
> Attached is a patch which enables casting int2/int4/int8 to money,
> with the same scaling as numeric uses.  Hence, 1::money yields '$1.00'

Thanks for the patch, but I think you forgot to worry about overflow:

rhaas=# select 9223372036854775807::money;
money
--------
-$1.00
(1 row)

> .  The only other numeric types (other than oid, cardinal_number,
> etc.) that can't be casted directly to money are float4 and float8,
> and I suspect this is intentional.

Agreed.

> The patch includes tests, but does not update the documentation.
> Should the docs be updated where it reads "Values of the numeric data
> type can be cast to money. Other numeric types can be converted to
> money by casting to numeric first" ?

Yes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Apr 1, 2011 at 10:33 PM, Joseph Adams
> <joeyadams3(dot)14159(at)gmail(dot)com> wrote:
>> The only other numeric types (other than oid, cardinal_number,
>> etc.) that can't be casted directly to money are float4 and float8,
>> and I suspect this is intentional.

> Agreed.

BTW, I think inclusion of int2 in this patch is just a waste of code
space. The main argument for supporting these casts seems to be that
integer literals produced by the parser should be castable to money
without special pushups. But the parser never produces native int2
constants. So int4 and int8 will cover all the useful cases.

regards, tom lane

On Sun, Apr 3, 2011 at 11:23 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Thanks for the patch, but I think you forgot to worry about overflow:
>
> rhaas=# select 9223372036854775807::money;
>  money
> --------
>  -$1.00
> (1 row)

cash_in doesn't test for overflow, either (tested on 8.4.0, 9.0.3, and HEAD):

joey=# select '9223372036854775807'::money;
money
--------
-$1.00
(1 row)

Is this a bug?

Detail: unlike cash_in, numeric_cash does check for overflow
(implicitly, through its use of numeric_int8).

Joey Adams

On Apr 4, 2011, at 1:46 AM, Joseph Adams <joeyadams3(dot)14159(at)gmail(dot)com> wrote:
> On Sun, Apr 3, 2011 at 11:23 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Thanks for the patch, but I think you forgot to worry about overflow:
>>
>> rhaas=# select 9223372036854775807::money;
>> money
>> --------
>> -$1.00
>> (1 row)
>
> cash_in doesn't test for overflow, either (tested on 8.4.0, 9.0.3, and HEAD):
>
> joey=# select '9223372036854775807'::money;
> money
> --------
> -$1.00
> (1 row)
>
> Is this a bug?

Seems like it. You have to feel sorry for the guy who deposits 9 quintillion dollars and then gets a note from the bank saying his account is overdrawn...

> Detail: unlike cash_in, numeric_cash does check for overflow
> (implicitly, through its use of numeric_int8).

Yeah.

...Robert

On Mon, April 4, 2011 7:02 am, Robert Haas wrote:
> You have to feel sorry for the guy who deposits 9
> quintillion dollars and then gets a note from the bank saying his account
> is overdrawn...
>

Not really ...

cheers

andrew

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Apr 4, 2011, at 1:46 AM, Joseph Adams <joeyadams3(dot)14159(at)gmail(dot)com> wrote:
>> On Sun, Apr 3, 2011 at 11:23 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> Thanks for the patch, but I think you forgot to worry about overflow:

>> cash_in doesn't test for overflow, either (tested on 8.4.0, 9.0.3, and HEAD):
>> Is this a bug?

> Seems like it. You have to feel sorry for the guy who deposits 9 quintillion dollars and then gets a note from the bank saying his account is overdrawn...

I'm fairly sure that *none* of the money operations bother to check for
overflow; not only input, but arithmetic. That falls somewhere between
bug and missing feature. It's probably worth fixing but seems outside
the scope of the current patch.

In the meantime, I'm not sure whether the newly added functions should
be held to a higher standard than the existing ones. It might be better
to leave it be, and plan to fix them all at once in a consistent style.

regards, tom lane

On Mon, Apr 4, 2011 at 10:58 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Apr 4, 2011, at 1:46 AM, Joseph Adams <joeyadams3(dot)14159(at)gmail(dot)com> wrote:
>>> On Sun, Apr 3, 2011 at 11:23 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> Thanks for the patch, but I think you forgot to worry about overflow:
>
>>> cash_in doesn't test for overflow, either (tested on 8.4.0, 9.0.3, and HEAD):
>>> Is this a bug?
>
>> Seems like it. You have to feel sorry for the guy who deposits 9 quintillion dollars and then gets a note from the bank saying his account is overdrawn...
>
> I'm fairly sure that *none* of the money operations bother to check for
> overflow; not only input, but arithmetic.  That falls somewhere between
> bug and missing feature.  It's probably worth fixing but seems outside
> the scope of the current patch.

Oh. Bummer. Yeah, that sounds more like a TODO than an open item.

> In the meantime, I'm not sure whether the newly added functions should
> be held to a higher standard than the existing ones.  It might be better
> to leave it be, and plan to fix them all at once in a consistent style.

Maybe. The numeric->money cast does handle it though, so there's at
least some precedent for checking. If you don't want to worry about
it, I'm OK with just putting it in as-is, but I'd probably be inclined
to look for a way to fix it if we can do that without adding too much
complexity.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attached is an updated version of the patch to allow conversion of
int4/int8 directly to money. I added overflow checks, dropped
int2->cash, and updated the documentation.

- Joey

On Tue, Apr 5, 2011 at 1:10 AM, Joseph Adams <joeyadams3(dot)14159(at)gmail(dot)com> wrote:
> Attached is an updated version of the patch to allow conversion of
> int4/int8 directly to money.  I added overflow checks, dropped
> int2->cash, and updated the documentation.

Excellent, thanks.

My only gripe is that I don't think we should duplicate int8mul, so
I've changed your patch to use this incantation:

+ result = DatumGetInt64(DirectFunctionCall2(int8mul, Int64GetDatum(amount
+ Int64GetDatum(scale)));

...which is parallel to what the existing numeric -> money cast
already does. That results in a slightly different error message, but
I think that's OK: no one has complained about the numeric -> cash
error message, or the fact that the remaining functions in this module
do no overflow checking at all.

With that change, committed. Thanks for picking this one up.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company