kumarUjjawal commented on code in PR #24703:
URL: https://github.com/apache/datafusion/pull/24703#discussion_r3877781312
##########
datafusion/functions/src/math/floor.rs:
##########
@@ -123,10 +133,7 @@ impl ScalarUDFImpl for FloorFunc {
}
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- match &arg_types[0] {
- DataType::Null => Ok(DataType::Float64),
- other => Ok(other.clone()),
- }
+ Ok(decimal_floor_ceil_return_type(&arg_types[0]))
Review Comment:
This changes the output type of a public SQL function. Any query that stores
`floor(decimal_col)` into a fixed schema, or reads `arrow_typeof`, sees a
different type. We should mention in upgrade guide
##########
datafusion/functions/src/math/floor.rs:
##########
@@ -280,17 +307,40 @@ impl ScalarUDFImpl for FloorFunc {
// DECIMAL(precision, scale) where precision ≤ 38 ->
Decimal128(precision, scale)
// DECIMAL(precision, scale) where precision > 38 ->
Decimal256(precision, scale)
// Decimal32 and Decimal64 are unreachable from SQL/SLT.
- ScalarValue::Decimal32(Some(n), precision, scale) => {
- preimage_bounds!(decimal: Decimal32, Decimal32Type, *n,
*precision, *scale)
+ //
+ // Simce floor()/ceil() do not preserve argument's scale, the
preimage bounds
+ // must be expressed in `arg`'s own (precision, scale), not the
literal's
+ ScalarValue::Decimal32(Some(n), lit_precision, lit_scale) => {
+ let DataType::Decimal32(arg_precision, arg_scale) =
+ info.get_data_type(&arg)?
+ else {
+ return Ok(PreimageResult::None);
+ };
+ preimage_bounds!(decimal: Decimal32, Decimal32Type, *n,
*lit_precision, *lit_scale, arg_precision, arg_scale)
}
- ScalarValue::Decimal64(Some(n), precision, scale) => {
- preimage_bounds!(decimal: Decimal64, Decimal64Type, *n,
*precision, *scale)
+ ScalarValue::Decimal64(Some(n), lit_precision, lit_scale) => {
+ let DataType::Decimal64(arg_precision, arg_scale) =
+ info.get_data_type(&arg)?
+ else {
+ return Ok(PreimageResult::None);
+ };
+ preimage_bounds!(decimal: Decimal64, Decimal64Type, *n,
*lit_precision, *lit_scale, arg_precision, arg_scale)
}
- ScalarValue::Decimal128(Some(n), precision, scale) => {
- preimage_bounds!(decimal: Decimal128, Decimal128Type, *n,
*precision, *scale)
+ ScalarValue::Decimal128(Some(n), lit_precision, lit_scale) => {
+ let DataType::Decimal128(arg_precision, arg_scale) =
+ info.get_data_type(&arg)?
Review Comment:
we could match on the `Ok` case and return `PreimageResult::None` otherwise?
##########
datafusion/functions/src/math/floor.rs:
##########
@@ -343,61 +393,90 @@ fn int_preimage_bounds<I: CheckedAdd + One + Copy>(n: I)
-> Option<(I, I)> {
Some((n, upper))
}
-/// Compute preimage bounds for floor function on decimal types.
-/// For floor(x) = n, the preimage is [n, n+1).
+/// Compute preimage bounds for floor/ceil functions on decimal types.
+/// Argument and literal have different precision and scales.
+///
+/// For floor(x) = n, the preimage is [n, n+1) in the argument's own scale.
/// Returns None if:
-/// - The value has a fractional part (floor always returns integers)
-/// - Adding 1 would overflow
+/// - The literal has a fractional part at its own scale
+/// - Rescaling to the argument's scale, or adding 1, would overflow
fn decimal_preimage_bounds<D: DecimalType>(
- value: D::Native,
- precision: u8,
- scale: i8,
+ lit_value: D::Native,
+ lit_precision: u8,
+ lit_scale: i8,
+ arg_precision: u8,
+ arg_scale: i8,
) -> Option<(D::Native, D::Native)>
where
D::Native: DecimalCast + ArrowNativeTypeOp + std::ops::Rem<Output =
D::Native>,
{
- // Use rescale_decimal to compute "1" at target scale (avoids manual pow)
- // Convert integer 1 (scale=0) to the target scale
+ if lit_scale > 0 {
+ let lit_factor: D::Native =
+ rescale_decimal::<D, D>(D::Native::ONE, 1, 0, lit_precision,
lit_scale)?;
+ if lit_value % lit_factor != D::Native::ZERO {
+ // The literal has a fractional part at its own scale
+ return None;
+ }
+ }
+
+ // Rescale the literal's (integer) value into the argument's own scale.
+ let base: D::Native = rescale_decimal::<D, D>(
+ lit_value,
+ lit_precision,
+ lit_scale,
+ arg_precision,
+ arg_scale,
+ )?;
+
+ // Use rescale_decimal to compute "1" at the argument's scale (avoids
manual pow)
let one_scaled: D::Native = rescale_decimal::<D, D>(
Review Comment:
this might be already there before the pr but would a guard that returns
`None` when `one_scaled` is zero be worth adding?
##########
datafusion/functions/src/math/decimal.rs:
##########
@@ -74,38 +82,66 @@ where
})
}
+/// Compute the return precision for floor/ceil result to accommodate the
result
+pub(super) fn decimal_floor_ceil_precision(
Review Comment:
can we reuse `calculate_new_precision_scale` with 0 decimal places?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
