Fixes for MONEY type using locale
In looking at the use of the thousands separator in formatting.c and
psql/print.c, I now see similar problems with the MONEY type, namely
that if the thousands separator is "" it is set to "," even if the
decimal separator is ",".
The attached patch fixes this.
I also removed a ssymbol test because ssymbol will never be \0 based on
the code. Also, what does this do:
if (buf[LAST_DIGIT] == ',')
buf[LAST_DIGIT] = buf[LAST_PAREN];
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Attachments:
/pgpatches/moneytext/x-diffDownload
Index: src/backend/utils/adt/cash.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/cash.c,v
retrieving revision 1.75
diff -c -c -r1.75 cash.c
*** src/backend/utils/adt/cash.c 23 Nov 2007 19:54:39 -0000 1.75
--- src/backend/utils/adt/cash.c 23 Nov 2007 20:04:38 -0000
***************
*** 148,154 ****
fpoint = 2; /* best guess in this case, I think */
dsymbol = ((*lconvert->mon_decimal_point != '\0') ? *lconvert->mon_decimal_point : '.');
! ssymbol = ((*lconvert->mon_thousands_sep != '\0') ? *lconvert->mon_thousands_sep : ',');
csymbol = ((*lconvert->currency_symbol != '\0') ? lconvert->currency_symbol : "$");
psymbol = ((*lconvert->positive_sign != '\0') ? *lconvert->positive_sign : '+');
nsymbol = ((*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-");
--- 148,158 ----
fpoint = 2; /* best guess in this case, I think */
dsymbol = ((*lconvert->mon_decimal_point != '\0') ? *lconvert->mon_decimal_point : '.');
! if (*lconvert->mon_thousands_sep != '\0')
! ssymbol = *lconvert->mon_thousands_sep;
! else
! /* ssymbol should not equal dsymbol */
! ssymbol = (dsymbol != ',') ? ',' : '.';
csymbol = ((*lconvert->currency_symbol != '\0') ? lconvert->currency_symbol : "$");
psymbol = ((*lconvert->positive_sign != '\0') ? *lconvert->positive_sign : '+');
nsymbol = ((*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-");
***************
*** 293,312 ****
if (mon_group <= 0 || mon_group > 6)
mon_group = 3;
- ssymbol = ((*lconvert->mon_thousands_sep != '\0') ? *lconvert->mon_thousands_sep : ',');
convention = lconvert->n_sign_posn;
dsymbol = ((*lconvert->mon_decimal_point != '\0') ? *lconvert->mon_decimal_point : '.');
csymbol = ((*lconvert->currency_symbol != '\0') ? lconvert->currency_symbol : "$");
nsymbol = ((*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-");
point_pos = LAST_DIGIT - points;
! /* allow more than three decimal points and separate them */
! if (ssymbol)
! {
! point_pos -= (points - 1) / mon_group;
! ssymbol_position = point_pos % (mon_group + 1);
! }
/* we work with positive amounts and add the minus sign at the end */
if (value < 0)
--- 297,316 ----
if (mon_group <= 0 || mon_group > 6)
mon_group = 3;
convention = lconvert->n_sign_posn;
dsymbol = ((*lconvert->mon_decimal_point != '\0') ? *lconvert->mon_decimal_point : '.');
+ if (*lconvert->mon_thousands_sep != '\0')
+ ssymbol = *lconvert->mon_thousands_sep;
+ else
+ /* ssymbol should not equal dsymbol */
+ ssymbol = (dsymbol != ',') ? ',' : '.';
csymbol = ((*lconvert->currency_symbol != '\0') ? lconvert->currency_symbol : "$");
nsymbol = ((*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-");
point_pos = LAST_DIGIT - points;
! point_pos -= (points - 1) / mon_group;
! ssymbol_position = point_pos % (mon_group + 1);
/* we work with positive amounts and add the minus sign at the end */
if (value < 0)
***************
*** 333,339 ****
strncpy((buf + count - strlen(csymbol) + 1), csymbol, strlen(csymbol));
count -= strlen(csymbol) - 1;
! if (buf[LAST_DIGIT] == ',')
buf[LAST_DIGIT] = buf[LAST_PAREN];
/* see if we need to signify negative amount */
--- 337,344 ----
strncpy((buf + count - strlen(csymbol) + 1), csymbol, strlen(csymbol));
count -= strlen(csymbol) - 1;
! /* XXX What does this do? It seems to duplicate the last character. */
! if (buf[LAST_DIGIT] == ssymbol)
buf[LAST_DIGIT] = buf[LAST_PAREN];
/* see if we need to signify negative amount */
On Fri, 23 Nov 2007 15:59:29 -0500 (EST)
Bruce Momjian <bruce@momjian.us> wrote:
I also removed a ssymbol test because ssymbol will never be \0 based on
the code. Also, what does this do:if (buf[LAST_DIGIT] == ',')
buf[LAST_DIGIT] = buf[LAST_PAREN];
Good question. This has been in the code since day one. I had to go
back to the book it came from (Software Solutions in C - ISBN
0-12-632360-7) to see what I wrote back in 1994. Here is what the text
says about that.
"The next part may look a little strange, but the previous code is a
little simpler if we just put a comma at the end of the string when
there are no decimal points and strip it off here. Otherwise we would
have to adjust the comma_position variable and test in the main loop
for the special case."
The if block has a second statement though which is effectively this;
buf[LAST_PAREN] = ' ';
This is followed by;
"The Two assignments deal with the case where there is a trailing
parenthesis without having to do another test."
So that's what I know now. I'm not sure what the edge case is. For
one thing it would only be triggered in locales with no places after
the decimal point. I'll try to work up some test later.
--
D'Arcy J.M. Cain <darcy@druid.net> | Democracy is three wolves
http://www.druid.net/darcy/ | and a sheep voting on
+1 416 425 1212 (DoD#0082) (eNTP) | what's for dinner.Patch applied. I will email on this LAST_DIGIT issue separately.
---------------------------------------------------------------------------
Bruce Momjian wrote:
In looking at the use of the thousands separator in formatting.c and
psql/print.c, I now see similar problems with the MONEY type, namely
that if the thousands separator is "" it is set to "," even if the
decimal separator is ",".The attached patch fixes this.
I also removed a ssymbol test because ssymbol will never be \0 based on
the code. Also, what does this do:if (buf[LAST_DIGIT] == ',')
buf[LAST_DIGIT] = buf[LAST_PAREN];--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com+ If your life is a hard drive, Christ can be your backup. +
---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
D'Arcy J.M. Cain wrote:
On Fri, 23 Nov 2007 15:59:29 -0500 (EST)
Bruce Momjian <bruce@momjian.us> wrote:I also removed a ssymbol test because ssymbol will never be \0 based on
the code. Also, what does this do:if (buf[LAST_DIGIT] == ',')
buf[LAST_DIGIT] = buf[LAST_PAREN];Good question. This has been in the code since day one. I had to go
back to the book it came from (Software Solutions in C - ISBN
0-12-632360-7) to see what I wrote back in 1994. Here is what the text
says about that."The next part may look a little strange, but the previous code is a
little simpler if we just put a comma at the end of the string when
there are no decimal points and strip it off here. Otherwise we would
have to adjust the comma_position variable and test in the main loop
for the special case."The if block has a second statement though which is effectively this;
buf[LAST_PAREN] = ' ';
This is followed by;
"The Two assignments deal with the case where there is a trailing
parenthesis without having to do another test."So that's what I know now. I'm not sure what the edge case is. For
one thing it would only be triggered in locales with no places after
the decimal point. I'll try to work up some test later.
Your summary is helpful. I ran some tests with points == 0 and number
of digits % mon_group == 0 and did see the trailing ssymbol before that
code, and it removed after. What they are doing is setting
buf[LAST_PAREN] to the NUL byte above the ssymbol code, and then at this
point they just trim off the ssymbol. (Perhaps the original code put the
parentheses in earlier and had to do this trick to shift in the
parentheses, but we do it after this block.)
What I did was to hard-code the \0 in there and add a comment about what
it is doing --- patch attached and applied.
I am confused about two other items with MONEY. First, why can't
anything but a string be cast to this type?
test=> select 871234872319489323::money;
ERROR: cannot cast type bigint to money
LINE 1: select 871234872319489323::money;
^
test=> select 871234872::money;
ERROR: cannot cast type integer to money
LINE 1: select 871234872::money;
^
test=> select 87123487231.3::money;
ERROR: cannot cast type numeric to money
LINE 1: select 87123487231.3::money;
^
Without such castings, how is this data type used? I did a \dC and saw
no entries for MONEY.
And second, why are there no regression tests for MONEY. I see it used
only once in the rules test.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Attachments:
/rtmp/difftext/x-diffDownload
Index: src/backend/utils/adt/cash.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/cash.c,v
retrieving revision 1.76
diff -c -c -r1.76 cash.c
*** src/backend/utils/adt/cash.c 24 Nov 2007 15:28:02 -0000 1.76
--- src/backend/utils/adt/cash.c 24 Nov 2007 16:13:30 -0000
***************
*** 337,345 ****
strncpy((buf + count - strlen(csymbol) + 1), csymbol, strlen(csymbol));
count -= strlen(csymbol) - 1;
! /* XXX What does this do? It seems to duplicate the last character. */
if (buf[LAST_DIGIT] == ssymbol)
! buf[LAST_DIGIT] = buf[LAST_PAREN];
/* see if we need to signify negative amount */
if (minus)
--- 337,349 ----
strncpy((buf + count - strlen(csymbol) + 1), csymbol, strlen(csymbol));
count -= strlen(csymbol) - 1;
! /*
! * If points == 0 and the number of digits % mon_group == 0,
! * the code above adds a trailing ssymbol on the far right,
! * so remove it.
! */
if (buf[LAST_DIGIT] == ssymbol)
! buf[LAST_DIGIT] = '\0';
/* see if we need to signify negative amount */
if (minus)
On Sat, 24 Nov 2007 11:27:38 -0500 (EST)
Bruce Momjian <bruce@momjian.us> wrote:
I am confused about two other items with MONEY. First, why can't
anything but a string be cast to this type?test=> select 871234872319489323::money;
ERROR: cannot cast type bigint to money
LINE 1: select 871234872319489323::money;
^
test=> select 871234872::money;
ERROR: cannot cast type integer to money
LINE 1: select 871234872::money;
^
test=> select 87123487231.3::money;
ERROR: cannot cast type numeric to money
LINE 1: select 87123487231.3::money;
^
I agree. I wasn't the one that added the meta information.
And second, why are there no regression tests for MONEY. I see it used
only once in the rules test.
I think that scrappy added this into the code before we were so
vigorous about creating regression tests for everything. I agree that
there should be something. I personally unit test all my own code and
I am a big test booster.
I will look at adding something in.
--
D'Arcy J.M. Cain <darcy@druid.net> | Democracy is three wolves
http://www.druid.net/darcy/ | and a sheep voting on
+1 416 425 1212 (DoD#0082) (eNTP) | what's for dinner.On Sat, 24 Nov 2007 11:27:38 -0500 (EST)
Bruce Momjian <bruce@momjian.us> wrote:
And second, why are there no regression tests for MONEY. I see it used
only once in the rules test.
I will be committing some regression tests as soon as I finish testing.
--
D'Arcy J.M. Cain <darcy@druid.net> | Democracy is three wolves
http://www.druid.net/darcy/ | and a sheep voting on
+1 416 425 1212 (DoD#0082) (eNTP) | what's for dinner.D'Arcy J.M. Cain wrote:
On Sat, 24 Nov 2007 11:27:38 -0500 (EST)
Bruce Momjian <bruce@momjian.us> wrote:I am confused about two other items with MONEY. First, why can't
anything but a string be cast to this type?test=> select 871234872319489323::money;
ERROR: cannot cast type bigint to money
LINE 1: select 871234872319489323::money;
^
test=> select 871234872::money;
ERROR: cannot cast type integer to money
LINE 1: select 871234872::money;
^
test=> select 87123487231.3::money;
ERROR: cannot cast type numeric to money
LINE 1: select 87123487231.3::money;
^I agree. I wasn't the one that added the meta information.
OK, so the big question then is if we are un-depricating this data type,
does it have the behavior we want? (And the regression addition will be
helpful too.)
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote:
D'Arcy J.M. Cain wrote:
On Sat, 24 Nov 2007 11:27:38 -0500 (EST)
Bruce Momjian <bruce@momjian.us> wrote:I am confused about two other items with MONEY. First, why can't
anything but a string be cast to this type?test=> select 871234872319489323::money;
ERROR: cannot cast type bigint to money
LINE 1: select 871234872319489323::money;
^
test=> select 871234872::money;
ERROR: cannot cast type integer to money
LINE 1: select 871234872::money;
^
test=> select 87123487231.3::money;
ERROR: cannot cast type numeric to money
LINE 1: select 87123487231.3::money;
^I agree. I wasn't the one that added the meta information.
OK, so the big question then is if we are un-depricating this data type,
does it have the behavior we want? (And the regression addition will be
helpful too.)
Added to TODO list:
* Allow MONEY to be cast to/from other numeric data types
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
Added to TODO list:
* Allow MONEY to be cast to/from other numeric data types
So in other words, that's been added to the TODO list *purely* on your
own say-so, and not because any users asked for it or anyone else thinks
it's a good idea.
Since MONEY really ought to be considered a tagged type, I'm not at all
impressed with the idea that there should be default casts between it
and plain numerics. There's a fundamental semantic gap there.
regards, tom lane
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Mon, 26 Nov 2007 21:19:48 -0500
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Bruce Momjian <bruce@momjian.us> writes:
Added to TODO list:
* Allow MONEY to be cast to/from other numeric data typesSo in other words, that's been added to the TODO list *purely* on your
own say-so, and not because any users asked for it or anyone else
thinks it's a good idea.
Well if we are going to continue to support money (which I am against)
we should support the casting to numeric as that is by far a more
common implementation of money and we will have mixed environments.
Since MONEY really ought to be considered a tagged type, I'm not at
all impressed with the idea that there should be default casts
between it and plain numerics. There's a fundamental semantic gap
there.
Perhaps but the practical gap is much narrower.
Sincerely,
Joshua D. Drake
regards, tom lane
---------------------------(end of
broadcast)--------------------------- TIP 2: Don't 'kill -9' the
postmaster
- --
=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 24x7/Emergency: +1.800.492.2240
PostgreSQL solutions since 1997 http://www.commandprompt.com/
UNIQUE NOT NULL
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate
PostgreSQL Replication: http://www.commandprompt.com/products/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFHS4QhATb/zqfZUUQRAraiAJ9HLeqG7nRbblPvJhu/JQwhOrmzxQCgpWGE
1JShnZ4xwM1+lQzTKCkGVcw=
=sErU
-----END PGP SIGNATURE-----
"Joshua D. Drake" <jd@commandprompt.com> writes:
Well if we are going to continue to support money (which I am against)
we should support the casting to numeric as that is by far a more
common implementation of money and we will have mixed environments.
So, you don't use MONEY, and you don't want to, but nonetheless you
know better than the people who do use MONEY what they need.
Aside from the semantic-gap issue, there is the point that providing
a cast might actually mask application errors. I can well imagine
cases where one of the reasons for using MONEY is *exactly* that it's
not a plain number or easily convertible to one.
regards, tom lane
Tom Lane wrote:
"Joshua D. Drake" <jd@commandprompt.com> writes:
Well if we are going to continue to support money (which I am against)
we should support the casting to numeric as that is by far a more
common implementation of money and we will have mixed environments.So, you don't use MONEY, and you don't want to, but nonetheless you
know better than the people who do use MONEY what they need.
I found out you can cast numerics to MONEY by using two casts:
test=> SELECT 12321.12::text::money;
money
------------
$12,321.12
(1 row)
For some reason this doesn't work in 8.2 but does in 8.3. The reverse
doesn't work:
test=> SELECT '12321.12'::money::text::numeric;
ERROR: invalid input syntax for type numeric: "$12,321.12"
The big problem is that MONEY is a string so the dollar sign and commas
are a problem.
Aside from the semantic-gap issue, there is the point that providing
a cast might actually mask application errors. I can well imagine
cases where one of the reasons for using MONEY is *exactly* that it's
not a plain number or easily convertible to one.
Perhaps all we need is a way to accomplish the casting so it isn't
automatic. This works:
test=> SELECT regexp_replace('2343'::money::text, '[^$,]*', '', 'g')::numeric;
regexp_replace
----------------
2343.00
(1 row)
but the '$' and ',' are locale-specific and I can't think of a way to do
this generically.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Mon, 26 Nov 2007 23:47:04 -0500 (EST)
Bruce Momjian <bruce@momjian.us> wrote:
Perhaps all we need is a way to accomplish the casting so it isn't
automatic. This works:test=> SELECT regexp_replace('2343'::money::text, '[^$,]*',
'', 'g')::numeric; regexp_replace
----------------
2343.00
(1 row)but the '$' and ',' are locale-specific and I can't think of a way to
do this generically.
With a regex?
Joshua D. Drake
- --
=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 24x7/Emergency: +1.800.492.2240
PostgreSQL solutions since 1997 http://www.commandprompt.com/
UNIQUE NOT NULL
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate
PostgreSQL Replication: http://www.commandprompt.com/products/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFHS6RjATb/zqfZUUQRAtZyAJ9VZeCzjX+RSGr3A9eWXTcbHwnc4gCgoZcH
LcO9KnjcycRo5YjCektSJXg=
=HZpl
-----END PGP SIGNATURE-----
Joshua D. Drake wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On Mon, 26 Nov 2007 23:47:04 -0500 (EST)
Bruce Momjian <bruce@momjian.us> wrote:Perhaps all we need is a way to accomplish the casting so it isn't
automatic. This works:test=> SELECT regexp_replace('2343'::money::text, '[^$,]*',
'', 'g')::numeric; regexp_replace
----------------
2343.00
(1 row)but the '$' and ',' are locale-specific and I can't think of a way to
do this generically.With a regex?
The problem is there is no locale-independent way to determine if
'123.456' is ~123k or ~123. I think we are going to need a way to
output the MONEY value without a currency and thousands symbols.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Tom Lane wrote:
"Joshua D. Drake" <jd@commandprompt.com> writes:
Well if we are going to continue to support money (which I am against)
we should support the casting to numeric as that is by far a more
common implementation of money and we will have mixed environments.So, you don't use MONEY, and you don't want to, but nonetheless you
know better than the people who do use MONEY what they need.Aside from the semantic-gap issue, there is the point that providing
a cast might actually mask application errors. I can well imagine
cases where one of the reasons for using MONEY is *exactly* that it's
not a plain number or easily convertible to one.
Right. I am not thinking of an auto-cast but rather give people _some_
way to cast to/from MONEY, which is what the TODO says:
* Allow MONEY to be cast to/from other numeric data types
Even when we have multiple currency designations I would think people
would need a way to cast. I am trying to anticpate how MONEY will be
used. You are right we don't have any field requests yet, but I am
expecting them.
I have added documentation on the issues of casting to/from MONEY;
patch attached and applied.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Attachments:
/rtmp/difftext/x-diffDownload
Index: doc/src/sgml/datatype.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/datatype.sgml,v
retrieving revision 1.217
diff -c -c -r1.217 datatype.sgml
*** doc/src/sgml/datatype.sgml 21 Nov 2007 04:01:37 -0000 1.217
--- doc/src/sgml/datatype.sgml 27 Nov 2007 05:45:13 -0000
***************
*** 842,847 ****
--- 842,855 ----
floating-point literals, as well as <quote>typical</quote>
currency formatting, such as <literal>'$1,000.00'</literal>.
Output is generally in the latter form but depends on the locale.
+ Non-quoted numeric values can be converted to <type>money</type> by
+ casting the numeric value to <type>text</type> and then
+ <type>money</type>:
+ <programlisting>
+ SELECT 1234::text::money;
+ </programlisting>
+ There is no simple way of doing the reverse; casting a <type>money</type> value to a
+ numeric type.
</para>
<para>
Tom Lane wrote:
Aside from the semantic-gap issue, there is the point that providing
a cast might actually mask application errors. �I can well imagine
cases where one of the reasons for using MONEY is *exactly* that it's
not a plain number or easily convertible to one.
I'm always against casts, but I can hardly object to an explicit cast between
money and numeric.
--
Peter Eisentraut
http://developer.postgresql.org/~petere/