Re: proposal: minscale, rtrim, btrim functions for numeric

po 9. 12. 2019 v 3:51 odesílatel Karl O. Pinc <kop@meme.com> napsal:

> 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.
>

I fixed almost all mentioned issues (that I understand)

I am sending updated patch

Regards

Pavel

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