On Tue, 20 Jul 2021 at 10:17, Yugo NAGATA <nag...@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
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);