Thread: Add numeric_trim(numeric)

Hi,

Here's a patch for the second function suggested in
5643125E.1030605@joh.to.  This is my first patch trying to do anything
with numerics, so please be gentle.  I'm sure it's full of stupid.

January's commit fest, feedback welcome, yada yada..


.m

On 27 December 2015 at 07:11, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> 2015-11-19 3:58 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
>>> Here's a patch for the second function suggested in
>>> 5643125E.1030605@joh.to.
>>>
> So I am sending a review of this patch.
>

I took a quick look at this too.

It seems like a useful function to have, but perhaps it should just be
called trim() rather than numeric_trim(), for consistency with the
names of the other numeric functions, which don't start with
"numeric_".


I found a bug in the dscale calculation:

select numeric_trim(1e100);
ERROR:  value overflows numeric format

The problem is that the trailing zeros are already stripped off, and
so the computation
   dscale = (arg.ndigits - arg.weight - 1) * DEC_DIGITS;

results in a negative dscale, which needs to be guarded against.


Actually I found the implementation a little confusing, partly because
the first comment doesn't really match the line of code that follows
it, and partly because of the complexity of the loop, the macros and
working out all the exit conditions from the loop. A much simpler
implementation would be to first call strip_var(), and then you'd only
need to inspect the final digit (known to be non-zero). In
pseudo-code, it might look a little like the following:
   init_var_from_num()   strip_var()   if ndigits > 0:       dscale = (arg.ndigits - arg.weight - 1) * DEC_DIGITS
ifdscale < 0:           dscale = 0       if dscale > 0:           // Here we know the last digit is non-zero and that
itis to the           // right of the decimal point, so inspect it and adjust dscale           // (reducing it by up to
3)to trim any remaining zero decimal           // digits           dscale -= ...   else:       dscale = 0
 

This then only has to test for non-zero decimal digits in the final
base-NBASE digit, rather than looping over digits from the right. In
addition, it removes the need to do the following at the start:
   /* for simplicity in the loop below, do full NBASE digits at a time */   dscale = ((arg.dscale + DEC_DIGITS - 1) /
DEC_DIGITS)* DEC_DIGITS;
 

which is the comment I found confusing because doesn't really match up
with what that line of code is doing.

Also, it then won't be necessary to do
   /* If it's zero, normalize the sign and weight */   if (ndigits == 0)   {       arg.sign = NUMERIC_POS;
arg.weight= 0;       arg.dscale = 0;   }
 

because strip_var() will have taken care of that.

In fact, in most (all?) cases, the equivalent of strip_var() will have
already been called by whatever produced the input Numeric, so it
won't actually have any work to do, but it will fall through very
quickly in those cases.


One final comment -- in the regression tests the quotes around all the
numbers are unnecessary.

Regards,
Dean

On Wed, Jan 6, 2016 at 10:21 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> It seems like a useful function to have, but perhaps it should just be
> called trim() rather than numeric_trim(), for consistency with the
> names of the other numeric functions, which don't start with
> "numeric_".

That wouldn't work in this case, because we have hard-coded parser
productions for TRIM().

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 6 January 2016 at 20:09, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Jan 6, 2016 at 10:21 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> It seems like a useful function to have, but perhaps it should just be
>> called trim() rather than numeric_trim(), for consistency with the
>> names of the other numeric functions, which don't start with
>> "numeric_".
>
> That wouldn't work in this case, because we have hard-coded parser
> productions for TRIM().
>

Ah. Good point.

Pity -- I would have liked a nice short name for this, in a similar
vein to the existing numeric functions.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On 6 January 2016 at 20:09, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Jan 6, 2016 at 10:21 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>>> It seems like a useful function to have, but perhaps it should just be
>>> called trim() rather than numeric_trim(), for consistency with the
>>> names of the other numeric functions, which don't start with
>>> "numeric_".

>> That wouldn't work in this case, because we have hard-coded parser
>> productions for TRIM().

Does it have to be called TRIM()?  After looking at the spec for it
I'd think rtrim() is the more correct analogy.

Also worth noting is that those hard-wired parser productions aren't
as hard-wired as all that.

regression=# select trim(43.5);
ERROR:  function pg_catalog.btrim(numeric) does not exist

If we wanted to call the function btrim() underneath, this would
Just Work.  However, to alleviate confusion, it might be better
if we altered the grammar productions to output "trim" not "btrim"
for the not-LEADING-or-TRAILING cases, and of course renamed the
relevant string functions to match.

A different approach is that I'm not real sure why we want a function
that returns a modified numeric value at all.  To the extent I understood
Marko's original use case, it seems like what you'd invariably do with the
result is extract its scale().  Why not skip the middleman and define a
function named something like minscale() or leastscale(), which returns an
int that is the smallest scale that would not drop data?  (If you actually
did want the modified numeric value, you could use round(x, minscale(x))
to get it.)
        regards, tom lane

On Wed, Jan 6, 2016 at 6:51 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 6 January 2016 at 20:09, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Jan 6, 2016 at 10:21 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>>> It seems like a useful function to have, but perhaps it should just be
>>> called trim() rather than numeric_trim(), for consistency with the
>>> names of the other numeric functions, which don't start with
>>> "numeric_".
>>
>> That wouldn't work in this case, because we have hard-coded parser
>> productions for TRIM().
>>
>
> Ah. Good point.
>
> Pity -- I would have liked a nice short name for this, in a similar
> vein to the existing numeric functions.

My experiences with function overloading haven't been enormously
positive - things that we think will work out sometimes don't, a la
the whole pg_size_pretty mess.

In this case, trim(stringy-thingy) and trim(numberish-thingy) aren't
even really doing the same thing, which makes me even less excited
about it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 7 January 2016 at 00:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> A different approach is that I'm not real sure why we want a function
> that returns a modified numeric value at all.  To the extent I understood
> Marko's original use case, it seems like what you'd invariably do with the
> result is extract its scale().  Why not skip the middleman and define a
> function named something like minscale() or leastscale(), which returns an
> int that is the smallest scale that would not drop data?  (If you actually
> did want the modified numeric value, you could use round(x, minscale(x))
> to get it.)
>

minscale() sounds good to me.

Re-reading Marko's original use case, it sounds like that specific
example would boil down to a check that minscale(x) <= 2, although
that can be done today using trunc(x,2) = x. Still, it seems that
minscale() would be a useful general purpose function to have.

Regards,
Dean

On 6 January 2016 at 15:21, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> Actually I found the implementation a little confusing, partly because
> the first comment doesn't really match the line of code that follows
> it, and partly because of the complexity of the loop, the macros and
> working out all the exit conditions from the loop. A much simpler
> implementation would be to first call strip_var() ...

Additionally, the central part of the algorithm where it computes dig
% 10, dig % 100 and dig % 1000 inside a bunch of conditional macros is
overly complex and hard to read. Once you've got it down to the case
where you know dig is non-zero, you can just do

while ((dig % 10) == 0)
{   dscale--;   dig /= 10;
}

which works for any NBASE, rather than having to introduce an
NBASE-dependent block of code with all those preprocessor
conditionals.

(Actually, given the other thread of this discussion, I would name
that local variable min_scale now.)

Regards,
Dean

On 2016-01-07 1:11 AM, Tom Lane wrote:
> A different approach is that I'm not real sure why we want a function
> that returns a modified numeric value at all.  To the extent I understood
> Marko's original use case, it seems like what you'd invariably do with the
> result is extract its scale().

Well, no, both are useful.  I think as far as the interface goes, having 
both a scale() and a way to "rtrim" a numeric is optimal.  rtrim() can 
also be used before storing and/or displaying values to get rid of 
unnecessary zeroes.

As for what the actual function should be called, I don't much care.  I 
wanted to avoid making trim() work because I thought it would interfere 
with the  trim('foo')  case where the input argument's type is unknown, 
but after some testing it appears that that would not be interfered with.


.m

Marko Tiikkaja wrote:
> Hi,
> 
> Here's a patch for the second function suggested in 5643125E.1030605@joh.to.

I think this patch got useful feedback but we never saw a followup
version posted.  I closed it as returned-with-feedback.  Feel free to
submit a new version for the 2016-03 commitfest.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jan 27, 2016 at 7:09 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
>> Here's a patch for the second function suggested in 5643125E.1030605@joh.to.
>
> I think this patch got useful feedback but we never saw a followup
> version posted.  I closed it as returned-with-feedback.  Feel free to
> submit a new version for the 2016-03 commitfest.

This was switched back to Waiting on Author despite not having been
updated.  I have switched it back to Returned with Feedback.  It's
very difficult to focus on the patches in a CommitFest that have a
chance of actually being ready to commit when the list is cluttered
with entries that have not even been updated.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company