Обсуждение: 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@gmail.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.Grittner@wicourts.gov> wrote: > Robert Haas <robertmhaas@gmail.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@gmail.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.Grittner@wicourts.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@snowman.net> wrote: > * Kevin Grittner (Kevin.Grittner@wicourts.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@snowman.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.14159@gmail.com> wrote: > On Thu, Mar 31, 2011 at 6:39 PM, Stephen Frost <sfrost@snowman.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@gmail.com> writes:
> On Fri, Apr 1, 2011 at 10:33 PM, Joseph Adams
> <joeyadams3.14159@gmail.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@gmail.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.14159@gmail.com> wrote: > On Sun, Apr 3, 2011 at 11:23 PM, Robert Haas <robertmhaas@gmail.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 sayinghis 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@gmail.com> writes:
> On Apr 4, 2011, at 1:46 AM, Joseph Adams <joeyadams3.14159@gmail.com> wrote:
>> On Sun, Apr 3, 2011 at 11:23 PM, Robert Haas <robertmhaas@gmail.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
banksaying 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@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Apr 4, 2011, at 1:46 AM, Joseph Adams <joeyadams3.14159@gmail.com> wrote: >>> On Sun, Apr 3, 2011 at 11:23 PM, Robert Haas <robertmhaas@gmail.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 banksaying 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.14159@gmail.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