mbutrovich commented on code in PR #27116:
URL: https://github.com/apache/datafusion/pull/27116#discussion_r4485153340
##########
datafusion/functions/src/math/floor.rs:
##########
@@ -826,4 +830,105 @@ mod tests {
"Expected None (not an Err) when the argument's type can't be
resolved"
);
}
+
+ #[test]
+ fn test_floor_decimal_scalar_zero_rows() {
+ use datafusion_expr::{ReturnFieldArgs, ScalarUDFImpl};
+
+ let udf = FloorFunc::new();
+
+ let test_scalars = vec![
+ ScalarValue::Decimal32(Some(150), 9, 2),
+ ScalarValue::Decimal32(Some(-150), 9, 2),
+ ScalarValue::Decimal32(None, 9, 2),
+ ScalarValue::Decimal64(Some(150), 18, 2),
+ ScalarValue::Decimal64(Some(-150), 18, 2),
+ ScalarValue::Decimal64(None, 18, 2),
+ ScalarValue::Decimal128(Some(150), 38, 2),
+ ScalarValue::Decimal128(Some(-150), 38, 2),
+ ScalarValue::Decimal128(None, 38, 2),
+ ScalarValue::Decimal256(Some(150.into()), 76, 2),
+ ScalarValue::Decimal256(Some((-150).into()), 76, 2),
+ ScalarValue::Decimal256(None, 76, 2),
+ ];
+
+ for arg in test_scalars {
+ let arg_fields = vec![Arc::new(Field::new("a", arg.data_type(),
true))];
+ let return_field = udf
+ .return_field_from_args(ReturnFieldArgs {
+ arg_fields: &arg_fields,
+ scalar_arguments: &[Some(&arg)],
+ })
+ .unwrap();
+
+ let result_zero_rows = udf
+ .invoke_with_args(ScalarFunctionArgs {
+ args: vec![ColumnarValue::Scalar(arg.clone())],
+ arg_fields: arg_fields.clone(),
+ number_rows: 0,
+ return_field: Arc::clone(&return_field),
+ config_options: Arc::new(
+ datafusion_common::config::ConfigOptions::default(),
+ ),
+ })
+ .unwrap();
+
+ let result_one_row = udf
+ .invoke_with_args(ScalarFunctionArgs {
+ args: vec![ColumnarValue::Scalar(arg)],
+ arg_fields,
+ number_rows: 1,
+ return_field,
+ config_options: Arc::new(
+ datafusion_common::config::ConfigOptions::default(),
+ ),
+ })
+ .unwrap();
+
+ let ColumnarValue::Scalar(scalar_zero_rows) = result_zero_rows
else {
+ panic!("Expected scalar result for zero rows");
+ };
+ let ColumnarValue::Scalar(scalar_one_row) = result_one_row else {
+ panic!("Expected scalar result for one row");
+ };
+ assert_eq!(scalar_zero_rows, scalar_one_row);
+ }
+ }
+
+ #[test]
+ fn test_floor_decimal_array_zero_rows() {
+ use arrow::array::Array;
+ use datafusion_expr::{ReturnFieldArgs, ScalarUDFImpl};
+
+ let udf = FloorFunc::new();
+ let array = Arc::new(
+ arrow::array::Decimal128Array::from(Vec::<Option<i128>>::new())
+ .with_precision_and_scale(38, 2)
+ .unwrap(),
+ );
+ let arg_fields = vec![Arc::new(Field::new("a",
array.data_type().clone(), true))];
+ let return_field = udf
+ .return_field_from_args(ReturnFieldArgs {
+ arg_fields: &arg_fields,
+ scalar_arguments: &[None],
+ })
+ .unwrap();
+
+ let result = udf
+ .invoke_with_args(ScalarFunctionArgs {
+ args: vec![ColumnarValue::Array(array)],
+ arg_fields,
+ number_rows: 0,
+ return_field,
+ config_options: Arc::new(
+ datafusion_common::config::ConfigOptions::default(),
+ ),
+ })
+ .unwrap();
+
+ match result {
+ ColumnarValue::Array(res_arr) => assert_eq!(res_arr.len(), 0),
+ _ => panic!("Expected array result"),
+ }
+ }
Review Comment:
Could we trim these the same way as in `ceil.rs`? The `floor` decimal cases
are in
[`scalar.slt`](https://github.com/apache/datafusion/blob/b6b2e0e5d5f119f46b7fb6288827aee7041ab00d/datafusion/sqllogictest/test_files/scalar.slt#L523-L571),
and `test_floor_decimal_array_zero_rows` passes on `main`. Importing
`ConfigOptions` also replaces the inline
`datafusion_common::config::ConfigOptions` paths.
```suggestion
#[test]
fn test_floor_decimal_scalar_zero_rows() {
use datafusion_common::config::ConfigOptions;
use datafusion_expr::ReturnFieldArgs;
let udf = FloorFunc::new();
let arg = ScalarValue::Decimal32(Some(150), 9, 2);
let arg_fields = vec![Arc::new(Field::new("a", arg.data_type(),
true))];
let return_field = udf
.return_field_from_args(ReturnFieldArgs {
arg_fields: &arg_fields,
scalar_arguments: &[Some(&arg)],
})
.unwrap();
let result = udf
.invoke_with_args(ScalarFunctionArgs {
args: vec![ColumnarValue::Scalar(arg)],
arg_fields,
number_rows: 0,
return_field,
config_options: Arc::new(ConfigOptions::default()),
})
.unwrap();
let ColumnarValue::Scalar(result) = result else {
panic!("expected a scalar result, got {result:?}");
};
assert_eq!(result, ScalarValue::Decimal32(Some(1), 8, 0));
}
```
##########
datafusion/functions/src/math/ceil.rs:
##########
@@ -211,3 +215,108 @@ impl ScalarUDFImpl for CeilFunc {
self.doc()
}
}
+
+#[cfg(test)]
+mod tests {
+ use std::sync::Arc;
+
+ use arrow::array::{Array, Decimal128Array};
+ use arrow::datatypes::Field;
+ use datafusion_common::ScalarValue;
+ use datafusion_common::config::ConfigOptions;
+ use datafusion_expr::{
+ ColumnarValue, ReturnFieldArgs, ScalarFunctionArgs, ScalarUDFImpl,
+ };
+
+ use crate::math::ceil::CeilFunc;
+
+ #[test]
+ fn test_ceil_decimal_scalar_zero_rows() {
+ let udf = CeilFunc::new();
+
+ let test_scalars = vec![
+ ScalarValue::Decimal32(Some(150), 9, 2),
+ ScalarValue::Decimal32(Some(-150), 9, 2),
+ ScalarValue::Decimal32(None, 9, 2),
+ ScalarValue::Decimal64(Some(150), 18, 2),
+ ScalarValue::Decimal64(Some(-150), 18, 2),
+ ScalarValue::Decimal64(None, 18, 2),
+ ScalarValue::Decimal128(Some(150), 38, 2),
+ ScalarValue::Decimal128(Some(-150), 38, 2),
+ ScalarValue::Decimal128(None, 38, 2),
+ ScalarValue::Decimal256(Some(150.into()), 76, 2),
+ ScalarValue::Decimal256(Some((-150).into()), 76, 2),
+ ScalarValue::Decimal256(None, 76, 2),
+ ];
+
+ for arg in test_scalars {
+ let arg_fields = vec![Arc::new(Field::new("a", arg.data_type(),
true))];
+ let return_field = udf
+ .return_field_from_args(ReturnFieldArgs {
+ arg_fields: &arg_fields,
+ scalar_arguments: &[Some(&arg)],
+ })
+ .unwrap();
+
+ let result_zero_rows = udf
+ .invoke_with_args(ScalarFunctionArgs {
+ args: vec![ColumnarValue::Scalar(arg.clone())],
+ arg_fields: arg_fields.clone(),
+ number_rows: 0,
+ return_field: Arc::clone(&return_field),
+ config_options: Arc::new(ConfigOptions::default()),
+ })
+ .unwrap();
+
+ let result_one_row = udf
+ .invoke_with_args(ScalarFunctionArgs {
+ args: vec![ColumnarValue::Scalar(arg)],
+ arg_fields,
+ number_rows: 1,
+ return_field,
+ config_options: Arc::new(ConfigOptions::default()),
+ })
+ .unwrap();
+
+ let ColumnarValue::Scalar(scalar_zero_rows) = result_zero_rows
else {
+ panic!("Expected scalar result for zero rows");
+ };
+ let ColumnarValue::Scalar(scalar_one_row) = result_one_row else {
+ panic!("Expected scalar result for one row");
+ };
+ assert_eq!(scalar_zero_rows, scalar_one_row);
+ }
+ }
+
+ #[test]
+ fn test_ceil_decimal_array_zero_rows() {
+ let udf = CeilFunc::new();
+ let array = Arc::new(
+ Decimal128Array::from(Vec::<Option<i128>>::new())
+ .with_precision_and_scale(38, 2)
+ .unwrap(),
+ );
+ let arg_fields = vec![Arc::new(Field::new("a",
array.data_type().clone(), true))];
+ let return_field = udf
+ .return_field_from_args(ReturnFieldArgs {
+ arg_fields: &arg_fields,
+ scalar_arguments: &[None],
+ })
+ .unwrap();
+
+ let result = udf
+ .invoke_with_args(ScalarFunctionArgs {
+ args: vec![ColumnarValue::Array(array)],
+ arg_fields,
+ number_rows: 0,
+ return_field,
+ config_options: Arc::new(ConfigOptions::default()),
+ })
+ .unwrap();
+
+ match result {
+ ColumnarValue::Array(res_arr) => assert_eq!(res_arr.len(), 0),
+ _ => panic!("Expected array result"),
+ }
+ }
Review Comment:
Could we trim these tests to one decimal scalar and assert the expected
value directly?
The fix is the `to_array` call at line 113, which runs before the match on
the decimal type. So every width, sign, and null in `test_scalars` takes the
same path, and any one of them fails without the fix. Value coverage for each
decimal width and for negative inputs is already in
[`scalar.slt`](https://github.com/apache/datafusion/blob/b6b2e0e5d5f119f46b7fb6288827aee7041ab00d/datafusion/sqllogictest/test_files/scalar.slt#L326-L374).
Comparing against the expected scalar also removes the second invocation with
`number_rows: 1`.
`test_ceil_decimal_array_zero_rows` passes on `main` too. For an array
input,
[`ColumnarValue::to_array`](https://github.com/apache/datafusion/blob/b6b2e0e5d5f119f46b7fb6288827aee7041ab00d/datafusion/expr-common/src/columnar_value.rs#L179-L184)
ignores `num_rows`, and the `else` branch is unchanged. So I think we can drop
it, along with the `Array` and `Decimal128Array` imports it uses.
```suggestion
use arrow::datatypes::Field;
use datafusion_common::ScalarValue;
use datafusion_common::config::ConfigOptions;
use datafusion_expr::{
ColumnarValue, ReturnFieldArgs, ScalarFunctionArgs, ScalarUDFImpl,
};
use crate::math::ceil::CeilFunc;
#[test]
fn test_ceil_decimal_scalar_zero_rows() {
let udf = CeilFunc::new();
let arg = ScalarValue::Decimal32(Some(150), 9, 2);
let arg_fields = vec![Arc::new(Field::new("a", arg.data_type(),
true))];
let return_field = udf
.return_field_from_args(ReturnFieldArgs {
arg_fields: &arg_fields,
scalar_arguments: &[Some(&arg)],
})
.unwrap();
let result = udf
.invoke_with_args(ScalarFunctionArgs {
args: vec![ColumnarValue::Scalar(arg)],
arg_fields,
number_rows: 0,
return_field,
config_options: Arc::new(ConfigOptions::default()),
})
.unwrap();
let ColumnarValue::Scalar(result) = result else {
panic!("expected a scalar result, got {result:?}");
};
assert_eq!(result, ScalarValue::Decimal32(Some(2), 8, 0));
}
```
--
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]
