On Tue, 20 Jul 2021 at 10:17, Yugo NAGATA <[email protected]> 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
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
new file mode 100644
index 2a0f68f..8a8822c
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -3935,7 +3935,9 @@ numeric_power(PG_FUNCTION_ARGS)
/*
* 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.
+ * divide-by-zero error code for 0 ^ -1. Raising a negative number to a
+ * non-integer power must produce the same error code, but that case is
+ * handled in power_var().
*/
sign1 = numeric_sign_internal(num1);
sign2 = numeric_sign_internal(num2);
@@ -3945,11 +3947,6 @@ numeric_power(PG_FUNCTION_ARGS)
(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
*/
@@ -9762,12 +9759,18 @@ exp_var(const NumericVar *arg, NumericVa
*/
val = numericvar_to_double_no_overflow(&x);
- /* Guard against overflow */
+ /* Guard against overflow/underflow */
/* If you change this limit, see also power_var()'s limit */
if (Abs(val) >= NUMERIC_MAX_RESULT_SCALE * 3)
- ereport(ERROR,
- (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
- errmsg("value overflows numeric format")));
+ {
+ if (val > 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("value overflows numeric format")));
+ zero_var(result);
+ result->dscale = rscale;
+ return;
+ }
/* decimal weight = log10(e^x) = x * log10(e) */
dweight = (int) (val * 0.434294481903252);
@@ -10125,6 +10128,8 @@ log_var(const NumericVar *base, const Nu
static void
power_var(const NumericVar *base, const NumericVar *exp, NumericVar *result)
{
+ int res_sign;
+ NumericVar abs_base;
NumericVar ln_base;
NumericVar ln_num;
int ln_dweight;
@@ -10168,9 +10173,40 @@ power_var(const NumericVar *base, const
return;
}
+ init_var(&abs_base);
init_var(&ln_base);
init_var(&ln_num);
+ /*
+ * If base is negative, insist that exp be an integer. The result is then
+ * positive if exp is even and negative if exp is odd.
+ */
+ if (base->sign == NUMERIC_NEG)
+ {
+ /*
+ * Check that exp is an integer. This error code is defined by the
+ * SQL standard, and matches other errors in numeric_power().
+ */
+ if (exp->ndigits > 0 && exp->ndigits > exp->weight + 1)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_ARGUMENT_FOR_POWER_FUNCTION),
+ errmsg("a negative number raised to a non-integer power yields a complex result")));
+
+ /* Test if exp is odd or even */
+ if (exp->ndigits > 0 && exp->ndigits == exp->weight + 1 &&
+ (exp->digits[exp->ndigits - 1] & 1))
+ res_sign = NUMERIC_NEG;
+ else
+ res_sign = NUMERIC_POS;
+
+ /* Then work with abs(base) below */
+ set_var_from_var(base, &abs_base);
+ abs_base.sign = NUMERIC_POS;
+ base = &abs_base;
+ }
+ else
+ res_sign = NUMERIC_POS;
+
/*----------
* Decide on the scale for the ln() calculation. For this we need an
* estimate of the weight of the result, which we obtain by doing an
@@ -10201,25 +10237,31 @@ power_var(const NumericVar *base, const
val = numericvar_to_double_no_overflow(&ln_num);
- /* initial overflow test with fuzz factor */
+ /* initial overflow/underflow test with fuzz factor */
if (Abs(val) > NUMERIC_MAX_RESULT_SCALE * 3.01)
- ereport(ERROR,
- (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
- errmsg("value overflows numeric format")));
+ {
+ if (val > 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("value overflows numeric format")));
+ zero_var(result);
+ result->dscale = NUMERIC_MAX_DISPLAY_SCALE;
+ return;
+ }
val *= 0.434294481903252; /* approximate decimal result weight */
- /* choose the result scale */
+ /* choose the result scale and the scale for the real calculation */
rscale = NUMERIC_MIN_SIG_DIGITS - (int) val;
rscale = Max(rscale, base->dscale);
rscale = Max(rscale, exp->dscale);
rscale = Max(rscale, NUMERIC_MIN_DISPLAY_SCALE);
- rscale = Min(rscale, NUMERIC_MAX_DISPLAY_SCALE);
- /* set the scale for the real exp * ln(base) calculation */
local_rscale = rscale + (int) val - ln_dweight + 8;
local_rscale = Max(local_rscale, NUMERIC_MIN_DISPLAY_SCALE);
+ rscale = Min(rscale, NUMERIC_MAX_DISPLAY_SCALE);
+
/* and do the real calculation */
ln_var(base, &ln_base, local_rscale);
@@ -10228,8 +10270,12 @@ power_var(const NumericVar *base, const
exp_var(&ln_num, result, rscale);
+ if (res_sign == NUMERIC_NEG && result->ndigits > 0)
+ result->sign = NUMERIC_NEG;
+
free_var(&ln_num);
free_var(&ln_base);
+ free_var(&abs_base);
}
/*
diff --git a/src/test/regress/expected/numeric.out b/src/test/regress/expected/numeric.out
new file mode 100644
index 385e963..659483f
--- a/src/test/regress/expected/numeric.out
+++ b/src/test/regress/expected/numeric.out
@@ -2333,6 +2333,12 @@ select 1.000000000123 ^ (-2147483648);
0.7678656556403084
(1 row)
+select 0.9999999999 ^ 23300000000000 = 0 as rounds_to_zero;
+ rounds_to_zero
+----------------
+ t
+(1 row)
+
-- cases that used to error out
select 0.12 ^ (-25);
?column?
@@ -2346,6 +2352,43 @@ select 0.5678 ^ (-85);
782333637740774446257.7719390061997396
(1 row)
+select 0.9999999999 ^ 70000000000000 = 0 as underflows;
+ underflows
+------------
+ t
+(1 row)
+
+-- negative base to integer powers
+select (-1.0) ^ 2147483646;
+ ?column?
+--------------------
+ 1.0000000000000000
+(1 row)
+
+select (-1.0) ^ 2147483647;
+ ?column?
+---------------------
+ -1.0000000000000000
+(1 row)
+
+select (-1.0) ^ 2147483648;
+ ?column?
+--------------------
+ 1.0000000000000000
+(1 row)
+
+select (-1.0) ^ 1000000000000000;
+ ?column?
+--------------------
+ 1.0000000000000000
+(1 row)
+
+select (-1.0) ^ 1000000000000001;
+ ?column?
+---------------------
+ -1.0000000000000000
+(1 row)
+
--
-- Tests for raising to non-integer powers
--
@@ -2482,6 +2525,18 @@ select exp('-inf'::numeric);
0
(1 row)
+select exp(-5000::numeric) = 0 as rounds_to_zero;
+ rounds_to_zero
+----------------
+ t
+(1 row)
+
+select exp(-10000::numeric) = 0 as underflows;
+ underflows
+------------
+ t
+(1 row)
+
-- cases that used to generate inaccurate results
select exp(32.999);
exp
diff --git a/src/test/regress/sql/numeric.sql b/src/test/regress/sql/numeric.sql
new file mode 100644
index 7e17c28..a930fae
--- a/src/test/regress/sql/numeric.sql
+++ b/src/test/regress/sql/numeric.sql
@@ -1092,10 +1092,19 @@ select 3.789 ^ 35;
select 1.2 ^ 345;
select 0.12 ^ (-20);
select 1.000000000123 ^ (-2147483648);
+select 0.9999999999 ^ 23300000000000 = 0 as rounds_to_zero;
-- cases that used to error out
select 0.12 ^ (-25);
select 0.5678 ^ (-85);
+select 0.9999999999 ^ 70000000000000 = 0 as underflows;
+
+-- negative base to integer powers
+select (-1.0) ^ 2147483646;
+select (-1.0) ^ 2147483647;
+select (-1.0) ^ 2147483648;
+select (-1.0) ^ 1000000000000000;
+select (-1.0) ^ 1000000000000001;
--
-- Tests for raising to non-integer powers
@@ -1138,6 +1147,8 @@ select exp(1.0::numeric(71,70));
select exp('nan'::numeric);
select exp('inf'::numeric);
select exp('-inf'::numeric);
+select exp(-5000::numeric) = 0 as rounds_to_zero;
+select exp(-10000::numeric) = 0 as underflows;
-- cases that used to generate inaccurate results
select exp(32.999);
