-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: Add decimal support for round #19384
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
Conversation
Jefffrey
left a comment
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.
Thanks for picking this up, left a few comments
|
@Jefffrey made some changes, take a look when you get the time. |
Jefffrey
left a comment
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.
Minor suggestion for refactor, otherwise looks good
| round_decimal_i256(value, scale, decimal_places) | ||
| } | ||
|
|
||
| fn round_decimal_i128( |
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.
We can replace these decimal implementations with a single generic version:
fn round_decimal<V: ArrowNativeTypeOp>(
value: V,
scale: i8,
decimal_places: i32,
) -> Result<V, ArrowError> {
let diff = scale as i32 - decimal_places;
if diff <= 0 {
return Ok(value);
}
let diff = diff as u32; // safe since we check sign above
// these can't truncate/overflow
let one = V::ONE;
let two = V::from_usize(2).unwrap();
let ten = V::from_usize(10).unwrap();
let factor = ten.pow_checked(diff).map_err(|_| {
ArrowError::ComputeError(format!(
"Overflow while rounding decimal with scale {scale} and decimal places {decimal_places}"
))
})?;
let mut quotient = value.div_wrapping(factor);
let remainder = value.mod_wrapping(factor);
// `factor` is an even number (10^n, n > 0), so `factor / 2` is the tie threshold
let threshold = factor.div_wrapping(two);
if remainder >= threshold {
quotient = quotient.add_checked(one).map_err(|_| {
ArrowError::ComputeError("Overflow while rounding decimal".into())
})?;
} else if remainder <= threshold.neg_wrapping() {
quotient = quotient.sub_checked(one).map_err(|_| {
ArrowError::ComputeError("Overflow while rounding decimal".into())
})?;
}
quotient
.mul_checked(factor)
.map_err(|_| ArrowError::ComputeError("Overflow while rounding decimal".into()))
}Then can call them directly like so:
Decimal128(precision, scale) => {
let result = calculate_binary_decimal_math::<
Decimal128Type,
Int32Type,
Decimal128Type,
_,
>(
value_array.as_ref(),
decimal_places,
|v, dp| round_decimal(v, *scale, dp), // <--- here
*precision,
*scale,
)?;
result as _
}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.
Done!
|
Thanks @kumarUjjawal |
Which issue does this PR close?
Rationale for this change
The math UDF lacks the support for the decimal for the round.
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?