Thread: Incorrect results from numeric round() and trunc()
The numeric round() and trunc() functions clamp the scale argument to the range between +/- NUMERIC_MAX_RESULT_SCALE, which is +/- 2000. That's a long way short of the actual allowed range of type numeric, so they produce incorrect results when rounding/truncating more than 2000 digits before or after the decimal point. For example, round(1e-5000, 5000) returns 0 instead of 1e-5000. Attached is a patch fixing that, using the actual documented range of type numeric. I've also tidied up a bit by replacing all instances of SHRT_MAX with a new constant NUMERIC_WEIGHT_MAX, whose name more accurately describes the limit, as used in various other overflow checks. In doing so, I also noticed a comment in power_var() which claimed that ln_dweight could be as low as -SHRT_MAX (-32767), which is wrong. It can only be as small as -NUMERIC_DSCALE_MAX (-16383), though that doesn't affect the point being made in that comment. I'd like to treat this as a bug-fix and back-patch it, since the current behaviour is clearly broken. Regards, Dean
Attachment
On Sun, Jul 7, 2024, at 13:28, Dean Rasheed wrote: > The numeric round() and trunc() functions clamp the scale argument to > the range between +/- NUMERIC_MAX_RESULT_SCALE, which is +/- 2000. > That's a long way short of the actual allowed range of type numeric, > so they produce incorrect results when rounding/truncating more than > 2000 digits before or after the decimal point. For example, > round(1e-5000, 5000) returns 0 instead of 1e-5000. > > Attached is a patch fixing that, using the actual documented range of > type numeric. > > I've also tidied up a bit by replacing all instances of SHRT_MAX with > a new constant NUMERIC_WEIGHT_MAX, whose name more accurately > describes the limit, as used in various other overflow checks. > > In doing so, I also noticed a comment in power_var() which claimed > that ln_dweight could be as low as -SHRT_MAX (-32767), which is wrong. > It can only be as small as -NUMERIC_DSCALE_MAX (-16383), though that > doesn't affect the point being made in that comment. > > I'd like to treat this as a bug-fix and back-patch it, since the > current behaviour is clearly broken. Fix seems straightforward to me. I agree it should be back-patched. Regards, Joel
On Sun, Jul 7, 2024, at 13:28, Dean Rasheed wrote: > I've also tidied up a bit by replacing all instances of SHRT_MAX with > a new constant NUMERIC_WEIGHT_MAX, whose name more accurately > describes the limit, as used in various other overflow checks. Having thought a bit more on this, I think we probably need a DEC_DIGITS sensitive definition of NUMERIC_WEIGHT_MAX, since per spec the max range for numeric is 0x20000 (131072) decimal digits. Therefore, I think perhaps what we want is: +#define NUMERIC_DSCALE_MIN 0 +#define NUMERIC_WEIGHT_MAX ((0x20000/DEC_DIGITS)-1) +#define NUMERIC_WEIGHT_MIN (-(NUMERIC_DSCALE_MAX+1)/DEC_DIGITS) Maybe also 0x20000 (131072) should be a defined constant. Regards, Joel
On Mon, 8 Jul 2024 at 00:40, Joel Jacobson <joel@compiler.org> wrote: > > On Sun, Jul 7, 2024, at 13:28, Dean Rasheed wrote: > > I've also tidied up a bit by replacing all instances of SHRT_MAX with > > a new constant NUMERIC_WEIGHT_MAX, whose name more accurately > > describes the limit, as used in various other overflow checks. > > Having thought a bit more on this, I think we probably need a > DEC_DIGITS sensitive definition of NUMERIC_WEIGHT_MAX, > since per spec the max range for numeric is 0x20000 (131072) > decimal digits. > No, the maximum weight is determined by the use of int16 to store the weight. Therefore if you did reduce DEC_DIGITS to 1 or 2, the number of decimal digits allowed before the decimal point would be reduced too. Regards, Dean
On Mon, Jul 8, 2024, at 11:45, Dean Rasheed wrote: > On Mon, 8 Jul 2024 at 00:40, Joel Jacobson <joel@compiler.org> wrote: >> >> On Sun, Jul 7, 2024, at 13:28, Dean Rasheed wrote: >> > I've also tidied up a bit by replacing all instances of SHRT_MAX with >> > a new constant NUMERIC_WEIGHT_MAX, whose name more accurately >> > describes the limit, as used in various other overflow checks. >> >> Having thought a bit more on this, I think we probably need a >> DEC_DIGITS sensitive definition of NUMERIC_WEIGHT_MAX, >> since per spec the max range for numeric is 0x20000 (131072) >> decimal digits. >> > > No, the maximum weight is determined by the use of int16 to store the > weight. Therefore if you did reduce DEC_DIGITS to 1 or 2, the number > of decimal digits allowed before the decimal point would be reduced > too. OK, that can actually be seen as a feature, especially since it's of course more likely DEC_DIGITS could increase in the future than decrease. For example, let's say we would double it to 8, then if NUMERIC_WEIGHT_MAX would still be 0x7FFF (32767), then the maximum range for numeric would increase from 131072 to 262144 decimal digits allowed before the decimal point. LGTM. Regards, Joel
On Mon, 8 Jul 2024 at 11:08, Joel Jacobson <joel@compiler.org> wrote: > > LGTM. > Thanks for the review. I have pushed and back-patched this. Regards, Dean