Use Fixed-Length Array in BasicDecimal new and raw_value #2405
Conversation
Signed-off-by: remzi <13716567376yh@gmail.com>
HaoYang670
changed the title
fn Decimal:new be consistent with fn Decimal:try_new_bytesfn Decimal:new be consistent with fn Decimal:try_new_bytes and add length bound for Decimal::raw_value
| @@ -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() | |||
There was a problem hiding this comment.
Is there any impaction for performance?
Iter the decimalarray will call value_unchecked function.
There was a problem hiding this comment.
I guess there is no performance regression. I just move the try_into outside the fn Decimal::new().
There was a problem hiding this comment.
Ok, I will cp this commit to my benchmark branch #2360 and test it.
| /// 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.
I am not sure if we should mark this as an unsafe function.
There was a problem hiding this comment.
From the discussion #2387, we have not come to a consistent conclusion. It better not to change the API?
Do you agree that?
|
Could we merge this PR @liukun4515 @tustvold ? |
|
@HaoYang670 I think this had a logical merge conflict - would you mind fixing please 🙏 - https://github.com/apache/arrow-rs/runs/7806676366?check_suite_focus=true |
|
Benchmark runs are scheduled for baseline = 0e97491 and contender = 0c3c686. 0c3c686 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
Will file a PR to fix this. Wait me ~10 min. Thx. |
|
Filed #2432. |
tustvold
changed the title
fn Decimal:new be consistent with fn Decimal:try_new_bytes and add length bound for Decimal::raw_value
Signed-off-by: remzi 13716567376yh@gmail.com
Which issue does this PR close?
None.
Rationale for this change
fn Decimal:newandfn Decimal:try_new_bytesshould have same API.What changes are included in this PR?
Add length bound for
Decimal:newandDecimal::raw_value.Update docs
Are there any user-facing changes?
Yes.