Cast numeric to decimal256#2923
Conversation
e3aa805 to
bf7f475
Compare
|
@tustvold would it be possible to review this PR (so we can get it into the release candidate I plan to make tomorrow)? |
tustvold
left a comment
There was a problem hiding this comment.
I have a couple of reservations about merging this as is:
- The decimal256 conversion will silently wrap at the boundary of an i128
- The conversion in general is unchecked, which is inconsistent with the other numeric conversions
The former I think should be fixed before merge, the latter was already inconsistent for decimal128 so can probably slide
| // with_precision_and_scale validates the | ||
| // value is within range for the output precision |
There was a problem hiding this comment.
This comment isn't correct anymore
| // with_precision_and_scale validates the | ||
| // value is within range for the output precision |
| where | ||
| <T as ArrowPrimitiveType>::Native: AsPrimitive<i256>, | ||
| { | ||
| let mul: i256 = i256::from_i128(10_i128.pow(scale as u32)); |
There was a problem hiding this comment.
This can overflow
(i64::MAX as i128).checked_mul(10_i128.checked_pow(DECIMAL256_MAX_SCALE as _).unwrap()).unwrap()
I think this needs a pow function on i256 and to then compute based on this
There was a problem hiding this comment.
I suspect the max is DECIMAL128_MAX_SCALE which is 38
|
|
||
| // with_precision_and_scale validates the | ||
| // value is within range for the output precision | ||
| cast_primitive_to_decimal256(array, |v| v.as_().wrapping_mul(mul), precision, scale) |
There was a problem hiding this comment.
I am aware the decimal128 conversions did this before, but this can silently overflow. This is inconsistent with how we handle other integer conversions in cast_numeric_arrays which uses https://docs.rs/num-traits/latest/num_traits/cast/trait.NumCast.html
There was a problem hiding this comment.
We can fix decimal128 together later.
|
Merged. Thanks. |
|
Benchmark runs are scheduled for baseline = b4872b7 and contender = 73416f8. 73416f8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2922.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?