Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes Display Overflow issue when using precision greater than 30 #433

Merged
merged 5 commits into from
Oct 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@ pub const SIGN_SHIFT: u32 = 31;
pub const MAX_STR_BUFFER_SIZE: usize = 32;

// The maximum supported precision
pub const MAX_PRECISION: u32 = 28;
pub const MAX_PRECISION: u8 = 28;
#[cfg(not(feature = "legacy-ops"))]
pub const MAX_PRECISION_I32: i32 = 28;
// u8 to i32 is infallible, therefore, this cast will never overflow
pub const MAX_PRECISION_I32: i32 = MAX_PRECISION as _;
// u8 to u32 is infallible, therefore, this cast will never overflow
pub const MAX_PRECISION_U32: u32 = MAX_PRECISION as _;
// 79,228,162,514,264,337,593,543,950,335
pub const MAX_I128_REPR: i128 = 0x0000_0000_FFFF_FFFF_FFFF_FFFF_FFFF_FFFF;

Expand Down
9 changes: 5 additions & 4 deletions src/db.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::constants::MAX_PRECISION;
use crate::constants::MAX_PRECISION_U32;
use crate::{
ops::array::{div_by_u32, is_all_zero, mul_by_u32},
Decimal,
Expand Down Expand Up @@ -64,12 +64,13 @@ impl Decimal {
let start_fractionals = if weight < 0 { (-weight as u32) - 1 } else { 0 };
for (i, digit) in digits.into_iter().enumerate() {
let fract_pow = 4 * (i as u32 + 1 + start_fractionals);
if fract_pow <= MAX_PRECISION {
if fract_pow <= MAX_PRECISION_U32 {
result += Decimal::new(digit as i64, 0) / Decimal::from_i128_with_scale(10i128.pow(fract_pow), 0);
} else if fract_pow == MAX_PRECISION + 4 {
} else if fract_pow == MAX_PRECISION_U32 + 4 {
// rounding last digit
if digit >= 5000 {
result += Decimal::new(1_i64, 0) / Decimal::from_i128_with_scale(10i128.pow(MAX_PRECISION), 0);
result +=
Decimal::new(1_i64, 0) / Decimal::from_i128_with_scale(10i128.pow(MAX_PRECISION_U32), 0);
}
}
}
Expand Down
41 changes: 29 additions & 12 deletions src/decimal.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::constants::{
MAX_I128_REPR, MAX_PRECISION, POWERS_10, SCALE_MASK, SCALE_SHIFT, SIGN_MASK, SIGN_SHIFT, U32_MASK, U8_MASK,
MAX_I128_REPR, MAX_PRECISION_U32, POWERS_10, SCALE_MASK, SCALE_SHIFT, SIGN_MASK, SIGN_SHIFT, U32_MASK, U8_MASK,
UNSIGN_MASK,
};
use crate::ops;
Expand Down Expand Up @@ -263,7 +263,7 @@ impl Decimal {
/// assert!(max.is_err());
/// ```
pub const fn try_new(num: i64, scale: u32) -> crate::Result<Decimal> {
if scale > MAX_PRECISION {
if scale > MAX_PRECISION_U32 {
return Err(Error::ScaleExceedsMaximumPrecision(scale));
}
let flags: u32 = scale << SCALE_SHIFT;
Expand Down Expand Up @@ -323,7 +323,7 @@ impl Decimal {
/// assert!(max.is_err());
/// ```
pub const fn try_from_i128_with_scale(num: i128, scale: u32) -> crate::Result<Decimal> {
if scale > MAX_PRECISION {
if scale > MAX_PRECISION_U32 {
return Err(Error::ScaleExceedsMaximumPrecision(scale));
}
let mut neg = false;
Expand Down Expand Up @@ -382,7 +382,7 @@ impl Decimal {
} else {
negative
},
scale % (MAX_PRECISION + 1),
scale % (MAX_PRECISION_U32 + 1),
),
}
}
Expand Down Expand Up @@ -442,7 +442,7 @@ impl Decimal {
// we've parsed 1.2 as the base and 10 as the exponent. To represent this within a
// Decimal type we effectively store the mantissa as 12,000,000,000 and scale as
// zero.
if exp > MAX_PRECISION {
if exp > MAX_PRECISION_U32 {
return Err(Error::ScaleExceedsMaximumPrecision(exp));
}
let mut exp = exp as usize;
Expand Down Expand Up @@ -606,7 +606,7 @@ impl Decimal {
/// assert_eq!(one.to_string(), "0.00001");
/// ```
pub fn set_scale(&mut self, scale: u32) -> Result<(), Error> {
if scale > MAX_PRECISION {
if scale > MAX_PRECISION_U32 {
return Err(Error::ScaleExceedsMaximumPrecision(scale));
}
self.flags = (scale << SCALE_SHIFT) | (self.flags & SIGN_MASK);
Expand Down Expand Up @@ -691,13 +691,25 @@ impl Decimal {
/// * Bytes 9-12: mid portion of `m`
/// * Bytes 13-16: high portion of `m`
#[must_use]
pub const fn deserialize(bytes: [u8; 16]) -> Decimal {
Decimal {
flags: (bytes[0] as u32) | (bytes[1] as u32) << 8 | (bytes[2] as u32) << 16 | (bytes[3] as u32) << 24,
pub fn deserialize(bytes: [u8; 16]) -> Decimal {
// We can bound flags by a bitwise mask to correspond to:
// Bits 0-15: unused
// Bits 16-23: Contains "e", a value between 0-28 that indicates the scale
// Bits 24-30: unused
// Bit 31: the sign of the Decimal value, 0 meaning positive and 1 meaning negative.
let raw = Decimal {
flags: ((bytes[0] as u32) | (bytes[1] as u32) << 8 | (bytes[2] as u32) << 16 | (bytes[3] as u32) << 24)
& 0x801F_0000,
lo: (bytes[4] as u32) | (bytes[5] as u32) << 8 | (bytes[6] as u32) << 16 | (bytes[7] as u32) << 24,
mid: (bytes[8] as u32) | (bytes[9] as u32) << 8 | (bytes[10] as u32) << 16 | (bytes[11] as u32) << 24,
hi: (bytes[12] as u32) | (bytes[13] as u32) << 8 | (bytes[14] as u32) << 16 | (bytes[15] as u32) << 24,
};
// Scale must be bound to maximum precision. The panic currently prevents this function being
// marked as a const function.
if raw.scale() > MAX_PRECISION_U32 {
panic!("{}", Error::ScaleExceedsMaximumPrecision(raw.scale()))
}
raw
}

/// Returns `true` if the decimal is negative.
Expand Down Expand Up @@ -1454,7 +1466,7 @@ impl Decimal {
// In order to bring exponent up to -MAX_PRECISION, the mantissa should
// be divided by 10 to compensate. If the exponent10 is too small, this
// will cause the mantissa to underflow and become 0.
while exponent10 < -(MAX_PRECISION as i32) {
while exponent10 < -(MAX_PRECISION_U32 as i32) {
let rem10 = ops::array::div_by_u32(bits, 10);
exponent10 += 1;
if ops::array::is_all_zero(bits) {
Expand Down Expand Up @@ -2053,8 +2065,13 @@ impl core::convert::TryFrom<Decimal> for f64 {

impl fmt::Display for Decimal {
fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
let rep = crate::str::to_str_internal(self, false, f.precision());
f.pad_integral(self.is_sign_positive(), "", rep.as_str())
let (rep, additional) = crate::str::to_str_internal(self, false, f.precision());
if let Some(additional) = additional {
let value = [rep.as_str(), "0".repeat(additional).as_str()].concat();
f.pad_integral(self.is_sign_positive(), "", value.as_str())
} else {
f.pad_integral(self.is_sign_positive(), "", rep.as_str())
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/error.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::constants::MAX_PRECISION;
use crate::constants::MAX_PRECISION_U32;
use alloc::string::String;
use core::fmt;

Expand Down Expand Up @@ -38,7 +38,7 @@ impl fmt::Display for Error {
write!(
f,
"Scale exceeds the maximum precision allowed: {} > {}",
scale, MAX_PRECISION
scale, MAX_PRECISION_U32
)
}
}
Expand Down
20 changes: 10 additions & 10 deletions src/ops/legacy.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::{
constants::{MAX_PRECISION, POWERS_10, U32_MASK},
constants::{MAX_PRECISION_U32, POWERS_10, U32_MASK},
decimal::{CalculationResult, Decimal},
ops::array::{
add_by_internal, cmp_internal, div_by_u32, is_all_zero, mul_by_u32, mul_part, rescale_internal, shl1_internal,
Expand Down Expand Up @@ -171,16 +171,16 @@ pub(crate) fn div_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult {

// Check for underflow
let mut final_scale: u32 = quotient_scale as u32;
if final_scale > MAX_PRECISION {
if final_scale > MAX_PRECISION_U32 {
let mut remainder = 0;

// Division underflowed. We must remove some significant digits over using
// an invalid scale.
while final_scale > MAX_PRECISION && !is_all_zero(&quotient) {
while final_scale > MAX_PRECISION_U32 && !is_all_zero(&quotient) {
remainder = div_by_u32(&mut quotient, 10);
final_scale -= 1;
}
if final_scale > MAX_PRECISION {
if final_scale > MAX_PRECISION_U32 {
// Result underflowed so set to zero
final_scale = 0;
quotient_negative = false;
Expand Down Expand Up @@ -228,8 +228,8 @@ pub(crate) fn mul_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult {
let mut u64_result = u64_to_array(u64::from(my[0]) * u64::from(ot[0]));

// If we're above max precision then this is a very small number
if final_scale > MAX_PRECISION {
final_scale -= MAX_PRECISION;
if final_scale > MAX_PRECISION_U32 {
final_scale -= MAX_PRECISION_U32;

// If the number is above 19 then this will equate to zero.
// This is because the max value in 64 bits is 1.84E19
Expand Down Expand Up @@ -258,7 +258,7 @@ pub(crate) fn mul_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult {
u64_result[0] += 1;
}

final_scale = MAX_PRECISION;
final_scale = MAX_PRECISION_U32;
}
return CalculationResult::Ok(Decimal::from_parts(
u64_result[0],
Expand Down Expand Up @@ -350,17 +350,17 @@ pub(crate) fn mul_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult {

// If we're still above max precision then we'll try again to
// reduce precision - we may be dealing with a limit of "0"
if final_scale > MAX_PRECISION {
if final_scale > MAX_PRECISION_U32 {
// We're in an underflow situation
// The easiest way to remove precision is to divide off the result
while final_scale > MAX_PRECISION && !is_all_zero(&product) {
while final_scale > MAX_PRECISION_U32 && !is_all_zero(&product) {
div_by_u32(&mut product, 10);
final_scale -= 1;
}
// If we're still at limit then we can't represent any
// significant decimal digits and will return an integer only
// Can also be invoked while representing 0.
if final_scale > MAX_PRECISION {
if final_scale > MAX_PRECISION_U32 {
final_scale = 0;
}
} else if !(product[3] == 0 && product[4] == 0 && product[5] == 0) {
Expand Down
12 changes: 6 additions & 6 deletions src/ops/mul.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::constants::{BIG_POWERS_10, MAX_I64_SCALE, MAX_PRECISION, U32_MAX};
use crate::constants::{BIG_POWERS_10, MAX_I64_SCALE, MAX_PRECISION_U32, U32_MAX};
use crate::decimal::{CalculationResult, Decimal};
use crate::ops::common::Buf24;

Expand All @@ -18,15 +18,15 @@ pub(crate) fn mul_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult {
if d2.hi() | d2.mid() == 0 {
// We're multiplying two 32 bit integers, so we can take some liberties to optimize this.
let mut low64 = d1.lo() as u64 * d2.lo() as u64;
if scale > MAX_PRECISION {
if scale > MAX_PRECISION_U32 {
// We've exceeded maximum scale so we need to start reducing the precision (aka
// rounding) until we have something that fits.
// If we're too big then we effectively round to zero.
if scale > MAX_PRECISION + MAX_I64_SCALE {
if scale > MAX_PRECISION_U32 + MAX_I64_SCALE {
return CalculationResult::Ok(Decimal::ZERO);
}

scale -= MAX_PRECISION + 1;
scale -= MAX_PRECISION_U32 + 1;
let mut power = BIG_POWERS_10[scale as usize];

let tmp = low64 / power;
Expand All @@ -39,7 +39,7 @@ pub(crate) fn mul_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult {
low64 += 1;
}

scale = MAX_PRECISION;
scale = MAX_PRECISION_U32;
}

// Early exit
Expand Down Expand Up @@ -129,7 +129,7 @@ pub(crate) fn mul_impl(d1: &Decimal, d2: &Decimal) -> CalculationResult {
// We may want to "rescale". This is the case if the mantissa is > 96 bits or if the scale
// exceeds the maximum precision.
let upper_word = product.upper_word();
if upper_word > 2 || scale > MAX_PRECISION {
if upper_word > 2 || scale > MAX_PRECISION_U32 {
scale = if let Some(new_scale) = product.rescale(upper_word, scale) {
new_scale
} else {
Expand Down
3 changes: 2 additions & 1 deletion src/serde.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,8 @@ impl serde::Serialize for Decimal {
where
S: serde::Serializer,
{
serializer.serialize_str(crate::str::to_str_internal(self, true, None).as_ref())
let value = crate::str::to_str_internal(self, true, None);
serializer.serialize_str(value.0.as_ref())
}
}

Expand Down
34 changes: 28 additions & 6 deletions src/str.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::{
constants::MAX_STR_BUFFER_SIZE,
constants::{MAX_PRECISION, MAX_STR_BUFFER_SIZE},
error::Error,
ops::array::{add_by_internal, add_one_internal, div_by_u32, is_all_zero, mul_by_10, mul_by_u32},
Decimal,
Expand All @@ -15,7 +15,7 @@ pub(crate) fn to_str_internal(
value: &Decimal,
append_sign: bool,
precision: Option<usize>,
) -> ArrayString<[u8; MAX_STR_BUFFER_SIZE]> {
) -> (ArrayString<[u8; MAX_STR_BUFFER_SIZE]>, Option<usize>) {
// Get the scale - where we need to put the decimal point
let scale = value.scale() as usize;

Expand All @@ -30,9 +30,16 @@ pub(crate) fn to_str_internal(
chars.push('0');
}

let prec = match precision {
Some(prec) => prec,
None => scale,
let (prec, additional) = match precision {
Some(prec) => {
let max: usize = MAX_PRECISION.into();
if prec > max {
(max, Some(prec - max))
} else {
(prec, None)
}
}
None => (scale, None),
};

let len = chars.len();
Expand Down Expand Up @@ -66,7 +73,7 @@ pub(crate) fn to_str_internal(
rep.push('0');
}

rep
(rep, additional)
}

pub(crate) fn fmt_scientific_notation(
Expand Down Expand Up @@ -531,3 +538,18 @@ pub(crate) fn parse_str_radix_n(str: &str, radix: u32) -> Result<Decimal, crate:

Ok(Decimal::from_parts(data[0], data[1], data[2], negative, scale))
}

#[cfg(test)]
mod test {
use crate::Decimal;
use arrayvec::ArrayString;
use core::{fmt::Write, str::FromStr};

#[test]
fn display_does_not_overflow_max_capacity() {
let num = Decimal::from_str("1.2").unwrap();
let mut buffer = ArrayString::<[u8; 64]>::new();
let _ = buffer.write_fmt(format_args!("{:.31}", num)).unwrap();
assert_eq!("1.2000000000000000000000000000000", buffer.as_str());
}
}
17 changes: 17 additions & 0 deletions tests/decimal_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,23 @@ fn it_can_serialize_deserialize() {
}
}

#[test]
#[should_panic(expected = "Scale exceeds the maximum precision allowed: 30 > 28")]
fn it_panics_deserializing_unbounded_values() {
let _ = Decimal::deserialize([1u8, 0, 30, 206, 97, 81, 216, 182, 20, 30, 165, 78, 18, 155, 169, 62]);
}

#[test]
fn it_can_deserialize_bounded_values() {
let tests = [[1u8, 0, 28, 206, 97, 81, 216, 182, 20, 30, 165, 78, 18, 155, 169, 62]];
for &bytes in &tests {
let dec = Decimal::deserialize(bytes);
let string = format!("{:.9999}", dec);
let dec2 = Decimal::from_str(&string).unwrap();
assert_eq!(dec, dec2);
}
}

// Formatting

#[test]
Expand Down