WIP: to_char, support for EEEE format

On Sat, Apr 11, 2009 at 2:16 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I was surprised so PostgreSQL doesn't support this basic output format.

Hi Pavel,

I had a look through your patch and would like to suggest improvements
to the new error messages you've introduced.

1. "invalid using of format EEEE"

This message occurs several times in the patch. For one thing, the
grammar is wrong; it should be "use", not "using".

Additionally, this message on its own is not very helpful. If I was
trying to use to_char and got "invalid use of format" my first thought
would be "Invalid how?" The message should at minimum have a DETAIL,
and possibly a HINT as well to make it effective.

2. "cannot use EEEE and others"

The wording on this message is a bit awkward. I think what you meant
to say is that EEEE cannot be used with certain other formatting
codes, but this should be made explicit in the message.

Cheers,
BJ

Hi,

I know very well, so all texts in my patches should be translated to
English. My language skills are really minimal.

So, please, if you can, propose these error messages (with hints)-
result will be much better.

Thank you
Pavel

2009/4/10 Brendan Jurd <direvus@gmail.com>:

On Sat, Apr 11, 2009 at 3:51 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

So, please, if you can, propose these error messages (with hints)-
result will be much better.

Hi Pavel,

I was doing some work on rewording these error messages, and I noticed
that the following code segment occurs identically in four different
locations:

numstr = orgnum = (char *) palloc(MAXDOUBLEWIDTH + 1);
if (Num.pre != 1)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid using of format EEEE")));

Rather than rewording all four copies of the message, I wonder if this
test might be better factored out into a separate function?

Cheers,
BJ

2009/4/21 Brendan Jurd <direvus@gmail.com>:

On Sat, Apr 11, 2009 at 3:51 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

So, please, if you can, propose these error messages (with hints)-
result will be much better.

Hi Pavel,

I was doing some work on rewording these error messages, and I noticed
that the following code segment occurs identically in four different
locations:

               numstr = orgnum = (char *) palloc(MAXDOUBLEWIDTH + 1);
               if (Num.pre != 1)
                       ereport(ERROR,
                                       (errcode(ERRCODE_SYNTAX_ERROR),
                                        errmsg("invalid using of format EEEE")));

Rather than rewording all four copies of the message, I wonder if this
test might be better factored out into a separate function?

maybe macro is better - it is too short and without any semantic for
function, but maybe not. The length of source code is not problem -
the short function will be inlined, so total length will be same. What
should be name for this function or for this macro? It hasn't any
semantic. There should be readable macro only for ereport function -
some

#define RAISE_EEEE_FORMAT_ERROR ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR), errmsg("invalid using of format
EEEE")));

regards
Pavel Stehule

On Wed, Apr 22, 2009 at 10:13 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2009/4/21 Brendan Jurd <direvus@gmail.com>:

               numstr = orgnum = (char *) palloc(MAXDOUBLEWIDTH + 1);
               if (Num.pre != 1)
                       ereport(ERROR,
                                       (errcode(ERRCODE_SYNTAX_ERROR),
                                        errmsg("invalid using of format EEEE")));

Rather than rewording all four copies of the message, I wonder if this
test might be better factored out into a separate function?

maybe macro is better - it is too short and without any semantic for
function, but maybe not. The length of source code is not problem -
the short function will be inlined, so total length will be same. What
should be name for this function or for this macro? It hasn't any
semantic. There should be readable macro only for ereport function -
some

I was thinking of factoring out the *test*, not just the error message.

If I've been reading this code correctly, the purpose of

if (Num.pre != 1)

is to make sure that the numeric format has been given with one digit
before the decimal place (so 9.99EEEE would be acceptable but
99.999EEEE would cause the ERROR).

Because this check is made from various places in the code, it makes
sense to me that it should be a function. Duplicated code makes me
itchy. Perhaps called something like
sci_notation_check_format(NUMDesc *).

Cheers,
BJ

On Sat, Apr 11, 2009 at 3:51 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

So, please, if you can, propose these error messages (with hints)-
result will be much better.

Hi Pavel, hackers.

I've done some work updating Pavel's sci notation patch for to_char().
I've attached a patch against HEAD (EEEE_2.diff.bz2) and against
Pavel's patch as originally submitted to the list
(EEEE_1-to-2.diff.bz2).

A summary of my changes:

* Improve the wording of error messages, and add some detail/hint parts.
* Update the documentation.
* Move the duplicated "one digit before decimal point" test into a macro.
* Fix a bug in the int8_to_char() implementation (was using the wrong
variable).

Hope you find this useful.

Cheers,
BJ

Hi Brendan

this looks well, so please, add it to commitfest page

regards
Pavel Stehule

2009/4/26 Brendan Jurd <direvus@gmail.com>:

2009/4/26 Brendan Jurd <direvus@gmail.com>:

I've done some work updating Pavel's sci notation patch for to_char().

That patch again, now with a couple of minor tweaks to make it apply
cleanly against the current HEAD.

Cheers,
BJ

Brendan Jurd escreveu:

2009/4/26 Brendan Jurd <direvus@gmail.com>:

I've done some work updating Pavel's sci notation patch for to_char().

That patch again, now with a couple of minor tweaks to make it apply
cleanly against the current HEAD.

Here is my review. The patch applied without problems. The docs and regression
tests are included. Both of them worked as expected. Also, you included a fix
in RN format, do it in another patch.

The behavior is not the same as Oracle. Oracle accepts an invalid scientific
notation '999.9EEEE'. Will we support it too? I think so.

euler=# SELECT to_char(1234.56789, '999.9EEEE');
ERRO: invalid format for scientific notation
DETALHE: "EEEE" requires exactly one digit before the decimal point.
DICA: For example, "9.999EEEE" is a valid format.

TO_CHAR(1234.56789,'999.9EEEE')
-------------------------------
1.2E+03

1 rows selected

The '9.999eeee' format error message is misleading.

euler=# select to_char(123, '9.999eeee');
ERRO: cannot use "EEEE" twice

You could include an example in manual too. You could add the two failing
cases above in regression tests too.

--
Euler Taveira de Oliveira
http://www.timbira.com/

2009/7/24 Euler Taveira de Oliveira <euler@timbira.com>:

Here is my review. The patch applied without problems. The docs and regression
tests are included. Both of them worked as expected. Also, you included a fix
in RN format, do it in another patch.

Well, I updated an error message for RN to keep it consistent with the
change I made to the nearby EEEE error message.

Neither RN or EEEE is supported for input, and the error messages were
vague on this point (they just said "not supported").

I understand that separate improvements should be submitted as
separate patches, but this is really part of the one improvement.
Implementing EEEE required improving the error messages, and
consistency required that we improve the RN error message also.

The behavior is not the same as Oracle. Oracle accepts an invalid scientific
notation '999.9EEEE'. Will we support it too? I think so.

euler=# SELECT to_char(1234.56789, '999.9EEEE');
ERRO:  invalid format for scientific notation
DETALHE:  "EEEE" requires exactly one digit before the decimal point.
DICA:  For example, "9.999EEEE" is a valid format.

TO_CHAR(1234.56789,'999.9EEEE')
-------------------------------
 1.2E+03

*shakes fist at Oracle* yes, I suppose we had better follow suit.

The '9.999eeee' format error message is misleading.

euler=# select to_char(123, '9.999eeee');
ERRO:  cannot use "EEEE" twice

Ah, thanks for picking up on this. This was a bug in the original
patch. Looks like we forgot to update the formatting keyword for
lowercase "e".

You could include an example in manual too. You could add the two failing
cases above in regression tests too.

I had already added an example to the manual.

Please find attached version 4 of the patch, and incremental diff from
version 3. It fixes the "eeee" bug ("eeee" is now accepted as a valid
form of "EEEE"), and lifts the restriction on only having one digit
before the decimal point.

Cheers,
BJ

On Sat, Jul 25, 2009 at 2:08 AM, Brendan Jurd<direvus@gmail.com> wrote:

2009/7/24 Euler Taveira de Oliveira <euler@timbira.com>:

Here is my review. The patch applied without problems. The docs and regression
tests are included. Both of them worked as expected. Also, you included a fix
in RN format, do it in another patch.

Well, I updated an error message for RN to keep it consistent with the
change I made to the nearby EEEE error message.

Neither RN or EEEE is supported for input, and the error messages were
vague on this point (they just said "not supported").

I understand that separate improvements should be submitted as
separate patches, but this is really part of the one improvement.
Implementing EEEE required improving the error messages, and
consistency required that we improve the RN error message also.

The behavior is not the same as Oracle. Oracle accepts an invalid scientific
notation '999.9EEEE'. Will we support it too? I think so.

euler=# SELECT to_char(1234.56789, '999.9EEEE');
ERRO:  invalid format for scientific notation
DETALHE:  "EEEE" requires exactly one digit before the decimal point.
DICA:  For example, "9.999EEEE" is a valid format.

TO_CHAR(1234.56789,'999.9EEEE')
-------------------------------
 1.2E+03

*shakes fist at Oracle* yes, I suppose we had better follow suit.

The '9.999eeee' format error message is misleading.

euler=# select to_char(123, '9.999eeee');
ERRO:  cannot use "EEEE" twice

Ah, thanks for picking up on this.  This was a bug in the original
patch.  Looks like we forgot to update the formatting keyword for
lowercase "e".

You could include an example in manual too. You could add the two failing
cases above in regression tests too.

I had already added an example to the manual.

Please find attached version 4 of the patch, and incremental diff from
version 3.  It fixes the "eeee" bug ("eeee" is now accepted as a valid
form of "EEEE"), and lifts the restriction on only having one digit
before the decimal point.

Euler,

Are you going to review this again? We need to know if it is Ready
for Committer.

...Robert

Brendan Jurd escreveu:

Please find attached version 4 of the patch, and incremental diff from
version 3. It fixes the "eeee" bug ("eeee" is now accepted as a valid
form of "EEEE"), and lifts the restriction on only having one digit
before the decimal point.

Looks better but I did some tests and caught some strange behaviors.

SQL> SELECT to_char(1234.56789, '8.999EEEE') FROM DUAL;
SELECT to_char(1234.56789, '8.999EEEE') FROM DUAL

ERROR at line 1:
ORA-01481: invalid number format model

SQL> SELECT to_char(1234.56789, '9.080EEEE') FROM DUAL;
SELECT to_char(1234.56789, '9.080EEEE') FROM DUAL

ERROR at line 1:
ORA-01481: invalid number format model

This is not a problem with your patch but something that needs to be fixed in
PostgreSQL to match Oracle behavior. The following example should emit an
error. IMHO, filling the string with # is very strange. TODO?

euler=# SELECT to_char(1234.56789, '9.080');
to_char
---------
#.#8#
(1 row)

Couldn't the following code be put inside switch clause? If not, you should
add a comment why the validation is outside switch.

+   if (IS_EEEE(num) && n->key->id != NUM_E)
+   {
+       NUM_cache_remove(last_NUMCacheEntry);
+       ereport(ERROR,
+               (errcode(ERRCODE_SYNTAX_ERROR),
+                errmsg("\"EEEE\" must be the last pattern used")));
+   }
+
    switch (n->key->id)
    {
        case NUM_9:

Oracle has a diferent overflow limit [1]http://download.oracle.com/docs/cd/B19306_01/server.102/b14200/sql_elements001.htm#sthref80 but I think we could stay with the
PostgreSQL one. But the #.#### is not the intended behavior. IIRC you're
limited to 99 exponent.

SQL> SELECT to_char(1.234567E+308, '9.999EEEE');
SELECT to_char(1.234567E+308, '9.999EEEE')

ERROR at line 1:
ORA-01426: numeric overflow

euler=# SELECT to_char(1.234567E+308, '9.999EEEE');
to_char
-----------
#.#######
(1 row)

The problem is in numeric_to_char() and float8_to_char(). You could fix it
with the following code. Besides that I think you should comment why '5' or
'6' in the other *_to_char() functions.

+	/* 6 means '.' (decimal point), 'E', '+', and 3 exponent digits */
+       if (isnan(value) || is_infinite(value) ||
+			len > Num.pre + Num.post + 6)
+       {
+           numstr = (char *) palloc(Num.pre + Num.post + 7);
+           fill_str(numstr, '#', Num.pre + Num.post + 6);
+           *(numstr + Num.pre) = '.';
+       }

I can't see more problems in your patch. When you fix it, it'll be ready for a
committer.

[1]: http://download.oracle.com/docs/cd/B19306_01/server.102/b14200/sql_elements001.htm#sthref80
http://download.oracle.com/docs/cd/B19306_01/server.102/b14200/sql_elements001.htm#sthref80

--
Euler Taveira de Oliveira
http://www.timbira.com/

2009/7/29 Euler Taveira de Oliveira <euler@timbira.com>:

Looks better but I did some tests and caught some strange behaviors.

Pavel,

Any comment on these cases? When I looked at the patch I wasn't
really sure why it filled the string with '#' either, but I didn't
question it.

Cheers,
BJ

2009/7/29 Brendan Jurd <direvus@gmail.com>:

2009/7/29 Euler Taveira de Oliveira <euler@timbira.com>:

Looks better but I did some tests and caught some strange behaviors.

Pavel,

Any comment on these cases?  When I looked at the patch I wasn't
really sure why it filled the string with '#' either, but I didn't
question it.

This is consistent with postgres.

Pavel

2009/7/29 Euler Taveira de Oliveira <euler@timbira.com>:

This is not a problem with your patch but something that needs to be fixed in
PostgreSQL to match Oracle behavior. The following example should emit an
error. IMHO, filling the string with # is very strange. TODO?

euler=# SELECT to_char(1234.56789, '9.080');
 to_char
---------
 #.#8#
(1 row)

The formatting functions have a lot of weird corner cases. I've been
trying to clean up some of the more bizarre behaviours in the
date/time formatting functions, but haven't touched the numeric
formatting because I haven't ever needed to use it.

Filling unused characters in the string with "#" may be strange, but
changing it would require a much broader patch that covers all of the
numeric formatting styles, not just EEEE. A TODO is probably the way
to go.

Couldn't the following code be put inside switch clause? If not, you should
add a comment why the validation is outside switch.

+   if (IS_EEEE(num) && n->key->id != NUM_E)
+   {
+       NUM_cache_remove(last_NUMCacheEntry);
+       ereport(ERROR,
+               (errcode(ERRCODE_SYNTAX_ERROR),
+                errmsg("\"EEEE\" must be the last pattern used")));
+   }
+
   switch (n->key->id)
   {
       case NUM_9:

The switch is on (n->key->id), but the test you mentioned above is
looking for any keywords *other than* the EEEE keyword, where EEEE has
previously been parsed.

So if you put the test inside the switch, it would need to appear in
every single branch of the switch except for the NUM_E one. I'm
confused about why you think this needs a comment. Perhaps I
misunderstood you?

Oracle has a diferent overflow limit [1] but I think we could stay with the
PostgreSQL one. But the #.#### is not the intended behavior. IIRC you're
limited to 99 exponent.

SQL> SELECT to_char(1.234567E+308, '9.999EEEE');
SELECT to_char(1.234567E+308, '9.999EEEE')

ERROR at line 1:
ORA-01426: numeric overflow

euler=# SELECT to_char(1.234567E+308, '9.999EEEE');
 to_char
-----------
 #.#######
(1 row)

I don't see any problem with extending this to allow up to 3 exponent
digits ... Pavel, any comment?

The problem is in numeric_to_char() and float8_to_char(). You could fix it
with the following code. Besides that I think you should comment why '5' or
'6' in the other *_to_char() functions.

+       /* 6 means '.' (decimal point), 'E', '+', and 3 exponent digits */

Agreed about the comment; I'll add it.

Cheers,
BJ

2009/7/29 Brendan Jurd <direvus@gmail.com>:

2009/7/29 Euler Taveira de Oliveira <euler@timbira.com>:

This is not a problem with your patch but something that needs to be fixed in
PostgreSQL to match Oracle behavior. The following example should emit an
error. IMHO, filling the string with # is very strange. TODO?

euler=# SELECT to_char(1234.56789, '9.080');
 to_char
---------
 #.#8#
(1 row)

The formatting functions have a lot of weird corner cases.  I've been
trying to clean up some of the more bizarre behaviours in the
date/time formatting functions, but haven't touched the numeric
formatting because I haven't ever needed to use it.

Filling unused characters in the string with "#" may be strange, but
changing it would require a much broader patch that covers all of the
numeric formatting styles, not just EEEE.  A TODO is probably the way
to go.

Couldn't the following code be put inside switch clause? If not, you should
add a comment why the validation is outside switch.

+   if (IS_EEEE(num) && n->key->id != NUM_E)
+   {
+       NUM_cache_remove(last_NUMCacheEntry);
+       ereport(ERROR,
+               (errcode(ERRCODE_SYNTAX_ERROR),
+                errmsg("\"EEEE\" must be the last pattern used")));
+   }
+
   switch (n->key->id)
   {
       case NUM_9:

The switch is on (n->key->id), but the test you mentioned above is
looking for any keywords *other than* the EEEE keyword, where EEEE has
previously been parsed.

So if you put the test inside the switch, it would need to appear in
every single branch of the switch except for the NUM_E one.  I'm
confused about why you think this needs a comment.  Perhaps I
misunderstood you?

Oracle has a diferent overflow limit [1] but I think we could stay with the
PostgreSQL one. But the #.#### is not the intended behavior. IIRC you're
limited to 99 exponent.

SQL> SELECT to_char(1.234567E+308, '9.999EEEE');
SELECT to_char(1.234567E+308, '9.999EEEE')

ERROR at line 1:
ORA-01426: numeric overflow

euler=# SELECT to_char(1.234567E+308, '9.999EEEE');
 to_char
-----------
 #.#######
(1 row)

I don't see any problem with extending this to allow up to 3 exponent
digits ... Pavel, any comment?

I am not sure - this function should be used in reports witl fixed
line's width. And I am thinking, so it's should be problem - I prefer
showing some #.### chars. It's clean signal, so some is wrong, but it
doesn't break generating long run reports (like exception in Oracle)
and doesn't broke formating like 3 exponent digits.

Pavel

Brendan Jurd escreveu:

Filling unused characters in the string with "#" may be strange, but
changing it would require a much broader patch that covers all of the
numeric formatting styles, not just EEEE. A TODO is probably the way
to go.

That was my suggestion: a TODO.

So if you put the test inside the switch, it would need to appear in
every single branch of the switch except for the NUM_E one. I'm
confused about why you think this needs a comment. Perhaps I
misunderstood you?

Yes, I know you need to modify every 'case' clause to test if EEEE was
previously used (that was one suggestion) but I said if you don't want to go
that way, add a comment explaining why you're using that 'if' above the
'switch' and not inside it.

--
Euler Taveira de Oliveira
http://www.timbira.com/

2009/7/30 Pavel Stehule <pavel.stehule@gmail.com>:

2009/7/29 Brendan Jurd <direvus@gmail.com>:

I don't see any problem with extending this to allow up to 3 exponent
digits ... Pavel, any comment?

I am not sure - this function should be used in reports witl fixed
line's width. And I am thinking, so it's should be problem - I prefer
showing some #.### chars. It's clean signal, so some is wrong, but it
doesn't break generating long run reports (like exception in Oracle)
and doesn't broke formating like 3 exponent digits.

Hmm. For what it's worth, I think Pavel makes a good point about the
number of exponent digits -- a large chunk of the use case for numeric
formatting would be fixed-width reporting.

Limiting to two exponent digits also has the nice property that the
output always matches the length of the format pattern:

9.99EEEE
1.23E+02

I don't know whether being able to represent 3-digit exponents
outweighs the value of reliable fixed-width output. Would anyone else
care to throw in their opinion? However we end up handling it, we
will probably need to flesh out the docs regarding this.

Cheers,
BJ

2009/7/30 Euler Taveira de Oliveira <euler@timbira.com>:

So if you put the test inside the switch, it would need to appear in
every single branch of the switch except for the NUM_E one.  I'm
confused about why you think this needs a comment.  Perhaps I
misunderstood you?

Yes, I know you need to modify every 'case' clause to test if EEEE was
previously used (that was one suggestion) but I said if you don't want to go
that way, add a comment explaining why you're using that 'if' above the
'switch' and not inside it.

I think we've pretty much reached an impasse on this one.

I can't imagine anyone reading the code getting confused about this,
and don't know how to go about writing a comment explaining something
that is intuitively obvious to me. I don't understand what aspect of
it requires explanation. The test is not in the switch because it
doesn't belong there.

Perhaps someone else could weigh in and help us to resolve this?

Cheers,
BJ