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

Jump to bottom

Collocate Decimal Array Validation Logic #2446

Merged
merged 2 commits into from Aug 17, 2022
Merged

Collocate Decimal Array Validation Logic #2446

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed60bbc
collation the validate precision code for decimal array
liukun4515 Aug 14, 2022
b933a42
Merge remote-tracking branch 'upstream/master' into collation_decimal…
liukun4515 Aug 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
167 changes: 72 additions & 95 deletions arrow/src/array/array_decimal.rs
View file
Open in desktop
Expand Up @@ -249,39 +249,6 @@ impl<const BYTE_WIDTH: usize> BasicDecimalArray<BYTE_WIDTH> {
fn raw_value_data_ptr(&self) -> *const u8 {
self.value_data.as_ptr()
}
}

impl Decimal128Array {
/// Creates a [Decimal128Array] with default precision and scale,
/// based on an iterator of `i128` values without nulls
pub fn from_iter_values<I: IntoIterator<Item = i128>>(iter: I) -> Self {
let val_buf: Buffer = iter.into_iter().collect();
let data = unsafe {
ArrayData::new_unchecked(
Self::default_type(),
val_buf.len() / std::mem::size_of::<i128>(),
None,
None,
0,
vec![val_buf],
vec![],
)
};
Decimal128Array::from(data)
}

// Validates decimal values in this array can be properly interpreted
// with the specified precision.
fn validate_decimal_precision(&self, precision: usize) -> Result<()> {
(0..self.len()).try_for_each(|idx| {
if self.is_valid(idx) {
let decimal = unsafe { self.value_unchecked(idx) };
validate_decimal_precision(decimal.as_i128(), precision)
} else {
Ok(())
}
})
}

/// Returns a Decimal array with the same data as self, with the
/// specified precision.
Expand All @@ -294,6 +261,23 @@ impl Decimal128Array {
where
Self: Sized,
{
// 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
if precision < self.precision {
self.validate_data(precision)?;
}

// safety: self.data is valid DataType::Decimal as checked above
let new_data_type = Self::TYPE_CONSTRUCTOR(precision, scale);
Ok(self.data().clone().with_data_type(new_data_type).into())
}

// validate that the new precision and scale are valid or not
fn validate_precision_scale(&self, precision: usize, scale: usize) -> Result<()> {
if precision > Self::MAX_PRECISION {
return Err(ArrowError::InvalidArgumentError(format!(
"precision {} is greater than max {}",
Expand All @@ -314,26 +298,67 @@ impl Decimal128Array {
scale, precision
)));
}

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

let data_type = Self::TYPE_CONSTRUCTOR(self.precision, self.scale);
assert_eq!(self.data().data_type(), &data_type);

// safety: self.data is valid DataType::Decimal as checked above
let new_data_type = Self::TYPE_CONSTRUCTOR(precision, scale);
Ok(())
}

// validate all the data in the array are valid within the new precision or not
fn validate_data(&self, precision: usize) -> Result<()> {
match BYTE_WIDTH {
16 => self
.as_any()
.downcast_ref::<Decimal128Array>()
.unwrap()
.validate_decimal_precision(precision),
32 => self
.as_any()
.downcast_ref::<Decimal256Array>()
.unwrap()
.validate_decimal_precision(precision),
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this logic should live in ArrayData, where all the rest of the validation logic lives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some usage of

arrow-rs/arrow/src/array/data.rs

Line 329 in 42e9531

new_self.validate_full().unwrap();

arrow-rs/arrow/src/array/data.rs

Line 377 in 42e9531

new_self.validate_full()?;

arrow-rs/arrow/src/array/data.rs

Line 1037 in 42e9531

pub fn validate_values(&self) -> Result<()> {

I will go through the usage, and check if it is necessary or not

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm suggesting that rather than this PR implementing additional validation, it should just use the validation logic that already exists in ArrayData?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds great, maybe I can try it in the follow up pr.
we remain the original logic and just refactor this struct and impl now
I just concerned about the performance of the ArrayData. We have the benchmark which can be used to bench it easily.

@alamb add the validation for decimalarray with_precision_scale, I also have the same question. Why not we use the logic in the ArrayData?

Copy link
Contributor

Choose a reason for hiding this comment

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

Consolidating validation in ArrayData sounds like a great plan to me. I can't remember any particular reason it isn't there

other_width => {
panic!("invalid byte width {}", other_width);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you test that if this is a compile error or a runtime error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't test this error.
Guess this is a runtime error, when use BasicDecimalArray<OTHER_WIDTH> to call this method.

}
}
}
}

Ok(self.data().clone().with_data_type(new_data_type).into())
impl Decimal128Array {
/// Creates a [Decimal128Array] with default precision and scale,
/// based on an iterator of `i128` values without nulls
pub fn from_iter_values<I: IntoIterator<Item = i128>>(iter: I) -> Self {
let val_buf: Buffer = iter.into_iter().collect();
let data = unsafe {
ArrayData::new_unchecked(
Self::default_type(),
val_buf.len() / std::mem::size_of::<i128>(),
None,
None,
0,
vec![val_buf],
vec![],
)
};
Decimal128Array::from(data)
}

// Validates decimal128 values in this array can be properly interpreted
// with the specified precision.
fn validate_decimal_precision(&self, precision: usize) -> Result<()> {
(0..self.len()).try_for_each(|idx| {
if self.is_valid(idx) {
let decimal = unsafe { self.value_unchecked(idx) };
validate_decimal_precision(decimal.as_i128(), precision)
} else {
Ok(())
}
})
}
}

impl Decimal256Array {
// Validates decimal values in this array can be properly interpreted
// Validates decimal256 values in this array can be properly interpreted
// with the specified precision.
fn validate_decimal_precision(&self, precision: usize) -> Result<()> {
(0..self.len()).try_for_each(|idx| {
Expand All @@ -351,54 +376,6 @@ impl Decimal256Array {
}
})
}

/// Returns a Decimal array with the same data as self, with the
/// specified precision.
///
/// Returns an Error if:
/// 1. `precision` is larger than [`Self::MAX_PRECISION`]
/// 2. `scale` is larger than [`Self::MAX_SCALE`];
/// 3. `scale` is > `precision`
pub fn with_precision_and_scale(self, precision: usize, scale: usize) -> Result<Self>
where
Self: Sized,
{
if precision > Self::MAX_PRECISION {
return Err(ArrowError::InvalidArgumentError(format!(
"precision {} is greater than max {}",
precision,
Self::MAX_PRECISION
)));
}
if scale > Self::MAX_SCALE {
return Err(ArrowError::InvalidArgumentError(format!(
"scale {} is greater than max {}",
scale,
Self::MAX_SCALE
)));
}
if scale > precision {
return Err(ArrowError::InvalidArgumentError(format!(
"scale {} is greater than precision {}",
scale, precision
)));
}

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

let data_type = Self::TYPE_CONSTRUCTOR(self.precision, self.scale);
assert_eq!(self.data().data_type(), &data_type);

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

Ok(self.data().clone().with_data_type(new_data_type).into())
}
}

impl<const BYTE_WIDTH: usize> From<ArrayData> for BasicDecimalArray<BYTE_WIDTH> {
Expand Down Expand Up @@ -942,7 +919,7 @@ mod tests {
Decimal256::from_big_int(
&value1,
DECIMAL256_MAX_PRECISION,
DECIMAL_DEFAULT_SCALE
DECIMAL_DEFAULT_SCALE,
)
.unwrap(),
array.value(0)
Expand All @@ -953,7 +930,7 @@ mod tests {
Decimal256::from_big_int(
&value2,
DECIMAL256_MAX_PRECISION,
DECIMAL_DEFAULT_SCALE
DECIMAL_DEFAULT_SCALE,
)
.unwrap(),
array.value(2)
Expand Down