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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
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 | ||
} | ||
|
||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 fnDecimal::new()
.There was a problem hiding this comment.
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.