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.

Sign up for GitHub

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

Fix max and min value for decimal precision greater than 38 #2245

Merged
merged 1 commit into from
Jul 31, 2022
Merged

Fix max and min value for decimal precision greater than 38 #2245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions arrow/src/array/array_decimal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,7 @@ mod tests {
let mut result = decimal_builder.append_value(123456);
let mut error = result.unwrap_err();
assert_eq!(
"Invalid argument error: 123456 is too large to store in a Decimal of precision 5. Max is 99999",
"Invalid argument error: 123456 is too large to store in a Decimal128 of precision 5. Max is 99999",
error.to_string()
);

Expand All @@ -558,7 +558,7 @@ mod tests {
result = decimal_builder.append_value(100);
error = result.unwrap_err();
assert_eq!(
"Invalid argument error: 100 is too large to store in a Decimal of precision 2. Max is 99",
"Invalid argument error: 100 is too large to store in a Decimal128 of precision 2. Max is 99",
error.to_string()
);

Expand Down Expand Up @@ -677,7 +677,7 @@ mod tests {

#[test]
#[should_panic(
expected = "-123223423432432 is too small to store in a Decimal of precision 5. Min is -99999"
expected = "-123223423432432 is too small to store in a Decimal128 of precision 5. Min is -99999"
)]
fn test_decimal_array_with_precision_and_scale_out_of_range() {
Decimal128Array::from_iter_values([12345, 456, 7890, -123223423432432])
Expand Down
16 changes: 7 additions & 9 deletions arrow/src/array/builder/decimal_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ mod tests {
use crate::array::array_decimal::{BasicDecimalArray, Decimal128Array};
use crate::array::{array_decimal, Array};
use crate::datatypes::DataType;
use crate::util::decimal::Decimal128;
use crate::util::decimal::{Decimal128, Decimal256};

#[test]
fn test_decimal_builder() {
Expand Down Expand Up @@ -357,31 +357,29 @@ mod tests {

#[test]
#[should_panic(
expected = "9999999999999999999999999999999999999999999999999999999999999999999999999999 is too large to store in a Decimal256 of precision 76. Max is 999999999999999999999999999999999999999999999999999999999999999999999999999"
expected = "9999999999999999999999999999999999999999999999999999999999999999999999999999 is too large to store in a Decimal256 of precision 75. Max is 999999999999999999999999999999999999999999999999999999999999999999999999999"
)]
fn test_decimal256_builder_out_of_range_precision_scale() {
let mut builder = Decimal256Builder::new(30, 76, 6);
let mut builder = Decimal256Builder::new(30, 75, 6);

let big_value = BigInt::from_str_radix("9999999999999999999999999999999999999999999999999999999999999999999999999999", 10).unwrap();
let bytes = big_value.to_signed_bytes_le();
let value = Decimal256::try_new_from_bytes(76, 6, &bytes).unwrap();
let value = Decimal256::from_big_int(&big_value, 75, 6).unwrap();
builder.append_value(&value).unwrap();
}

#[test]
#[should_panic(
expected = "9999999999999999999999999999999999999999999999999999999999999999999999999999 is too large to store in a Decimal256 of precision 76. Max is 999999999999999999999999999999999999999999999999999999999999999999999999999"
expected = "9999999999999999999999999999999999999999999999999999999999999999999999999999 is too large to store in a Decimal256 of precision 75. Max is 999999999999999999999999999999999999999999999999999999999999999999999999999"
)]
fn test_decimal256_data_validation() {
let mut builder = Decimal256Builder::new(30, 76, 6);
let mut builder = Decimal256Builder::new(30, 75, 6);
// Disable validation at builder
unsafe {
builder.disable_value_validation();
}

let big_value = BigInt::from_str_radix("9999999999999999999999999999999999999999999999999999999999999999999999999999", 10).unwrap();
let bytes = big_value.to_signed_bytes_le();
let value = Decimal256::try_new_from_bytes(76, 6, &bytes).unwrap();
let value = Decimal256::from_big_int(&big_value, 75, 6).unwrap();
builder
.append_value(&value)
.expect("should not validate invalid value at builder");
Expand Down
2 changes: 1 addition & 1 deletion arrow/src/array/data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2841,7 +2841,7 @@ mod tests {
let validation_result = array_data.validate_full();
let error = validation_result.unwrap_err();
assert_eq!(
"Invalid argument error: 123456 is too large to store in a Decimal of precision 5. Max is 99999",
"Invalid argument error: 123456 is too large to store in a Decimal128 of precision 5. Max is 99999",
error.to_string()
);
}
Expand Down
4 changes: 2 additions & 2 deletions arrow/src/compute/kernels/cast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2230,7 +2230,7 @@ mod tests {
let array = Arc::new(input_decimal_array) as ArrayRef;
let result = cast(&array, &DataType::Decimal128(2, 2));
assert!(result.is_err());
assert_eq!("Invalid argument error: 12345600 is too large to store in a Decimal of precision 2. Max is 99",
assert_eq!("Invalid argument error: 12345600 is too large to store in a Decimal128 of precision 2. Max is 99",
result.unwrap_err().to_string());
}

Expand Down Expand Up @@ -2412,7 +2412,7 @@ mod tests {
let array = Arc::new(array) as ArrayRef;
let casted_array = cast(&array, &DataType::Decimal128(3, 1));
assert!(casted_array.is_err());
assert_eq!("Invalid argument error: 1000 is too large to store in a Decimal of precision 3. Max is 999", casted_array.unwrap_err().to_string());
assert_eq!("Invalid argument error: 1000 is too large to store in a Decimal128 of precision 3. Max is 999", casted_array.unwrap_err().to_string());

// test f32 to decimal type
let array = Float32Array::from(vec![
Expand Down
16 changes: 9 additions & 7 deletions arrow/src/datatypes/datatype.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,6 @@ pub const MAX_DECIMAL_FOR_EACH_PRECISION: [i128; 38] = [
/// `MAX_DECIMAL_FOR_LARGER_PRECISION[p]` holds the maximum integer value
/// that can be stored in [DataType::Decimal256] value of precision `p` > 38
pub const MAX_DECIMAL_FOR_LARGER_PRECISION: [&str; 38] = [
"99999999999999999999999999999999999999",
Copy link
Member Author

@viirya viirya Jul 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only 38 digits, it should be 39. 😓

"999999999999999999999999999999999999999",
"9999999999999999999999999999999999999999",
"99999999999999999999999999999999999999999",
Expand Down Expand Up @@ -347,6 +346,7 @@ pub const MAX_DECIMAL_FOR_LARGER_PRECISION: [&str; 38] = [
"9999999999999999999999999999999999999999999999999999999999999999999999999",
"99999999999999999999999999999999999999999999999999999999999999999999999999",
"999999999999999999999999999999999999999999999999999999999999999999999999999",
"9999999999999999999999999999999999999999999999999999999999999999999999999999",
];

/// `MIN_DECIMAL_FOR_EACH_PRECISION[p]` holds the minimum `i128` value
Expand Down Expand Up @@ -395,7 +395,6 @@ pub const MIN_DECIMAL_FOR_EACH_PRECISION: [i128; 38] = [
/// `MIN_DECIMAL_FOR_LARGER_PRECISION[p]` holds the minimum integer value
/// that can be stored in a [DataType::Decimal256] value of precision `p` > 38
pub const MIN_DECIMAL_FOR_LARGER_PRECISION: [&str; 38] = [
"-99999999999999999999999999999999999999",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

"-999999999999999999999999999999999999999",
"-9999999999999999999999999999999999999999",
"-99999999999999999999999999999999999999999",
Expand Down Expand Up @@ -433,6 +432,7 @@ pub const MIN_DECIMAL_FOR_LARGER_PRECISION: [&str; 38] = [
"-9999999999999999999999999999999999999999999999999999999999999999999999999",
"-99999999999999999999999999999999999999999999999999999999999999999999999999",
"-999999999999999999999999999999999999999999999999999999999999999999999999999",
"-9999999999999999999999999999999999999999999999999999999999999999999999999999",
];

/// The maximum precision for [DataType::Decimal128] values
Expand All @@ -454,22 +454,24 @@ pub const DECIMAL_DEFAULT_SCALE: usize = 10;
/// interpreted as a Decimal number with precision `precision`
#[inline]
pub(crate) fn validate_decimal_precision(value: i128, precision: usize) -> Result<i128> {
// TODO: add validation logic for precision > 38
if precision > 38 {
return Ok(value);
if precision > DECIMAL128_MAX_PRECISION {
return Err(ArrowError::InvalidArgumentError(format!(
"Max precision of a Decimal128 is {}, but got {}",
DECIMAL128_MAX_PRECISION, precision,
)));
}

let max = MAX_DECIMAL_FOR_EACH_PRECISION[precision - 1];
let min = MIN_DECIMAL_FOR_EACH_PRECISION[precision - 1];

if value > max {
Err(ArrowError::InvalidArgumentError(format!(
"{} is too large to store in a Decimal of precision {}. Max is {}",
"{} is too large to store in a Decimal128 of precision {}. Max is {}",
value, precision, max
)))
} else if value < min {
Err(ArrowError::InvalidArgumentError(format!(
"{} is too small to store in a Decimal of precision {}. Min is {}",
"{} is too small to store in a Decimal128 of precision {}. Min is {}",
value, precision, min
)))
} else {
Expand Down