Thread: 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. + 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. + 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.