On Tue, 20 Jul 2021 at 10:17, Yugo NAGATA <nagata@sraoss.co.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.
> 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.
Regards,
Dean
