Re: proposal: minscale, rtrim, btrim functions for numeric - Mailing list pgsql-hackers

Hi Pavel,

Thanks for your changes.  More inline below:

On Sun, 8 Dec 2019 08:38:38 +0100
Pavel Stehule <pavel.stehule@gmail.com> wrote:

> ne 8. 12. 2019 v 2:23 odesílatel Karl O. Pinc <kop@meme.com> napsal:

> > On Mon, 11 Nov 2019 15:47:37 +0100
> > Pavel Stehule <pavel.stehule@gmail.com> wrote:

> > > I implemented two functions - first minscale, second trim_scale.
> > > The overhead of second is minimal - so I think it can be good to
> > > have it. I started design with the name "trim_scale", but the
> > > name can be any other. 


> > I comment on various hunks in line below:

>
> > diff --git a/src/backend/utils/adt/numeric.c
> > b/src/backend/utils/adt/numeric.c index a00db3ce7a..35234aee4c
> > 100644 --- a/src/backend/utils/adt/numeric.c
> > +++ b/src/backend/utils/adt/numeric.c
> >
> > ****
> > I believe the hunks in this file should start at about line# 3181.
> > This is right after numeric_scale().  Seems like all the scale
> > related functions should be together.
> >
> > There's no hard standard but I don't see why lines (comment lines in
> > your case) should be longer than 78 characters without good reason.
> > Please reformat.
> > ****

I don't see any response from you regarding the above two suggestions.


>
> > + */
> > +static int
> > +get_min_scale(NumericVar *var)
> > +{
> > +       int             minscale = 0;
> > +
> > +       if (var->ndigits > 0)
> > +       {
> > +               NumericDigit last_digit;
> > +
> > +               /* maximal size of minscale, can be lower */
> > +               minscale = (var->ndigits - var->weight - 1) *
> >   DEC_DIGITS; +
> > +               /*
> > +                * When there are not digits after decimal point,
> > the previous expression
> >
> > ****
> > s/not/no/
> > ****
> >
> > +                * can be negative. In this case, the minscale must
> > be zero.
> > +                */
> >
> > ****
> > s/can be/is/
> > ****

By the above, I intended the comment be changed (after line wrapping)
to:
               /*
                 * When there are no digits after decimal point,
                 * the previous expression is negative. In this
                 * case the minscale must be zero.
                 */

(Oh yes, on re-reading I think the comma is unnecessary so I removed it too.)



> >
> > +               if (minscale > 0)
> > +               {
> > +                       /* reduce minscale if trailing digits in
> > last numeric digits are zero */

And the above comment should either be wrapped (as requested above)
or eliminated.  I like comments but I'm not sure this one contributes
anything.


> > --- a/src/include/catalog/pg_proc.dat
> > +++ b/src/include/catalog/pg_proc.dat
> > @@ -4288,6 +4288,12 @@
> >    proname => 'width_bucket', prorettype => 'int4',
> >    proargtypes => 'numeric numeric numeric int4',
> >    prosrc => 'width_bucket_numeric' },
> > +{ oid => '3434', descr => 'returns minimal scale of numeric value',
> >
> > ****
> > How about a descr of?:
> >
> >   minimal scale needed to store the supplied value without data loss
> > ****
> > 
>
> done
>
> >
> > +  proname => 'minscale', prorettype => 'int4', proargtypes =>
> >   'numeric',
> > +  prosrc => 'numeric_minscale' },
> > +{ oid => '3435', descr => 'returns numeric value with minimal
> > scale',
> >
> > ****
> > And likewise a descr of?:
> >
> >   numeric with minimal scale needed to represent the given value
> > ****
> >
> > +  proname => 'trim_scale', prorettype => 'numeric', proargtypes =>
> >   'numeric',
> > +  prosrc => 'numeric_trim_scale' },
> > 
>
> done

Thanks for these changes.  Looking at pg_proc.dat there seems to
be an effort made to keep the lines to a maximum of 78 or 80
characters.  This means starting "descr => '..." on new lines
when the description is long.  Please reformat, doing this or,
if you like, something even more clever to keep the lines short.

Looking good.  We're making progress.

Regards,

Karl <kop@meme.com>
Free Software:  "You don't pay back, you pay forward."
                 -- Robert A. Heinlein