po 9. 12. 2019 v 3:51 odesÃlatel Karl O. Pinc <[email protected]> napsal:
> Hi Pavel,
>
> Thanks for your changes. More inline below:
>
> On Sun, 8 Dec 2019 08:38:38 +0100
> Pavel Stehule <[email protected]> wrote:
>
> > ne 8. 12. 2019 v 2:23 odesÃlatel Karl O. Pinc <[email protected]> napsal:
>
> > > On Mon, 11 Nov 2019 15:47:37 +0100
> > > Pavel Stehule <[email protected]> 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 <[email protected]>
> Free Software: "You don't pay back, you pay forward."
> -- Robert A. Heinlein
>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 57a1539506..2359478792 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -918,6 +918,20 @@
<entry><literal>6.0000000000</literal></entry>
</row>
+ <row>
+ <entry>
+ <indexterm>
+ <primary>min_scale</primary>
+ </indexterm>
+ <literal><function>min_scale(<type>numeric</type>)</function></literal>
+ </entry>
+ <entry><type>integer</type></entry>
+ <entry>minimal scale (number of decimal digits in the fractional part) needed
+ to store the supplied value without data loss</entry>
+ <entry><literal>min_scale(8.4100)</literal></entry>
+ <entry><literal>2</literal></entry>
+ </row>
+
<row>
<entry>
<indexterm>
@@ -1041,6 +1055,20 @@
<entry><literal>1.4142135623731</literal></entry>
</row>
+ <row>
+ <entry>
+ <indexterm>
+ <primary>trim_scale</primary>
+ </indexterm>
+ <literal><function>trim_scale(<type>numeric</type>)</function></literal>
+ </entry>
+ <entry><type>numeric</type></entry>
+ <entry>reduce the scale (the number of decimal digits in the fractional part)
+ to the minimum needed to store the supplied value without data loss</entry>
+ <entry><literal>scale(8.4100)</literal></entry>
+ <entry><literal>8.41</literal></entry>
+ </row>
+
<row>
<entry>
<indexterm>
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index a00db3ce7a..0095fb2f88 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -5620,6 +5620,92 @@ int2int4_sum(PG_FUNCTION_ARGS)
PG_RETURN_DATUM(Int64GetDatumFast(transdata->sum));
}
+/*
+ * Calculate minimal scale. The var should be stripped already.
+ */
+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 no digits after decimal point,
+ * the previous expression is negative. In this
+ * case the minscale must be zero.
+ */
+ if (minscale > 0)
+ {
+ /*
+ * Reduce minscale if trailing digits in last numeric
+ * digits are zero.
+ */
+ last_digit = var->digits[var->ndigits - 1];
+
+ while (last_digit % 10 == 0)
+ {
+ minscale--;
+ last_digit /= 10;
+ }
+ }
+ else
+ minscale = 0;
+ }
+
+ return minscale;
+}
+
+/*
+ * Returns minimal scale required to represent supplied value without loss.
+ */
+Datum
+numeric_min_scale(PG_FUNCTION_ARGS)
+{
+ Numeric num = PG_GETARG_NUMERIC(0);
+ NumericVar arg;
+ int minscale;
+
+ if (NUMERIC_IS_NAN(num))
+ PG_RETURN_NULL();
+
+ init_var_from_num(num, &arg);
+ strip_var(&arg);
+
+ minscale = get_min_scale(&arg);
+ free_var(&arg);
+
+ PG_RETURN_INT32(minscale);
+}
+
+/*
+ * Reduce scale of numeric value to represent supplied value without loss.
+ */
+Datum
+numeric_trim_scale(PG_FUNCTION_ARGS)
+{
+ Numeric num = PG_GETARG_NUMERIC(0);
+ Numeric res;
+ NumericVar result;
+
+ if (NUMERIC_IS_NAN(num))
+ PG_RETURN_NUMERIC(make_result(&const_nan));
+
+ init_var_from_num(num, &result);
+ strip_var(&result);
+
+ result.dscale = get_min_scale(&result);
+
+ res = make_result(&result);
+ free_var(&result);
+
+ PG_RETURN_NUMERIC(res);
+}
/* ----------------------------------------------------------------------
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index ac8f64b219..71a9387652 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -4254,6 +4254,14 @@
{ oid => '3281', descr => 'number of decimal digits in the fractional part',
proname => 'scale', prorettype => 'int4', proargtypes => 'numeric',
prosrc => 'numeric_scale' },
+{ oid => '3434',
+ descr => 'minimal scale needed to store the supplied value without data loss',
+ proname => 'min_scale', prorettype => 'int4', proargtypes => 'numeric',
+ prosrc => 'numeric_min_scale' },
+{ oid => '3435',
+ descr => 'numeric with minimal scale needed to represent the given value',
+ proname => 'trim_scale', prorettype => 'numeric', proargtypes => 'numeric',
+ prosrc => 'numeric_trim_scale' },
{ oid => '1740', descr => 'convert int4 to numeric',
proname => 'numeric', prorettype => 'numeric', proargtypes => 'int4',
prosrc => 'int4_numeric' },
diff --git a/src/test/regress/expected/numeric.out b/src/test/regress/expected/numeric.out
index 1cb3c3bfab..91fc90b80d 100644
--- a/src/test/regress/expected/numeric.out
+++ b/src/test/regress/expected/numeric.out
@@ -2078,6 +2078,192 @@ select scale(-13.000000000000000);
15
(1 row)
+--
+-- Tests for min_scale()
+--
+select min_scale(numeric 'NaN') is NULL; -- should be true
+ ?column?
+----------
+ t
+(1 row)
+
+select min_scale(NULL::numeric) is NULL; -- should be true
+ ?column?
+----------
+ t
+(1 row)
+
+select min_scale(0); -- no digits
+ min_scale
+-----------
+ 0
+(1 row)
+
+select min_scale(0.00); -- no digits again
+ min_scale
+-----------
+ 0
+(1 row)
+
+select min_scale(1.0); -- no scale
+ min_scale
+-----------
+ 0
+(1 row)
+
+select min_scale(1.1); -- scale 1
+ min_scale
+-----------
+ 1
+(1 row)
+
+select min_scale(1.12); -- scale 2
+ min_scale
+-----------
+ 2
+(1 row)
+
+select min_scale(1.123); -- scale 3
+ min_scale
+-----------
+ 3
+(1 row)
+
+select min_scale(1.1234); -- scale 4, filled digit
+ min_scale
+-----------
+ 4
+(1 row)
+
+select min_scale(1.12345); -- scale 5, 2 NDIGITS
+ min_scale
+-----------
+ 5
+(1 row)
+
+select min_scale(1.1000); -- 1 pos in NDIGITS
+ min_scale
+-----------
+ 1
+(1 row)
+
+select min_scale(1.1200); -- 2 pos in NDIGITS
+ min_scale
+-----------
+ 2
+(1 row)
+
+select min_scale(1.1230); -- 3 pos in NDIGITS
+ min_scale
+-----------
+ 3
+(1 row)
+
+select min_scale(1.1234); -- all pos in NDIGITS
+ min_scale
+-----------
+ 4
+(1 row)
+
+select min_scale(1.12345000); -- 2 NDIGITS
+ min_scale
+-----------
+ 5
+(1 row)
+
+select min_scale(1.123400000000); -- strip() required/done
+ min_scale
+-----------
+ 4
+(1 row)
+
+select min_scale(12345.123456789012345); -- "big" number
+ min_scale
+-----------
+ 15
+(1 row)
+
+select min_scale(-12345.12345); -- negative number
+ min_scale
+-----------
+ 5
+(1 row)
+
+select min_scale(1e100); -- very big number
+ min_scale
+-----------
+ 0
+(1 row)
+
+select min_scale(1e100::numeric + 0.1); -- big number with scale
+ min_scale
+-----------
+ 1
+(1 row)
+
+--
+-- Tests for trim_scale()
+--
+select trim_scale(numeric 'NaN') = 'NaN':: numeric; -- should be true
+ ?column?
+----------
+ t
+(1 row)
+
+select trim_scale(NULL::numeric) IS NULL; -- should be true
+ ?column?
+----------
+ t
+(1 row)
+
+select trim_scale(1.120);
+ trim_scale
+------------
+ 1.12
+(1 row)
+
+select trim_scale(0);
+ trim_scale
+------------
+ 0
+(1 row)
+
+select trim_scale(0.00);
+ trim_scale
+------------
+ 0
+(1 row)
+
+select trim_scale(1.1234500);
+ trim_scale
+------------
+ 1.12345
+(1 row)
+
+select trim_scale(110123.12475871856128000);
+ trim_scale
+-----------------------
+ 110123.12475871856128
+(1 row)
+
+select trim_scale(-1123.124718561280000000);
+ trim_scale
+-------------------
+ -1123.12471856128
+(1 row)
+
+select trim_scale(-13.00000000000000000000);
+ trim_scale
+------------
+ -13
+(1 row)
+
+select trim_scale(1e100);
+ trim_scale
+-------------------------------------------------------------------------------------------------------
+ 10000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
+(1 row)
+
--
-- Tests for SUM()
--
diff --git a/src/test/regress/sql/numeric.sql b/src/test/regress/sql/numeric.sql
index a939412359..c4fb23df3e 100644
--- a/src/test/regress/sql/numeric.sql
+++ b/src/test/regress/sql/numeric.sql
@@ -1036,6 +1036,46 @@ select scale(110123.12475871856128);
select scale(-1123.12471856128);
select scale(-13.000000000000000);
+--
+-- Tests for min_scale()
+--
+
+select min_scale(numeric 'NaN') is NULL; -- should be true
+select min_scale(NULL::numeric) is NULL; -- should be true
+select min_scale(0); -- no digits
+select min_scale(0.00); -- no digits again
+select min_scale(1.0); -- no scale
+select min_scale(1.1); -- scale 1
+select min_scale(1.12); -- scale 2
+select min_scale(1.123); -- scale 3
+select min_scale(1.1234); -- scale 4, filled digit
+select min_scale(1.12345); -- scale 5, 2 NDIGITS
+select min_scale(1.1000); -- 1 pos in NDIGITS
+select min_scale(1.1200); -- 2 pos in NDIGITS
+select min_scale(1.1230); -- 3 pos in NDIGITS
+select min_scale(1.1234); -- all pos in NDIGITS
+select min_scale(1.12345000); -- 2 NDIGITS
+select min_scale(1.123400000000); -- strip() required/done
+select min_scale(12345.123456789012345); -- "big" number
+select min_scale(-12345.12345); -- negative number
+select min_scale(1e100); -- very big number
+select min_scale(1e100::numeric + 0.1); -- big number with scale
+
+--
+-- Tests for trim_scale()
+--
+
+select trim_scale(numeric 'NaN') = 'NaN':: numeric; -- should be true
+select trim_scale(NULL::numeric) IS NULL; -- should be true
+select trim_scale(1.120);
+select trim_scale(0);
+select trim_scale(0.00);
+select trim_scale(1.1234500);
+select trim_scale(110123.12475871856128000);
+select trim_scale(-1123.124718561280000000);
+select trim_scale(-13.00000000000000000000);
+select trim_scale(1e100);
+
--
-- Tests for SUM()
--
