Re: Numeric x^y for negative x

On Wed, 21 Jul 2021 11:10:16 +0100
Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:

> On Tue, 20 Jul 2021 at 10:17, Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> wrote:
> >
> > This patch fixes numeric_power() to handle negative bases correctly and not
> > to raise an error "cannot take logarithm of a negative number", as well as a
> > bug that a result whose values is almost zero is incorrectly returend as 1.
> > The previous behaviors are obvious strange, and these fixes seem to me reasonable.
> >
> > Also, improper overflow errors are corrected in numeric_power() and
> > numeric_exp() to return 0 when it is underflow in fact.
> > I think it is no problem that these functions return zero instead of underflow
> > errors because power_var_int() already do the same.
> >
> > The patch includes additional tests for checking negative bases cases and
> > underflow and rounding of the almost zero results. It seems good.
>
> Thanks for the review!
>
>
> > Let me just make one comment.
> >
> > (errcode(ERRCODE_INVALID_ARGUMENT_FOR_POWER_FUNCTION),
> > errmsg("zero raised to a negative power is undefined")));
> >
> > - if (sign1 < 0 && !numeric_is_integral(num2))
> > - ereport(ERROR,
> > - (errcode(ERRCODE_INVALID_ARGUMENT_FOR_POWER_FUNCTION),
> > - errmsg("a negative number raised to a non-integer power yields a complex result")));
> > -
> > /*
> > * Initialize things
> > */
> >
> > I don't think we need to move this check from numeric_power to power_var.
>
> Moving it to power_var() means that it only needs to be checked in the
> case of a negative base, together with an exponent that cannot be
> handled by power_var_int(), which saves unnecessary checking. It isn't
> necessary to do this test at all if the exponent is an integer small
> enough to fit in a 32-bit int. And if it's not an integer, or it's a
> larger integer than that, it seems more logical to do the test in
> power_var() near to the other code handling that case.

Indeed, I agree with that this change saves unnecessary checking.

>
> > I noticed the following comment in a numeric_power().
> >
> > /*
> > * The SQL spec requires that we emit a particular SQLSTATE error code for
> > * certain error conditions. Specifically, we don't return a
> > * divide-by-zero error code for 0 ^ -1.
> > */
> >
> > In the original code, two checks that could raise an error of
> > ERRCODE_INVALID_ARGUMENT_FOR_POWER_FUNCTION are following the comment.
> > I think these check codes are placed together under this comment intentionally,
> > so I suggest not to move one of them.
>
> Ah, that's a good point about the SQL spec. The comment only refers to
> the case of 0 ^ -1, but the SQL spec does indeed say that a negative
> number to a non-integer power should return the same error code.
>
> Here is an updated patch with additional comments about the required
> error code when raising a negative number to a non-integer power, and
> where it is checked.

Thank you for updating the patch. I am fine with the additional comments.
I don't think there is any other problem left, so I marked it Ready-for-Committers.

Regards,
Yugo Nagata

--
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>