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

Use Fixed-Length Array in BasicDecimal new and raw_value #2405

Merged
merged 2 commits into from Aug 12, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions arrow/src/array/array_decimal.rs
Expand Up @@ -123,6 +123,8 @@ impl<const BYTE_WIDTH: usize> BasicDecimalArray<BYTE_WIDTH> {
self.raw_value_data_ptr().offset(pos as isize),
Self::VALUE_LENGTH as usize,
)
.try_into()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any impaction for performance?
Iter the decimalarray will call value_unchecked function.

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 guess there is no performance regression. I just move the try_into outside the fn Decimal::new().

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I will cp this commit to my benchmark branch #2360 and test it.

.unwrap()
};
BasicDecimal::<BYTE_WIDTH>::new(self.precision(), self.scale(), raw_val)
}
Expand Down
11 changes: 5 additions & 6 deletions arrow/src/util/decimal.rs
Expand Up @@ -64,8 +64,7 @@ impl<const BYTE_WIDTH: usize> BasicDecimal<BYTE_WIDTH> {
Self::MAX_PRECISION_SCALE_CONSTRUCTOR_DEFAULT_TYPE.3;

/// Tries to create a decimal value from precision, scale and bytes.
/// If the length of bytes isn't same as the bit width of this decimal,
/// returning an error. The bytes should be stored in little-endian order.
/// The bytes should be stored in little-endian order.
///
/// Safety:
/// This method doesn't validate if the decimal value represented by the bytes
Expand Down Expand Up @@ -114,17 +113,17 @@ impl<const BYTE_WIDTH: usize> BasicDecimal<BYTE_WIDTH> {
/// Creates a decimal value from precision, scale, and bytes.
///
/// Safety:
/// This method doesn't check if the length of bytes is compatible with this decimal.
/// This method doesn't check if the precision and scale are valid.
/// Use `try_new_from_bytes` for safe constructor.
pub fn new(precision: usize, scale: usize, bytes: &[u8]) -> Self {
pub fn new(precision: usize, scale: usize, bytes: &[u8; BYTE_WIDTH]) -> Self {
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 am not sure if we should mark this as an unsafe function.

Copy link
Contributor

Choose a reason for hiding this comment

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

From the discussion #2387, we have not come to a consistent conclusion. It better not to change the API?
Do you agree that?

Self {
precision,
scale,
value: bytes.try_into().unwrap(),
value: *bytes,
}
}
/// Returns the raw bytes of the integer representation of the decimal.
pub fn raw_value(&self) -> &[u8] {
pub fn raw_value(&self) -> &[u8; BYTE_WIDTH] {
&self.value
}

Expand Down