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

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

> Hi Pavel,
>
> Thanks for your changes. More inline below:
>
> On Sun, 8 Dec 2019 08:38:38 +0100
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
> > ne 8. 12. 2019 v 2:23 odesílatel Karl O. Pinc <kop(at)meme(dot)com> napsal:
>
> > > On Mon, 11 Nov 2019 15:47:37 +0100
> > > Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(at)meme(dot)com>
> Free Software: "You don't pay back, you pay forward."
> -- Robert A. Heinlein
>