Add Decimal128 API and use it in DecimalArray and DecimalBuilder
#1871
Conversation
github-actions
bot
added
arrow
| use std::cmp::Ordering; | ||
|
|
||
| #[derive(Clone, Debug)] | ||
| pub struct Decimal128 { |
There was a problem hiding this comment.
C++ Decimal128 has implemented some operators. I don't implement the same in this change. This change tries to be functionally equal with current i128 API. We can consider to have these operators if they are needed.
Codecov Report
@@ Coverage Diff @@
## master #1871 +/- ##
==========================================
- Coverage 83.46% 83.46% -0.01%
==========================================
Files 201 202 +1
Lines 57014 57069 +55
==========================================
+ Hits 47586 47630 +44
- Misses 9428 9439 +11
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| impl PartialEq<Self> for Decimal128 { | ||
| fn eq(&self, other: &Self) -> bool { |
There was a problem hiding this comment.
I'm a bit not sure about comparing Decimal128. Although C++ Decimal128 just compares two uint64 values. We also compare i128 directly currently (e.g., ord kernel).
But I'm still wondering it is correct to compare two values with different scale? E.g., 100_i128 (scale 2) and 100_i128 (scale 3)? Isn't it "1.00" and "0.100" respectively?
So I put an assert to check scale here.
There was a problem hiding this comment.
I think the assert is needed.
If two decimal128 has diff type(precision or scale), we can't compare the value of i128.
|
|
||
| use std::cmp::Ordering; | ||
|
|
||
| #[derive(Debug)] |
There was a problem hiding this comment.
Perhaps a doc comment or something
| } | ||
| } | ||
|
|
||
| pub fn new_from_i128(precision: usize, scale: usize, value: i128) -> Self { |
There was a problem hiding this comment.
I'm presuming we will want to make Decimal256 and Decimal128 generic versions of the same impl, and so I wonder how methods like this which explicitly name the type will translate? Maybe new_from_raw?
There was a problem hiding this comment.
I have considered making them generic versions. Because I only implement Decimal128 now, new_from_i128 is used to make Decimal128 fit into existing codes.
Next step I will implement Decimal256 and try generalise it with Decimal128. I think if it works, new_from_bytes (maybe rename to new_from_raw) will be the generalised API. new_from_i128 will be removed if the above idea works.
There was a problem hiding this comment.
I think we have some time before next release to have Decimal256 and generalise the API.
|
I want to take a look this pr, please hold it. |
|
|
||
| impl Eq for Decimal128 {} | ||
|
|
||
| impl Decimal128 { |
There was a problem hiding this comment.
Do we need a function to get the type of the decimal128 value?
There was a problem hiding this comment.
Decimal(p, s)? We can have it. Because it is easy to add API but harder to remove. The API starts from minimalism. Currently I keep it as less as possible only to be on par with existing functionality.
|
@viirya |
Like C++ Arrow Decimal256, we can represent the integer in an array of parts of it. C++ Arrow Decimal256 uses an uint64_t array. |
|
Thank you @tustvold @liukun4515. I'm going to merge this and keeping working on Decimal256 and generalise them. |
| let as_array = bytes.try_into(); | ||
| let value = match as_array { | ||
| Ok(v) if bytes.len() == 16 => i128::from_le_bytes(v), | ||
| _ => panic!("Input to Decimal128 is not 128bit integer."), |
There was a problem hiding this comment.
Wouldn't it be better to return a Result instead of panic-ing here ?
There was a problem hiding this comment.
Good idea. I will change it to Result. Thanks.
alamb
changed the title
Decimal128 API and use it in DecimalArray and DecimalBuilder
Which issue does this PR close?
Closes #1870.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?