Re: [PATCH] Don't truncate integer part in to_char for 'FM99.'

Hi,

This patch fixes an edge case bug in the numeric to_char() function.

When the numeric to_char format used fillmode (FM), didn't contain 0s
and had a trailing dot, the integer part of the number was truncated in
error.

to_char(10, 'FM99.') used to return '1', after this patch it will return '10'

This is known to affect the format() function in the mysqlcompat
pgFoundry project.

Regards,
Marti

Marti Raudsepp <marti(at)juffo(dot)org> writes:
> This patch fixes an edge case bug in the numeric to_char() function.

> When the numeric to_char format used fillmode (FM), didn't contain 0s
> and had a trailing dot, the integer part of the number was truncated in
> error.

> to_char(10, 'FM99.') used to return '1', after this patch it will return '10'

Hmm. I agree that this is a bug, but the proposed fix seems like a bit
of a kluge. Wouldn't it be better to make get_last_relevant_decnum
honor its contract, that is not delete any relevant digits? I'm
thinking instead of this

if (!p)
p = num;

when there is no decimal point it should do something like

if (!p)
return num + strlen(num) - 1;

regards, tom lane

On Wed, Sep 7, 2011 at 21:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Hmm.  I agree that this is a bug, but the proposed fix seems like a bit
> of a kluge. Wouldn't it be better to make get_last_relevant_decnum
> honor its contract, that is not delete any relevant digits?

You're right, it was a kludge.

Here's an improved version. I need to take a NUMProc* argument to do
that right, because that's how it knows how many '0's to keep from the
format.

What do you think?

Regards,
Marti

Marti Raudsepp <marti(at)juffo(dot)org> writes:
> On Wed, Sep 7, 2011 at 21:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Hmm. I agree that this is a bug, but the proposed fix seems like a bit
>> of a kluge. Wouldn't it be better to make get_last_relevant_decnum
>> honor its contract, that is not delete any relevant digits?

> You're right, it was a kludge.

> Here's an improved version. I need to take a NUMProc* argument to do
> that right, because that's how it knows how many '0's to keep from the
> format.

Yeah, after fooling with it myself I saw that it was the interconnection
of the don't-suppress-'0'-format-positions logic with the find-the-last-
nonzero-digit logic that was confusing matters. (formatting.c may not
be the most spaghetti-ish code I've ever worked with, but it's up
there.) However, I don't think that inserting knowledge of the other
consideration into get_last_relevant_decnum is really the way to make
things cleaner. Also, the way yours is set up, I'm dubious that it
does the right thing when the last '0' specifier is to the left of the
decimal point. I'm currently testing this patch:

diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 7efd988362889346af86c642f6ee660a4ae1b3d2..a7000250b0363165bee5697ad72036aad28b830e 100644
*** a/src/backend/utils/adt/formatting.c
--- b/src/backend/utils/adt/formatting.c
*************** NUM_prepare_locale(NUMProc *Np)
*** 3908,3913 ****
--- 3908,3916 ----
/* ----------
* Return pointer of last relevant number after decimal point
* 12.0500 --> last relevant is '5'
+ * 12.0000 --> last relevant is '.'
+ * If there is no decimal point, return NULL (which will result in same
+ * behavior as if FM hadn't been specified).
* ----------
*/
static char *
*************** get_last_relevant_decnum(char *num)
*** 3921,3927 ****
#endif

if (!p)
! p = num;
result = p;

while (*(++p))
--- 3924,3931 ----
#endif

if (!p)
! return NULL;
!
result = p;

while (*(++p))
*************** NUM_processor(FormatNode *node, NUMDesc
*** 4458,4470 ****
{
Np->num_pre = plen;

! if (IS_FILLMODE(Np->Num))
{
! if (IS_DECIMAL(Np->Num))
! Np->last_relevant = get_last_relevant_decnum(
! Np->number +
! ((Np->Num->zero_end - Np->num_pre > 0) ?
! Np->Num->zero_end - Np->num_pre : 0));
}

if (Np->sign_wrote == FALSE && Np->num_pre == 0)
--- 4462,4483 ----
{
Np->num_pre = plen;

! if (IS_FILLMODE(Np->Num) && IS_DECIMAL(Np->Num))
{
! Np->last_relevant = get_last_relevant_decnum(Np->number);
!
! /*
! * If any '0' specifiers are present, make sure we don't strip
! * those digits.
! */
! if (Np->last_relevant && Np->Num->zero_end > Np->num_pre)
! {
! char *last_zero;
!
! last_zero = Np->number + (Np->Num->zero_end - Np->num_pre);
! if (Np->last_relevant < last_zero)
! Np->last_relevant = last_zero;
! }
}

if (Np->sign_wrote == FALSE && Np->num_pre == 0)

regards, tom lane

On Wed, Sep 7, 2011 at 23:48, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Also, the way yours is set up, I'm dubious that it
> does the right thing when the last '0' specifier is to the left of the
> decimal point.

When the last '0' is left of the decimal point, Num->zero_end is set
to 0, so the branch dealing with that is never executed.

> I'm currently testing this patch:

Looks good to me. It's potentially less efficient, but I'm not worried
about that.

Regards,
Marti