From 281cd793750182d434e9add99770a1e7c14fc811 Mon Sep 17 00:00:00 2001 From: Liang-Chi Hsieh Date: Sun, 31 Jul 2022 10:38:42 -0700 Subject: [PATCH] Fix max and min value for decimal256 (#2245) --- arrow/src/array/array_decimal.rs | 6 +++--- arrow/src/array/builder/decimal_builder.rs | 16 +++++++--------- arrow/src/array/data.rs | 2 +- arrow/src/compute/kernels/cast.rs | 4 ++-- arrow/src/datatypes/datatype.rs | 16 +++++++++------- 5 files changed, 22 insertions(+), 22 deletions(-) diff --git a/arrow/src/array/array_decimal.rs b/arrow/src/array/array_decimal.rs index 6a453fc922e..cb9f799fd2b 100644 --- a/arrow/src/array/array_decimal.rs +++ b/arrow/src/array/array_decimal.rs @@ -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() ); @@ -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() ); @@ -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]) diff --git a/arrow/src/array/builder/decimal_builder.rs b/arrow/src/array/builder/decimal_builder.rs index 09127e76f5a..22c1490e86f 100644 --- a/arrow/src/array/builder/decimal_builder.rs +++ b/arrow/src/array/builder/decimal_builder.rs @@ -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() { @@ -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"); diff --git a/arrow/src/array/data.rs b/arrow/src/array/data.rs index 51de65f0964..43c43b04a51 100644 --- a/arrow/src/array/data.rs +++ b/arrow/src/array/data.rs @@ -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() ); } diff --git a/arrow/src/compute/kernels/cast.rs b/arrow/src/compute/kernels/cast.rs index 7e803766bb1..ea166f921df 100644 --- a/arrow/src/compute/kernels/cast.rs +++ b/arrow/src/compute/kernels/cast.rs @@ -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()); } @@ -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![ diff --git a/arrow/src/datatypes/datatype.rs b/arrow/src/datatypes/datatype.rs index 9f23e6f790a..034920d3753 100644 --- a/arrow/src/datatypes/datatype.rs +++ b/arrow/src/datatypes/datatype.rs @@ -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", "999999999999999999999999999999999999999", "9999999999999999999999999999999999999999", "99999999999999999999999999999999999999999", @@ -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 @@ -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", "-999999999999999999999999999999999999999", "-9999999999999999999999999999999999999999", "-99999999999999999999999999999999999999999", @@ -433,6 +432,7 @@ pub const MIN_DECIMAL_FOR_LARGER_PRECISION: [&str; 38] = [ "-9999999999999999999999999999999999999999999999999999999999999999999999999", "-99999999999999999999999999999999999999999999999999999999999999999999999999", "-999999999999999999999999999999999999999999999999999999999999999999999999999", + "-9999999999999999999999999999999999999999999999999999999999999999999999999999", ]; /// The maximum precision for [DataType::Decimal128] values @@ -454,9 +454,11 @@ 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 { - // 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]; @@ -464,12 +466,12 @@ pub(crate) fn validate_decimal_precision(value: i128, precision: usize) -> Resul 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 {