Skip to content

Commit

Permalink
Remove value validation in with_precision_and_scale
Browse files Browse the repository at this point in the history
  • Loading branch information
@viirya
viirya committed Oct 12, 2022
1 parent 89889be commit 3c1c27e
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 73 deletions.
80 changes: 19 additions & 61 deletions arrow-array/src/array/primitive_array.rs
View file
Expand Up @@ -855,14 +855,6 @@ impl<T: DecimalType + ArrowPrimitiveType> PrimitiveArray<T> {
// validate precision and scale
self.validate_precision_scale(precision, scale)?;

// Ensure that all values are within the requested
// precision. For performance, only check if the precision is
// decreased
let p = self.precision()?;
if precision < p {
self.validate_decimal_precision(precision)?;
}

// safety: self.data is valid DataType::Decimal as checked above
let new_data_type = T::TYPE_CONSTRUCTOR(precision, scale);
let data = self.data().clone().into_builder().data_type(new_data_type);
Expand All @@ -872,30 +864,6 @@ impl<T: DecimalType + ArrowPrimitiveType> PrimitiveArray<T> {
Ok(unsafe { data.build_unchecked().into() })
}

/// Returns a Decimal array with the same data as self, with the
/// specified precision.
///
/// # Safety
///
/// This doesn't validate decimal values with specified precision.
pub unsafe fn unchecked_with_precision_and_scale(
self,
precision: u8,
scale: u8,
) -> Result<Self, ArrowError>
where
Self: Sized,
{
// validate precision and scale
self.validate_precision_scale(precision, scale)?;

// safety: self.data is valid DataType::Decimal as checked above
let new_data_type = T::TYPE_CONSTRUCTOR(precision, scale);
let data = self.data().clone().into_builder().data_type(new_data_type);

Ok(data.build_unchecked().into())
}

// validate that the new precision and scale are valid or not
fn validate_precision_scale(
&self,
Expand Down Expand Up @@ -926,9 +894,9 @@ impl<T: DecimalType + ArrowPrimitiveType> PrimitiveArray<T> {
Ok(())
}

// Validates values in this array can be properly interpreted
// with the specified precision.
fn validate_decimal_precision(&self, precision: u8) -> Result<(), ArrowError> {
/// Validates values in this array can be properly interpreted
/// with the specified precision.
pub fn validate_decimal_precision(&self, precision: u8) -> Result<(), ArrowError> {
(0..self.len()).try_for_each(|idx| {
if self.is_valid(idx) {
let decimal = unsafe { self.value_unchecked(idx) };
Expand Down Expand Up @@ -1582,35 +1550,20 @@ mod tests {
}

#[test]
#[cfg(not(feature = "force_validate"))]
fn test_decimal_append_error_value() {
let mut decimal_builder = Decimal128Builder::with_capacity(10);
decimal_builder.append_value(123456);
let result = decimal_builder.finish().with_precision_and_scale(5, 3);
let mut error = result.unwrap_err();
assert_eq!(
"Invalid argument error: 123456 is too large to store in a Decimal128 of precision 5. Max is 99999",
error.to_string()
);

decimal_builder = Decimal128Builder::with_capacity(10);
decimal_builder.append_value(123456);
decimal_builder.append_value(12345);
let result = unsafe {
decimal_builder
.finish()
.unchecked_with_precision_and_scale(5, 3)
};
let result = decimal_builder.finish().with_precision_and_scale(5, 3);
assert!(result.is_ok());
let arr = result.unwrap();
assert_eq!("12.345", arr.value_as_string(1).unwrap());

decimal_builder = Decimal128Builder::new();
decimal_builder.append_value(100);
let result = decimal_builder.finish().with_precision_and_scale(2, 1);
error = result.unwrap_err();
// Validate it explicitly
let result = arr.validate_decimal_precision(5);
let error = result.unwrap_err();
assert_eq!(
"Invalid argument error: 100 is too large to store in a Decimal128 of precision 2. Max is 99",
"Invalid argument error: 123456 is too large to store in a Decimal128 of precision 5. Max is 99999",
error.to_string()
);

Expand All @@ -1619,15 +1572,19 @@ mod tests {
decimal_builder.append_value(99);
decimal_builder.append_value(-100);
decimal_builder.append_value(-99);
let result = unsafe {
decimal_builder
.finish()
.unchecked_with_precision_and_scale(2, 1)
};
let result = decimal_builder.finish().with_precision_and_scale(2, 1);
assert!(result.is_ok());
let arr = result.unwrap();
assert_eq!("9.9", arr.value_as_string(1).unwrap());
assert_eq!("-9.9", arr.value_as_string(3).unwrap());

// Validate it explicitly
let result = arr.validate_decimal_precision(2);
let error = result.unwrap_err();
assert_eq!(
"Invalid argument error: 100 is too large to store in a Decimal128 of precision 2. Max is 99",
error.to_string()
);
}

#[test]
Expand Down Expand Up @@ -1717,10 +1674,11 @@ mod tests {
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])
let arr = Decimal128Array::from_iter_values([12345, 456, 7890, -123223423432432])
// precision is too small to hold value
.with_precision_and_scale(5, 2)
.unwrap();
arr.validate_decimal_precision(5).unwrap();
}

#[test]
Expand Down
14 changes: 10 additions & 4 deletions arrow/src/compute/kernels/cast.rs
View file
Expand Up @@ -2816,9 +2816,12 @@ mod tests {
let input_decimal_array = create_decimal_array(array, 10, 0).unwrap();
let array = Arc::new(input_decimal_array) as ArrayRef;
let result = cast(&array, &DataType::Decimal128(2, 2));
assert!(result.is_err());
assert!(result.is_ok());
let array = result.unwrap();
let array: &Decimal128Array = as_primitive_array(&array);
let err = array.validate_decimal_precision(2);
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());
err.unwrap_err().to_string());
}

#[test]
Expand Down Expand Up @@ -3071,8 +3074,11 @@ mod tests {
let array = Int8Array::from(vec![1, 2, 3, 4, 100]);
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 Decimal128 of precision 3. Max is 999", casted_array.unwrap_err().to_string());
assert!(casted_array.is_ok());
let array = casted_array.unwrap();
let array: &Decimal128Array = as_primitive_array(&array);
let err = array.validate_decimal_precision(3);
assert_eq!("Invalid argument error: 1000 is too large to store in a Decimal128 of precision 3. Max is 999", err.unwrap_err().to_string());

// test f32 to decimal type
let array = Float32Array::from(vec![
Expand Down
14 changes: 6 additions & 8 deletions integration-testing/src/util/mod.rs
View file
Expand Up @@ -786,7 +786,7 @@ pub fn array_from_json(
))),
}
}
DataType::Decimal128(precision, scale) => unsafe {
DataType::Decimal128(precision, scale) => {
let mut b = Decimal128Builder::with_capacity(json_col.count);
for (is_valid, value) in json_col
.validity
Expand All @@ -801,11 +801,10 @@ pub fn array_from_json(
};
}
Ok(Arc::new(
b.finish()
.unchecked_with_precision_and_scale(*precision, *scale)?,
b.finish().with_precision_and_scale(*precision, *scale)?,
))
},
DataType::Decimal256(precision, scale) => unsafe {
}
DataType::Decimal256(precision, scale) => {
let mut b = Decimal256Builder::with_capacity(json_col.count);
for (is_valid, value) in json_col
.validity
Expand All @@ -832,10 +831,9 @@ pub fn array_from_json(
}
}
Ok(Arc::new(
b.finish()
.unchecked_with_precision_and_scale(*precision, *scale)?,
b.finish().with_precision_and_scale(*precision, *scale)?,
))
},
}
DataType::Map(child_field, _) => {
let null_buf = create_null_buf(&json_col);
let children = json_col.children.clone().unwrap();
Expand Down

0 comments on commit 3c1c27e

Please sign in to comment.